From 6328b3ce7f5b18c3c0e25e605ca94ed5f6f5cb05 Mon Sep 17 00:00:00 2001 From: Eric Griffin Date: Wed, 26 Aug 2026 21:27:55 -0400 Subject: [PATCH 1/3] fix(trips): treat unrenderable dive weather as no weather Addresses two review comments on PR #1319. A dive whose weather lookup resolved nothing still stores Precipitation.none, because WeatherMapper never returns null precipitation. TripStoryDay.weather was therefore non-null for such a day, so the backfill skipped it and the header drew nothing: a permanently badge-free day. The renderability rule now lives on TripStoryDayWeather and is shared by three callers, the backfill skip decision, the header's precedence, and the entity's is-this-worth-storing check, so the three cannot drift apart. The repository also normalizes the day to local midnight on both read and write. It owns the (trip, date) uniqueness invariant, and a caller passing a time component, or a row arriving through sync from a peer, would otherwise store a second row for the same calendar day that no midnight-keyed lookup could see. --- .../trip_day_weather_repository.dart | 18 +++++- .../domain/entities/trip_day_weather.dart | 13 ++-- .../trips/domain/entities/trip_story_day.dart | 13 ++++ .../services/trip_day_weather_backfill.dart | 8 ++- .../widgets/story/trip_story_day_header.dart | 12 +++- .../trip_day_weather_repository_test.dart | 27 ++++++++ .../trip_day_weather_backfill_test.dart | 63 ++++++++++++++++++- .../story/trip_story_day_header_test.dart | 31 +++++++++ 8 files changed, 168 insertions(+), 17 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 399645d5d8..89bb58c64a 100644 --- a/lib/features/trips/data/repositories/trip_day_weather_repository.dart +++ b/lib/features/trips/data/repositories/trip_day_weather_repository.dart @@ -19,6 +19,17 @@ class TripDayWeatherRepository { final SyncRepository _syncRepository = SyncRepository(); final _log = LoggerService.forClass(TripDayWeatherRepository); + /// Local midnight 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 + /// a time component would otherwise store a second row for the same + /// calendar day, invisible to every midnight-keyed lookup and refetched on + /// every view. Reads normalize too, because a row can also arrive through + /// sync from a peer, bypassing this class entirely. + static int _dayKey(DateTime date) => + DateTime(date.year, date.month, date.day).millisecondsSinceEpoch; + /// Emits whenever `trip_day_weather` changes, so the display provider /// refreshes after a backfill write or a sync import. Stream watchWeatherChanges() => @@ -30,7 +41,10 @@ class TripDayWeatherRepository { final rows = await (_db.select( _db.tripDayWeather, )..where((t) => t.tripId.equals(tripId))).get(); - return {for (final row in rows) row.date: _mapRow(row)}; + return { + for (final row in rows) + _dayKey(DateTime.fromMillisecondsSinceEpoch(row.date)): _mapRow(row), + }; } catch (e, stackTrace) { _log.error( 'Failed to read weather for trip: $tripId', @@ -45,7 +59,7 @@ class TripDayWeatherRepository { Future upsert(domain.TripDayWeather weather) async { try { final now = DateTime.now().millisecondsSinceEpoch; - final dateMillis = weather.date.millisecondsSinceEpoch; + final dateMillis = _dayKey(weather.date); // Reuse the stored row's id when the day already has one: a peer may // have written its own uuid for this day, and replacing it under a new diff --git a/lib/features/trips/domain/entities/trip_day_weather.dart b/lib/features/trips/domain/entities/trip_day_weather.dart index a9ae4be434..fd1412e491 100644 --- a/lib/features/trips/domain/entities/trip_day_weather.dart +++ b/lib/features/trips/domain/entities/trip_day_weather.dart @@ -65,15 +65,10 @@ class TripDayWeather extends Equatable { /// carrying only those renders as nothing and would suppress the retry that /// a later archive update would satisfy. /// - /// [Precipitation.none] does not count. `WeatherMapper.mapPrecipitation` - /// returns non-null always, defaulting a missing reading to `none`, so - /// `none` cannot be read as evidence that the fetch resolved anything. It - /// also earns no glyph of its own in `weatherIconFor`, which falls through - /// to cloud cover. - bool get hasRenderableWeather => - airTemp != null || - cloudCover != null || - (precipitation != null && precipitation != Precipitation.none); + /// Delegates to [TripStoryDayWeather.isRenderable] so the rule that decides + /// what is worth storing is the same one that decides what the header can + /// draw, and the same one the backfill uses to judge a day's dive weather. + bool get hasRenderableWeather => toStoryWeather().isRenderable; /// The compact view model the day header consumes. TripStoryDayWeather toStoryWeather() => TripStoryDayWeather( diff --git a/lib/features/trips/domain/entities/trip_story_day.dart b/lib/features/trips/domain/entities/trip_story_day.dart index 000e73b5da..70d272153a 100644 --- a/lib/features/trips/domain/entities/trip_story_day.dart +++ b/lib/features/trips/domain/entities/trip_story_day.dart @@ -133,6 +133,19 @@ class TripStoryDayWeather extends Equatable { this.precipitation, }); + /// True when the day header's badge would actually draw something. + /// + /// [Precipitation.none] does not count, and that is the whole point of this + /// getter. `WeatherMapper.mapPrecipitation` never returns null: a missing + /// reading becomes `none`, so a dive whose weather lookup resolved nothing + /// still stores `none`. `weatherIconFor` gives `none` no glyph of its own, + /// so such a day renders as blank. Treating it as "this day has weather" + /// would leave the day badge-free forever. + bool get isRenderable => + airTemp != null || + cloudCover != null || + (precipitation != null && precipitation != Precipitation.none); + @override List get props => [airTemp, cloudCover, precipitation]; } diff --git a/lib/features/trips/domain/services/trip_day_weather_backfill.dart b/lib/features/trips/domain/services/trip_day_weather_backfill.dart index 05222ec407..0842cfa18f 100644 --- a/lib/features/trips/domain/services/trip_day_weather_backfill.dart +++ b/lib/features/trips/domain/services/trip_day_weather_backfill.dart @@ -45,7 +45,13 @@ class TripDayWeatherBackfill { // A dive that logged weather is the better source: it is what the diver // recorded. Never override it with a fetched summary. - if (day.weather != null) continue; + // + // Renderability, not mere presence, is the test. A dive whose weather + // lookup resolved nothing still stores Precipitation.none, because + // WeatherMapper never returns null precipitation, and that renders as a + // blank badge. Skipping on presence alone would leave such a day + // permanently badge-free. + if (day.weather?.isRenderable ?? false) continue; // A historical archive has nothing for a day that has not happened. if (day.kind == TripStoryDayKind.future) continue; diff --git a/lib/features/trips/presentation/widgets/story/trip_story_day_header.dart b/lib/features/trips/presentation/widgets/story/trip_story_day_header.dart index bea6ef77ee..cf55cc3427 100644 --- a/lib/features/trips/presentation/widgets/story/trip_story_day_header.dart +++ b/lib/features/trips/presentation/widgets/story/trip_story_day_header.dart @@ -61,9 +61,15 @@ class TripStoryDayHeader extends ConsumerWidget { ...day.siteNames, ].map((part) => part.trim()).where((part) => part.isNotEmpty).toList(); - // Dive-logged weather always wins: it is what the diver recorded, and a - // stored day summary is only ever a stand-in for days that logged none. - final weather = day.weather ?? storedWeather; + // Dive-logged weather wins: it is what the diver recorded, and a stored + // day summary is only ever a stand-in for days that logged none. It wins + // only when it can actually draw something, though: a dive whose weather + // lookup resolved nothing still carries Precipitation.none, and letting + // that mask a stored row would render the day blank. + final diveWeather = day.weather; + final weather = (diveWeather != null && diveWeather.isRenderable) + ? diveWeather + : (storedWeather ?? diveWeather); final units = UnitFormatter(ref.watch(settingsProvider)); final weatherBadge = _weatherBadge(context, theme, units, weather); 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 3686260aa5..503be868e0 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 @@ -119,6 +119,33 @@ void main() { expect(stored[day1.millisecondsSinceEpoch]!.airTemp, 25); }); + test('a date with a time component is stored under local 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. + 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); + }(); + }); + + test('the same day at two times of day stays one row', () async { + await repository.upsert(sample(date: DateTime(2026, 3, 8, 6))); + await repository.upsert( + sample(id: 'w2', date: DateTime(2026, 3, 8, 23), airTemp: 25), + ); + + final stored = await repository.getForTrip(testTripId); + + expect(stored, hasLength(1)); + expect(stored[day1.millisecondsSinceEpoch]!.airTemp, 25); + }); + test('two different days both persist', () async { await repository.upsert(sample()); await repository.upsert(sample(id: 'w2', date: day2, airTemp: 19)); diff --git a/test/features/trips/domain/services/trip_day_weather_backfill_test.dart b/test/features/trips/domain/services/trip_day_weather_backfill_test.dart index 70e6563e72..44e1152522 100644 --- a/test/features/trips/domain/services/trip_day_weather_backfill_test.dart +++ b/test/features/trips/domain/services/trip_day_weather_backfill_test.dart @@ -1,4 +1,5 @@ import 'package:flutter_test/flutter_test.dart'; +import 'package:submersion/core/constants/enums.dart'; import 'package:submersion/features/dive_log/domain/entities/dive.dart'; import 'package:submersion/features/trips/domain/entities/trip.dart'; import 'package:submersion/features/trips/domain/entities/trip_day_weather.dart'; @@ -28,8 +29,17 @@ void main() { ); } - Dive diveWith({double? airTemp}) => - Dive(id: 'd1', dateTime: DateTime(2026, 3, 8, 9), airTemp: airTemp); + Dive diveWith({ + double? airTemp, + CloudCover? cloudCover, + Precipitation? precipitation, + }) => Dive( + id: 'd1', + dateTime: DateTime(2026, 3, 8, 9), + airTemp: airTemp, + cloudCover: cloudCover, + precipitation: precipitation, + ); TripStoryDay day({ required int index, @@ -196,6 +206,55 @@ void main() { ]); }); + test( + 'a dive carrying only Precipitation.none does NOT count as weather', + () { + // WeatherMapper never returns null precipitation, so a dive whose + // lookup resolved nothing still stores `none`. The header draws no + // glyph for it, so treating it as "this day has weather" would skip the + // backfill and leave the day permanently badge-free. + final story = storyWith( + [ + day(index: 0, dives: [diveWith(precipitation: Precipitation.none)]), + ], + points: [pointFor(0)], + ); + + expect( + TripDayWeatherBackfill.targetsFor(story: story, stored: const {}), + hasLength(1), + ); + }, + ); + + test('a dive carrying active precipitation DOES count as weather', () { + final story = storyWith( + [ + day(index: 0, dives: [diveWith(precipitation: Precipitation.rain)]), + ], + points: [pointFor(0)], + ); + + expect( + TripDayWeatherBackfill.targetsFor(story: story, stored: const {}), + isEmpty, + ); + }); + + test('a dive carrying only cloud cover counts as weather', () { + final story = storyWith( + [ + day(index: 0, dives: [diveWith(cloudCover: CloudCover.overcast)]), + ], + points: [pointFor(0)], + ); + + expect( + TripDayWeatherBackfill.targetsFor(story: story, stored: const {}), + isEmpty, + ); + }); + test('a day date with a time component is normalized to midnight', () { // The story day's date should already be date-only, but a stored row is // keyed on midnight millis, so a stray time would never match and the diff --git a/test/features/trips/presentation/widgets/story/trip_story_day_header_test.dart b/test/features/trips/presentation/widgets/story/trip_story_day_header_test.dart index a6c4adc02e..0fde9196fa 100644 --- a/test/features/trips/presentation/widgets/story/trip_story_day_header_test.dart +++ b/test/features/trips/presentation/widgets/story/trip_story_day_header_test.dart @@ -291,6 +291,37 @@ void main() { expect(find.byType(CircularProgressIndicator), findsNothing); }); + testWidgets('stored weather wins when the dive weather renders nothing', ( + tester, + ) async { + // A dive whose lookup resolved nothing still carries + // Precipitation.none, which draws no glyph. The stored row is the only + // thing that can render, so it must not be masked. + final day = TripStoryDay( + date: DateTime(2026, 3, 8), + dayNumber: 2, + kind: TripStoryDayKind.past, + dives: [ + Dive( + id: 'd1', + dateTime: DateTime(2026, 3, 8, 9), + precipitation: Precipitation.none, + ), + ], + ); + + await pumpHeader( + tester, + day, + storedWeather: const TripStoryDayWeather( + airTemp: 22, + cloudCover: CloudCover.clear, + ), + ); + + expect(find.text('22°C'), findsOneWidget); + }); + testWidgets('dive-logged weather wins over stored weather', (tester) async { // What the diver recorded outranks a fetched day summary. final day = TripStoryDay( From 5cd3e2e362541170893c6f6d0ee0e1dae88cfd42 Mon Sep 17 00:00:00 2001 From: Eric Griffin Date: Wed, 26 Aug 2026 22:02:17 -0400 Subject: [PATCH 2/3] fix(trips): derive the trip day weather row id from trip and day A v4 id per device meant two devices that both fetched the same day inserted two rows. The serializer upserts by primary key, so the peer's differing id missed the ON CONFLICT target and hit the unique (trip_id, date) index instead: SqliteException 2067, thrown inside the merge transaction, which aborts the whole sync pull rather than one row. Reproduced in a test before fixing. The id is now a UUIDv5 over (tripId, dayMillis), following the same convention as importedDiveComputerId and qualityFindingId, whose docstring already warned that a unique constraint on a replicated table turns an inbound insert into a throw rather than a merge. The repository derives it and ignores whatever id a caller passes. _mapRow also normalizes the entity's date, not just the map key, so a row carrying a time component cannot hand a time-bearing date to downstream logic. --- .../trip_day_weather_repository.dart | 33 ++++++--- .../domain/entities/trip_day_weather.dart | 20 ++++++ .../providers/trip_day_weather_providers.dart | 8 +-- .../sync/trip_day_weather_sync_test.dart | 67 +++++++++++++++++-- 4 files changed, 105 insertions(+), 23 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 89bb58c64a..cd8e73af6d 100644 --- a/lib/features/trips/data/repositories/trip_day_weather_repository.dart +++ b/lib/features/trips/data/repositories/trip_day_weather_repository.dart @@ -8,6 +8,8 @@ import 'package:submersion/core/services/logger_service.dart'; 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 tripDayWeatherRowId; /// Reads and writes stored per-day trip weather. /// @@ -61,16 +63,19 @@ class TripDayWeatherRepository { final now = DateTime.now().millisecondsSinceEpoch; final dateMillis = _dayKey(weather.date); - // Reuse the stored row's id when the day already has one: a peer may - // have written its own uuid for this day, and replacing it under a new - // id would violate the unique index and orphan the peer's sync record. - final existing = - await (_db.select(_db.tripDayWeather)..where( - (t) => - t.tripId.equals(weather.tripId) & t.date.equals(dateMillis), - )) - .getSingleOrNull(); - final id = existing?.id ?? weather.id; + // The id is derived from (trip, day), never taken from the caller, so + // every device writing this day produces the same primary key and sync + // merges by id instead of colliding on the unique index. + final id = tripDayWeatherRowId( + tripId: weather.tripId, + dayMillis: dateMillis, + ); + + // Only to preserve createdAt across an update; insertOnConflictUpdate + // would otherwise overwrite it with this write's timestamp. + final existing = await (_db.select( + _db.tripDayWeather, + )..where((t) => t.id.equals(id))).getSingleOrNull(); await _db .into(_db.tripDayWeather) @@ -149,7 +154,13 @@ class TripDayWeatherRepository { return domain.TripDayWeather( id: row.id, tripId: row.tripId, - date: DateTime.fromMillisecondsSinceEpoch(row.date), + // Normalized, matching the map key getForTrip returns it under: a row + // 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)), + ), 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 fd1412e491..ed63935617 100644 --- a/lib/features/trips/domain/entities/trip_day_weather.dart +++ b/lib/features/trips/domain/entities/trip_day_weather.dart @@ -1,8 +1,28 @@ import 'package:equatable/equatable.dart'; +import 'package:uuid/uuid.dart'; import 'package:submersion/core/constants/enums.dart'; import 'package:submersion/features/trips/domain/entities/trip_story_day.dart'; +/// Fixed namespace for deterministic trip-day-weather ids (UUIDv5). +/// Never change: the ids already stored depend on it. +const String kTripDayWeatherNamespace = '3f1c8a52-9e47-4d6b-8b3a-16c9d0f27e45'; + +/// Deterministic row id for one trip day. +/// +/// The day is the identity, so the id must be derived from it rather than +/// minted per device. Two devices that both fetch the same day would +/// otherwise insert two rows, and the unique (trip_id, date) index turns the +/// second one into an inbound-sync failure rather than a merge: the +/// serializer upserts by primary key, so a differing id misses the conflict +/// 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; the repository +/// does that before calling here. +String tripDayWeatherRowId({required String tripId, required int dayMillis}) => + const Uuid().v5(kTripDayWeatherNamespace, '$tripId|$dayMillis'); + /// Stored historical weather for one trip day. /// /// Written only for days whose dives supply no weather of their own; a day diff --git a/lib/features/trips/presentation/providers/trip_day_weather_providers.dart b/lib/features/trips/presentation/providers/trip_day_weather_providers.dart index f337c35611..e16b69db6b 100644 --- a/lib/features/trips/presentation/providers/trip_day_weather_providers.dart +++ b/lib/features/trips/presentation/providers/trip_day_weather_providers.dart @@ -1,5 +1,3 @@ -import 'package:uuid/uuid.dart'; - import 'package:submersion/core/providers/provider.dart'; import 'package:submersion/features/trips/data/repositories/trip_day_weather_repository.dart'; import 'package:submersion/features/trips/domain/entities/trip_day_weather.dart'; @@ -52,8 +50,6 @@ final tripDayWeatherBackfillProvider = FutureProvider.family(( ); if (targets.isEmpty) return; - const uuid = Uuid(); - // Sequential on purpose: a two-week trip would otherwise open with a burst // of parallel requests, and rows landing one at a time let the day headers // fill in progressively. @@ -71,7 +67,9 @@ final tripDayWeatherBackfillProvider = FutureProvider.family(( final now = DateTime.now(); final row = TripDayWeather( - id: uuid.v4(), + // Ignored by the repository, which derives the id from (trip, day) so + // every device converges on one row. + id: '', tripId: tripId, date: target.date, latitude: target.latitude, diff --git a/test/core/services/sync/trip_day_weather_sync_test.dart b/test/core/services/sync/trip_day_weather_sync_test.dart index 781fb8f41c..66719a2aee 100644 --- a/test/core/services/sync/trip_day_weather_sync_test.dart +++ b/test/core/services/sync/trip_day_weather_sync_test.dart @@ -4,6 +4,7 @@ import 'package:submersion/core/data/repositories/sync_repository.dart'; import 'package:submersion/core/database/database.dart'; import 'package:submersion/core/services/sync/sync_data_serializer.dart'; import 'package:submersion/core/services/sync/sync_service.dart'; +import 'package:submersion/features/trips/domain/entities/trip_day_weather.dart'; import '../../../helpers/test_database.dart'; @@ -11,6 +12,9 @@ void main() { late AppDatabase db; late SyncDataSerializer serializer; + final dayMillis = DateTime(2026, 3, 8).millisecondsSinceEpoch; + final rowId = tripDayWeatherRowId(tripId: 'trip-1', dayMillis: dayMillis); + setUp(() async { db = await setUpTestDatabase(); serializer = SyncDataSerializer(); @@ -30,7 +34,10 @@ void main() { .into(db.tripDayWeather) .insert( TripDayWeatherCompanion.insert( - id: 'w-1', + id: tripDayWeatherRowId( + tripId: 'trip-1', + dayMillis: DateTime(2026, 3, 8).millisecondsSinceEpoch, + ), tripId: 'trip-1', date: DateTime(2026, 3, 8).millisecondsSinceEpoch, latitude: 12.16, @@ -47,7 +54,7 @@ void main() { tearDown(tearDownTestDatabase); test('tripDayWeather export, fetch, upsert, and delete round-trip', () async { - final record = await serializer.fetchRecord('tripDayWeather', 'w-1'); + final record = await serializer.fetchRecord('tripDayWeather', rowId); expect(record, isNotNull); expect(record!['airTemp'], 24.0); expect(record['cloudCover'], 'clear'); @@ -58,19 +65,19 @@ void main() { 'airTemp': 26.0, 'updatedAt': 2, }); - final merged = await serializer.fetchRecord('tripDayWeather', 'w-1'); + final merged = await serializer.fetchRecord('tripDayWeather', rowId); expect(merged!['airTemp'], 26.0); - expect(await serializer.recordIdsFor('tripDayWeather'), contains('w-1')); + expect(await serializer.recordIdsFor('tripDayWeather'), contains(rowId)); - await serializer.deleteRecord('tripDayWeather', 'w-1'); - expect(await serializer.fetchRecord('tripDayWeather', 'w-1'), isNull); + await serializer.deleteRecord('tripDayWeather', rowId); + expect(await serializer.fetchRecord('tripDayWeather', rowId), isNull); }); test('the delta export filters on the row own hlc', () async { await (db.update( db.tripDayWeather, - )..where((t) => t.id.equals('w-1'))).write( + )..where((t) => t.id.equals(rowId))).write( const TripDayWeatherCompanion(hlc: Value('2026-08-16T00:00:00.000-0000')), ); @@ -90,6 +97,52 @@ void main() { expect(await changesetCount('2026-08-15T00:00:00.000-0000'), 1); }); + test('a peer row for the same day merges instead of throwing', () async { + // Two devices that both fetch the same day must converge. A v4 id per + // device would insert a second row and violate the unique (trip_id, date) + // index, and because the merge runs in a transaction that aborts the + // whole sync pull, not just this row. + // The peer derives the same id from the same (trip, day). + await serializer.upsertRecord('tripDayWeather', { + 'id': tripDayWeatherRowId(tripId: 'trip-1', dayMillis: dayMillis), + 'tripId': 'trip-1', + 'date': dayMillis, + 'latitude': 12.16, + 'longitude': -68.28, + 'airTemp': 26.0, + 'weatherSource': 'openMeteo', + 'fetchedAt': 2, + 'createdAt': 2, + 'updatedAt': 2, + }); + + final rows = await db.select(db.tripDayWeather).get(); + expect(rows, hasLength(1)); + expect(rows.single.airTemp, 26.0); + }); + + test('the row id is derived from trip and day, not minted per device', () { + final day = DateTime(2026, 3, 8).millisecondsSinceEpoch; + + expect( + tripDayWeatherRowId(tripId: 'trip-1', dayMillis: day), + tripDayWeatherRowId(tripId: 'trip-1', dayMillis: day), + ); + expect( + tripDayWeatherRowId(tripId: 'trip-1', dayMillis: day), + isNot(tripDayWeatherRowId(tripId: 'trip-2', dayMillis: day)), + ); + expect( + tripDayWeatherRowId(tripId: 'trip-1', dayMillis: day), + isNot( + tripDayWeatherRowId( + tripId: 'trip-1', + dayMillis: DateTime(2026, 3, 9).millisecondsSinceEpoch, + ), + ), + ); + }); + test('tripDayWeather is registered as an hlc target', () { // An omission here is silent: _stampHlc no-ops on an unknown entity type, // the column stays NULL, and the incremental export's hlc > watermark From 47c12a59d3d1a3a9a53467841a0fe452cfcbd7a6 Mon Sep 17 00:00:00 2001 From: Eric Griffin Date: Thu, 27 Aug 2026 01:25:41 -0400 Subject: [PATCH 3/3] fix(trips): reconcile trip day weather rows upsert did not write A row for the same trip and calendar day that this repository did not write is not a hypothetical: a peer on a build that predates the derived id, or a database written before this class normalized, produces one. Two things went wrong with it. The upsert threw. The unique index is on (trip_id, date) but Drift emits ON CONFLICT("id"), so a foreign-id row on the same midnight is not a conflict the statement can absorb: the insert misses the ON CONFLICT target and hits the index instead. The day could not be written at all, not merely written twice. The read collapsed it arbitrarily. getForTrip keyed a map by normalized day over an unordered result, so which of two same-day rows showed depended on the order SQLite happened to return them in. upsert now deletes every same-day row whose id is not the derived one before inserting the canonical row, both in one transaction so a day is never left with its old row gone and no new one in its place. The strays are tombstoned after the commit: they are synced records, and dropping one without a tombstone lets the peer that sent it hand it straight back. createdAt is the minimum across the day's rows, because a stray is this day under an old id rather than a different record. Reads resolve deterministically instead: the canonical row wins, then the most recently updated, then the id. They deliberately do not clean up, since a write here would fire the table tick the display provider subscribes to and invalidate the read in flight. The backfill no longer passes a placeholder id. The local-midnight conversion moved next to tripDayWeatherRowId as tripDayMillis, so the caller derives the real id and the repository's _dayKey delegates to the same function; the two agree by construction rather than by comment. The repository still derives its own id and ignores the caller's. --- .../trip_day_weather_repository.dart | 163 +++++++++++++---- .../domain/entities/trip_day_weather.dart | 13 +- .../providers/trip_day_weather_providers.dart | 9 +- .../trip_day_weather_repository_test.dart | 165 ++++++++++++++++++ 4 files changed, 309 insertions(+), 41 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 cd8e73af6d..046e2030c7 100644 --- a/lib/features/trips/data/repositories/trip_day_weather_repository.dart +++ b/lib/features/trips/data/repositories/trip_day_weather_repository.dart @@ -1,3 +1,4 @@ +import 'package:collection/collection.dart'; import 'package:drift/drift.dart'; import 'package:submersion/core/constants/enums.dart'; @@ -9,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 tripDayWeatherRowId; + show tripDayMillis, tripDayWeatherRowId; /// Reads and writes stored per-day trip weather. /// @@ -29,8 +30,7 @@ class TripDayWeatherRepository { /// calendar day, invisible to every midnight-keyed lookup and refetched on /// every view. Reads normalize too, because a row can also arrive through /// sync from a peer, bypassing this class entirely. - static int _dayKey(DateTime date) => - DateTime(date.year, date.month, date.day).millisecondsSinceEpoch; + static int _dayKey(DateTime date) => tripDayMillis(date); /// Emits whenever `trip_day_weather` changes, so the display provider /// refreshes after a backfill write or a sync import. @@ -38,15 +38,26 @@ class TripDayWeatherRepository { _db.tableUpdates(TableUpdateQuery.onTable(_db.tripDayWeather)); /// Stored weather for a trip, keyed by `date.millisecondsSinceEpoch`. + /// + /// One entry per calendar day. Where more than one row lands on the same + /// day, [_preferred] picks which one shows, and explains how a second row + /// gets there in the first place. Future> getForTrip(String tripId) async { try { final rows = await (_db.select( _db.tripDayWeather, )..where((t) => t.tripId.equals(tripId))).get(); - return { - for (final row in rows) - _dayKey(DateTime.fromMillisecondsSinceEpoch(row.date)): _mapRow(row), - }; + + final winners = {}; + for (final row in rows) { + final day = _dayKey(DateTime.fromMillisecondsSinceEpoch(row.date)); + final held = winners[day]; + winners[day] = held == null + ? row + : _preferred(held, row, tripId: tripId, dayMillis: day); + } + + return winners.map((day, row) => MapEntry(day, _mapRow(row))); } catch (e, stackTrace) { _log.error( 'Failed to read weather for trip: $tripId', @@ -71,37 +82,67 @@ class TripDayWeatherRepository { dayMillis: dateMillis, ); + final sameDay = await _rowsForDay( + tripId: weather.tripId, + dayMillis: dateMillis, + ); // Only to preserve createdAt across an update; insertOnConflictUpdate - // would otherwise overwrite it with this write's timestamp. - final existing = await (_db.select( - _db.tripDayWeather, - )..where((t) => t.id.equals(id))).getSingleOrNull(); - - await _db - .into(_db.tripDayWeather) - .insertOnConflictUpdate( - TripDayWeatherCompanion( - id: Value(id), - tripId: Value(weather.tripId), - date: Value(dateMillis), - latitude: Value(weather.latitude), - longitude: Value(weather.longitude), - airTemp: Value(weather.airTemp), - cloudCover: Value(weather.cloudCover?.name), - precipitation: Value(weather.precipitation?.name), - windSpeed: Value(weather.windSpeed), - windDirection: Value(weather.windDirection?.name), - humidity: Value(weather.humidity), - surfacePressure: Value(weather.surfacePressure), - weatherCode: Value(weather.weatherCode), - weatherSource: Value(weather.weatherSource.name), - fetchedAt: Value(weather.fetchedAt.millisecondsSinceEpoch), - createdAt: Value( - existing?.createdAt ?? weather.createdAt.millisecondsSinceEpoch, + // would otherwise overwrite it with this write's timestamp. A stray is + // this day under an old id rather than a different record, so the day + // keeps the age it already had when one is absorbed. + final createdAt = sameDay.map((r) => r.createdAt).minOrNull; + final strays = sameDay.where((r) => r.id != id).map((r) => r.id).toList(); + + await _db.transaction(() async { + // Strays go before the insert, not after. insertOnConflictUpdate + // targets the primary key, so a stray sitting on this same + // (trip_id, date) is not a conflict it can absorb: the insert misses + // the ON CONFLICT target and hits the unique index instead, which + // throws and fails the whole write. One transaction, so a day is + // never left with its old row deleted and no new one in its place. + if (strays.isNotEmpty) { + await (_db.delete( + _db.tripDayWeather, + )..where((t) => t.id.isIn(strays))).go(); + } + + await _db + .into(_db.tripDayWeather) + .insertOnConflictUpdate( + TripDayWeatherCompanion( + id: Value(id), + tripId: Value(weather.tripId), + date: Value(dateMillis), + latitude: Value(weather.latitude), + longitude: Value(weather.longitude), + airTemp: Value(weather.airTemp), + cloudCover: Value(weather.cloudCover?.name), + precipitation: Value(weather.precipitation?.name), + windSpeed: Value(weather.windSpeed), + windDirection: Value(weather.windDirection?.name), + humidity: Value(weather.humidity), + surfacePressure: Value(weather.surfacePressure), + weatherCode: Value(weather.weatherCode), + weatherSource: Value(weather.weatherSource.name), + fetchedAt: Value(weather.fetchedAt.millisecondsSinceEpoch), + createdAt: Value( + createdAt ?? weather.createdAt.millisecondsSinceEpoch, + ), + updatedAt: Value(now), ), - updatedAt: Value(now), - ), - ); + ); + }); + + // After the row is in place, so a failed write leaves no tombstone for + // a day that still has its original row. A stray is a synced record: + // dropping it without one lets the peer that sent it hand it back on + // the next pull. + for (final stray in strays) { + await _syncRepository.logDeletion( + entityType: 'tripDayWeather', + recordId: stray, + ); + } await _syncRepository.markRecordPending( entityType: 'tripDayWeather', @@ -150,6 +191,56 @@ 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. + Future> _rowsForDay({ + required String tripId, + required int dayMillis, + }) async { + final rows = await (_db.select( + _db.tripDayWeather, + )..where((t) => t.tripId.equals(tripId))).get(); + return rows + .where( + (r) => + _dayKey(DateTime.fromMillisecondsSinceEpoch(r.date)) == dayMillis, + ) + .toList(); + } + + /// Which of two rows for the same calendar day to show. + /// + /// Two rows reach one day only when [upsert] did not write one of them: a + /// peer on a build that predates the derived id, or a database written + /// before this class normalized. [upsert] clears them out, but a read can + /// land between a sync import and the next write, and it cannot tidy up + /// itself: a delete here would fire the table tick that the display + /// provider subscribes to and invalidate the read in flight. So it chooses. + /// + /// The canonical row wins, so what shows now is what the next upsert keeps. + /// Failing that the most recently updated wins, and an exact tie falls back + /// to the id, so the answer never depends on the order SQLite returned the + /// rows in. + TripDayWeatherData _preferred( + TripDayWeatherData a, + TripDayWeatherData b, { + required String tripId, + required int dayMillis, + }) { + final canonicalId = tripDayWeatherRowId( + tripId: tripId, + dayMillis: dayMillis, + ); + if (a.id == canonicalId) return a; + if (b.id == canonicalId) return b; + if (a.updatedAt != b.updatedAt) return a.updatedAt > b.updatedAt ? a : b; + return a.id.compareTo(b.id) <= 0 ? a : b; + } + domain.TripDayWeather _mapRow(TripDayWeatherData row) { return domain.TripDayWeather( id: row.id, diff --git a/lib/features/trips/domain/entities/trip_day_weather.dart b/lib/features/trips/domain/entities/trip_day_weather.dart index ed63935617..227b404b6b 100644 --- a/lib/features/trips/domain/entities/trip_day_weather.dart +++ b/lib/features/trips/domain/entities/trip_day_weather.dart @@ -8,6 +8,15 @@ 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 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. +int tripDayMillis(DateTime date) => + DateTime(date.year, date.month, date.day).millisecondsSinceEpoch; + /// Deterministic row id for one trip day. /// /// The day is the identity, so the id must be derived from it rather than @@ -18,8 +27,8 @@ const String kTripDayWeatherNamespace = '3f1c8a52-9e47-4d6b-8b3a-16c9d0f27e45'; /// 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; the repository -/// does that before calling here. +/// [dayMillis] must already be normalized to local midnight; pass it through +/// [tripDayMillis]. String tripDayWeatherRowId({required String tripId, required int dayMillis}) => const Uuid().v5(kTripDayWeatherNamespace, '$tripId|$dayMillis'); diff --git a/lib/features/trips/presentation/providers/trip_day_weather_providers.dart b/lib/features/trips/presentation/providers/trip_day_weather_providers.dart index e16b69db6b..2591b4ff15 100644 --- a/lib/features/trips/presentation/providers/trip_day_weather_providers.dart +++ b/lib/features/trips/presentation/providers/trip_day_weather_providers.dart @@ -66,10 +66,13 @@ final tripDayWeatherBackfillProvider = FutureProvider.family(( if (weather == null) continue; final now = DateTime.now(); + final dayMillis = tripDayMillis(target.date); final row = TripDayWeather( - // Ignored by the repository, which derives the id from (trip, day) so - // every device converges on one row. - id: '', + // The repository derives this same id from (trip, day) and never takes + // the caller's, so every device converges on one row. Derived here too + // rather than left as a placeholder: an entity carrying an id that is + // not its own reaches logs and any future validation as a lie. + id: tripDayWeatherRowId(tripId: tripId, dayMillis: dayMillis), tripId: tripId, date: target.date, latitude: target.latitude, 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 503be868e0..4379f53a3d 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 @@ -1,5 +1,9 @@ +import 'package:drift/drift.dart' show Value; import 'package:flutter_test/flutter_test.dart'; import 'package:submersion/core/constants/enums.dart'; +import 'package:submersion/core/data/repositories/sync_repository.dart'; +import 'package:submersion/core/database/database.dart' as db; +import 'package:submersion/core/services/database_service.dart'; import 'package:submersion/features/trips/data/repositories/trip_day_weather_repository.dart'; import 'package:submersion/features/trips/data/repositories/trip_repository.dart'; import 'package:submersion/features/trips/domain/entities/trip.dart'; @@ -203,4 +207,165 @@ void main() { expect(emissions, isNotEmpty); }); }); + + group('rows upsert did not write', () { + // Every row in this group goes straight into the table, because that is + // the only way one can carry an id other than the derived one: a peer on + // an older build of this feature, or a database written before the id + // became deterministic. Reconciling rows it did not create is exactly + // what the repository is being asked to do here. + Future insertRaw({ + required String id, + required DateTime date, + double? airTemp, + int updatedAt = 0, + int createdAt = 0, + }) async { + await DatabaseService.instance.database + .into(DatabaseService.instance.database.tripDayWeather) + .insert( + db.TripDayWeatherCompanion( + id: Value(id), + tripId: Value(testTripId), + date: Value(date.millisecondsSinceEpoch), + latitude: const Value(12.16), + longitude: const Value(-68.28), + airTemp: Value(airTemp), + weatherSource: Value(WeatherSource.openMeteo.name), + fetchedAt: const Value(0), + createdAt: Value(createdAt), + updatedAt: Value(updatedAt), + ), + ); + } + + Future> allRows() => DatabaseService + .instance + .database + .select(DatabaseService.instance.database.tripDayWeather) + .get(); + + test('upsert replaces a same-day row stored under a foreign id', () async { + // The unique index is on (trip_id, date) but insertOnConflictUpdate + // targets the primary key, so a foreign-id row on the same midnight + // makes the canonical insert miss the conflict target and hit the + // index. Without cleanup this throws rather than merging. + await insertRaw(id: 'from-a-peer', date: day1, airTemp: 10); + + await repository.upsert(sample(airTemp: 25)); + + final rows = await allRows(); + expect(rows, hasLength(1)); + expect(rows.single.airTemp, 25); + expect(rows.single.id, isNot('from-a-peer')); + }); + + test('upsert replaces a same-day row stored off midnight', () async { + await insertRaw( + id: 'from-an-older-build', + date: DateTime(2026, 3, 8, 17, 30), + airTemp: 10, + ); + + await repository.upsert(sample(airTemp: 25)); + + final rows = await allRows(); + expect(rows, hasLength(1)); + expect(rows.single.date, day1.millisecondsSinceEpoch); + expect(rows.single.airTemp, 25); + }); + + test('a row for another day is left alone', () async { + await insertRaw(id: 'other-day', date: day2, airTemp: 10); + + await repository.upsert(sample()); + + final rows = await allRows(); + expect(rows, hasLength(2)); + expect(rows.map((r) => r.id), contains('other-day')); + }); + + test('replacing a stray logs its deletion for sync', () async { + // A stray is a synced record. Dropping it without a tombstone lets the + // peer that sent it hand it straight back on the next pull. + await insertRaw(id: 'from-a-peer', date: day1); + + await repository.upsert(sample()); + + final deletions = await SyncRepository().getAllDeletions(); + expect( + deletions.where( + (d) => + d.entityType == 'tripDayWeather' && d.recordId == 'from-a-peer', + ), + hasLength(1), + ); + }); + + test('upsert keeps the createdAt of the stray it absorbs', () async { + // The stray is this day's row under an old id, not a different record, + // so the day keeps the age it already had. + await insertRaw( + id: 'from-a-peer', + date: DateTime(2026, 3, 8, 17, 30), + createdAt: 1000, + ); + + await repository.upsert(sample()); + + expect((await allRows()).single.createdAt, 1000); + }); + + test( + 'getForTrip prefers the canonical row over a same-day stray', + () async { + // Reads land between a sync import and the next upsert, so the choice + // cannot wait for the write side to tidy up, and it cannot depend on + // the order SQLite happens to return rows in. + await repository.upsert(sample(airTemp: 25)); + await insertRaw( + id: 'from-a-peer', + date: DateTime(2026, 3, 8, 17, 30), + airTemp: 10, + updatedAt: 9999999, + ); + + final stored = await repository.getForTrip(testTripId); + + expect(stored, hasLength(1)); + expect(stored[day1.millisecondsSinceEpoch]!.airTemp, 25); + }, + ); + + test('getForTrip falls back to the most recently updated stray', () async { + await insertRaw( + id: 'peer-a', + date: DateTime(2026, 3, 8, 6), + airTemp: 10, + updatedAt: 100, + ); + await insertRaw( + id: 'peer-b', + date: DateTime(2026, 3, 8, 23), + airTemp: 20, + updatedAt: 200, + ); + + final stored = await repository.getForTrip(testTripId); + + expect(stored, hasLength(1)); + expect(stored[day1.millisecondsSinceEpoch]!.airTemp, 20); + }); + + test('getForTrip does not write while resolving strays', () async { + // Reads stay pure: a cleanup here would fire the table tick and + // invalidate the provider that just read. + await insertRaw(id: 'peer-a', date: DateTime(2026, 3, 8, 6)); + await insertRaw(id: 'peer-b', date: DateTime(2026, 3, 8, 23)); + + await repository.getForTrip(testTripId); + + expect(await allRows(), hasLength(2)); + }); + }); }