Address review comments: unrenderable dive weather and day-key normalization

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';
@@ -8,6 +9,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 tripDayMillis, tripDayWeatherRowId;
 
 /// Reads and writes stored per-day trip weather.
 ///
@@ -19,18 +22,42 @@ 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) => tripDayMillis(date);
+
   /// Emits whenever `trip_day_weather` changes, so the display provider
   /// refreshes after a backfill write or a sync import.
   Stream<void> watchWeatherChanges() =>
       _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<Map<int, domain.TripDayWeather>> 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) row.date: _mapRow(row)};
+
+      final winners = <int, TripDayWeatherData>{};
+      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',
@@ -45,44 +72,77 @@ class TripDayWeatherRepository {
   Future<void> upsert(domain.TripDayWeather weather) async {
     try {
       final now = DateTime.now().millisecondsSinceEpoch;
-      final dateMillis = weather.date.millisecondsSinceEpoch;
-
-      // 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;
-
-      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,
+      final dateMillis = _dayKey(weather.date);
+
+      // 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,
+      );
+
+      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. 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',
@@ -131,11 +191,67 @@ 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<List<TripDayWeatherData>> _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,
       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,

lib/features/trips/domain/entities/trip_day_weather.dart

@@ -1,8 +1,37 @@
 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';
+
+/// 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
+/// 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; pass it through
+/// [tripDayMillis].
+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
@@ -65,15 +94,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(

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<Object?> get props => [airTemp, cloudCover, precipitation];
 }

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;

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<void, String>((
   );
   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.
@@ -70,8 +66,13 @@ final tripDayWeatherBackfillProvider = FutureProvider.family<void, String>((
     if (weather == null) continue;
 
     final now = DateTime.now();
+    final dayMillis = tripDayMillis(target.date);
     final row = TripDayWeather(
-      id: uuid.v4(),
+      // 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,

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);