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

[ericgriffin]

Addresses the two review comments on #1319. Stacked on worktree-trip-day-weather, so the diff here is just the two fixes.

1. Unrenderable dive weather blocked the backfill

WeatherRepository.fetchAndSaveWeather writes precipitation unconditionally from WeatherMapper, which never returns null: a missing reading becomes Precipitation.none. So a dive whose archive lookup resolved nothing carries none and nothing else.

TripStoryDay.weather is non-null for that day, so the backfill skipped it as "already has weather". But weatherIconFor gives none no glyph and there is no temperature, so the header drew nothing. The day was permanently badge-free, and nothing would ever fill it.

This is the same root cause as the hasRenderableWeather guard already in the branch, which I had applied only to the rows being written and not to the skip decision.

The rule now lives in one place, TripStoryDayWeather.isRenderable, with three callers:

• the backfill's skip decision (does this day's dive weather actually supply anything?)
• the header's precedence (dive weather wins only when it can draw something, otherwise the stored row shows instead of being masked)
• TripDayWeather.hasRenderableWeather, which now delegates rather than restating the condition

Keeping them on one definition is the point: three copies of "is this worth showing" would drift.

2. Day identity normalized inside the repository

upsert used weather.date.millisecondsSinceEpoch directly. The only current caller normalizes first, but the repository owns the (trip, date) uniqueness invariant and should enforce it rather than trust every caller. A DateTime carrying a time component would store a second row for the same calendar day, invisible to every midnight-keyed lookup and refetched on every view.

Normalization applies on read as well as write, because a row can also arrive through sync from a peer and bypass this class entirely.

3. Rows upsert did not write

A second round of review asked what happens to a row for the same trip and calendar day that this repository did not write: a peer on a build predating the derived id, or a database written before this class normalized. Two things went wrong with it.

The upsert threw. The unique index is on (trip_id, date), but Drift's insertOnConflictUpdate emits ON CONFLICT("id"). A foreign-id row on the same midnight is therefore not a conflict the statement can absorb: the insert misses the ON CONFLICT target and hits the index instead, failing with SqliteException(2067). The day could not be written at all, not merely written twice. The off-midnight variant is the milder case, where the dates differ, the index does not fire, and you get a silent duplicate.

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 returned 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 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 of cleaning up: canonical row, then most recently updated, then id. A write on the read path would fire the table tick tripDayWeatherProvider subscribes to and invalidate the read in flight.

The backfill also 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.

Testing

• 3 backfill tests: none-only precipitation is not weather, active precipitation is, cloud-cover-only is.
• 1 header test: a stored row is not masked by unrenderable dive weather.
• 2 repository tests: a time-carrying date stores under local midnight, and the same day at two times of day stays one row.
• 8 repository tests for rows upsert did not write: a foreign-id row on the same midnight, an off-midnight row, a row for another day left alone, the stray's deletion logged for sync, createdAt carried across, the canonical row preferred on read, the most recently updated preferred otherwise, and reads leaving the table untouched.

692 trips and weather tests pass. flutter analyze clean.

[Copilot]

Pull request overview

This PR addresses follow-up review items from #1319 by (1) preventing “unrenderable” dive weather (precipitation = none only) from masking stored trip-day weather, and (2) enforcing day-key normalization (local midnight) inside the trip-day weather repository.

Changes:

• Introduces TripStoryDayWeather.isRenderable and uses it to drive backfill skip logic and header precedence.
• Delegates TripDayWeather.hasRenderableWeather to the shared renderability rule to avoid drift.
• Normalizes (trip, date) identity inside TripDayWeatherRepository (write + read keying) and adds targeted regression tests.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
test/features/trips/presentation/widgets/story/trip_story_day_header_test.dart	Adds coverage ensuring stored weather is shown when dive weather can’t render.
test/features/trips/domain/services/trip_day_weather_backfill_test.dart	Adds tests for renderability-based backfill targeting (none-only precipitation, active precip, cloud-cover-only).
test/features/trips/data/repositories/trip_day_weather_repository_test.dart	Adds tests ensuring time-bearing dates are normalized and don’t create duplicate rows.
lib/features/trips/presentation/widgets/story/trip_story_day_header.dart	Updates weather precedence so dive weather wins only when it can render.
lib/features/trips/domain/services/trip_day_weather_backfill.dart	Updates backfill skip condition to use isRenderable instead of non-null presence.
lib/features/trips/domain/entities/trip_story_day.dart	Adds TripStoryDayWeather.isRenderable as the shared “badge would draw” rule.
lib/features/trips/domain/entities/trip_day_weather.dart	Delegates hasRenderableWeather to TripStoryDayWeather.isRenderable.
lib/features/trips/data/repositories/trip_day_weather_repository.dart	Adds _dayKey normalization and uses it for storage + normalized map keys on read.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (1)

lib/features/trips/data/repositories/trip_day_weather_repository.dart:145

• upsert deletes same-day stray rows inside a DB transaction, then logs their tombstones afterward. If _syncRepository.logDeletion throws (e.g. clock config / DB error), the strays are already permanently deleted locally but no tombstone is recorded, so a peer that originally sent the stray can re-send it on the next pull and you may reintroduce duplicates.

Consider making stray deletion + tombstone logging atomic, e.g. by adding a SyncRepository.logDeletionWithExecutor/logDeletionInTransaction helper that can participate in the existing _db.transaction, or by writing the deletion_log rows using the same transaction executor as the stray delete + canonical insert (so failures roll back everything together).

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

      // 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,
        );
      }