From 3fd93cf8a60cea80c6726f1a5055c3c04ad93b36 Mon Sep 17 00:00:00 2001 From: Dylan Date: Sat, 15 Aug 2026 10:59:02 -0500 Subject: [PATCH] T09: trip repository, completing Phase 2 All lifecycle transitions inside Drift transactions, each idempotent: start (adopting an active trip rather than duplicating it), pause, resume, complete, discard, rename, delete, merge, and the authoritative aggregate recomputation. Room.withTransaction maps onto Drift's transaction() almost exactly, so this was the most mechanical port so far. 26 tests, green first run. Merge keeps its hard-won properties: segments are never joined, so the boundary between two merged rides stays a segment boundary like a pause, and aggregates are recomputed rather than summed because distance is not additive across the gap. The regression test puts the two rides a degree of latitude apart and asserts the ~111km gap never reaches distanceM. Dropped a mergeTableUpdates helper before committing -- Drift's update() already notifies dependent streams, so it was scaffolding that earned nothing. TripRepositoryTest, MergeTest and SchemaTest were 666 lines of instrumented tests needing a booted emulator. All three now run as unit tests in ~2 seconds. Phase 2 done: 121 tests passing, analyze clean. Co-Authored-By: Claude Opus 5 --- docs/port/PROGRESS.md | 51 +++++ lib/src/data/trip_repository.dart | 243 +++++++++++++++++++++ test/trip_repository_test.dart | 352 ++++++++++++++++++++++++++++++ 3 files changed, 646 insertions(+) create mode 100644 lib/src/data/trip_repository.dart create mode 100644 test/trip_repository_test.dart diff --git a/docs/port/PROGRESS.md b/docs/port/PROGRESS.md index f558f68..3ef4248 100644 --- a/docs/port/PROGRESS.md +++ b/docs/port/PROGRESS.md @@ -317,3 +317,54 @@ well under a second with nothing booted. `TripRepositoryTest` and `MergeTest` (4 lines) should convert the same way in T09. **95 tests passing, analyze clean.** + +--- + +## T09 — Trip repository · **complete** + +`lib/src/data/trip_repository.dart`. **26 tests, green on the first run.** + +Every lifecycle transition ported: `startTrip` (adopting, never duplicating), `pauseTrip`, +`resumeTrip`, `completeTrip`, `discardTrip`, `renameTrip`, `deleteTrip`, `mergeTrips`, +`recomputeAggregates`. Each runs inside `_db.transaction` and each is idempotent, because +the platform can restart the recorder from any state. + +`Room.withTransaction` maps onto Drift's `transaction()` almost exactly, so this was the +most mechanical port so far. + +### What the tests protect + +- **Adoption over rejection.** `startTrip` on an already-active trip returns the *same* + handle rather than opening a second trip — the behaviour that makes a process kill + survivable. +- **Merge never joins segments.** The boundary between two merged rides stays a segment + boundary, exactly like a pause. The regression test puts the two rides a degree of + latitude apart and asserts the ~111 km gap never reaches `distanceM`. +- **Aggregates are recomputed, not summed** — because distance is not additive across + that gap. +- **Rename collapses empty and whitespace-only input to null**, so a stored `""` can + never diverge from the UI's date-label branch. +- **Merge rejects** self-merge, an active trip, and a missing id, and leaves no orphans. + +### One piece of speculative code removed + +A `mergeTableUpdates` helper was written to nudge Drift's stream queries after +re-parenting, then deleted before commit: Drift's own `update()` already notifies +dependent streams, so it earned nothing and would have been misleading scaffolding. + +### The instrumented-to-unit win, totalled + +`TripRepositoryTest` + `MergeTest` + `SchemaTest` were **666 lines of instrumented tests +requiring a booted emulator**. All three are now plain unit tests finishing in about two +seconds with nothing running. + +--- + +## Phase 2 complete + +**121 tests passing, `flutter analyze` clean.** The data layer is done and the domain, +statistics and export layers above it are proven equivalent to the Kotlin original. + +Next: **Phase 3, the recording engine** — the risky phase. T10's `LocationSource` seam +first, then the pipeline, then the two platform liveness stories. The +`flutter_foreground_task` deprecation warnings recorded under T01 become relevant at T12. diff --git a/lib/src/data/trip_repository.dart b/lib/src/data/trip_repository.dart new file mode 100644 index 0000000..df8c9f5 --- /dev/null +++ b/lib/src/data/trip_repository.dart @@ -0,0 +1,243 @@ +/// Owns every trip lifecycle transition. +/// +/// Ported from `com.rippr.data.TripRepository`. +/// +/// Recording state lives in the database rather than in memory. A row in `trips` with +/// `endedAt IS NULL` *is* the fact of an in-progress ride, so it survives process death — +/// which an in-memory flag could not. On Android the OS can restart the process with no +/// intent, and on iOS the app can be suspended and resumed; in both cases a flag would +/// come back `false` while a ride was genuinely underway. +/// +/// Every transition is idempotent and safe to call from an unexpected state. The platform +/// can restart the recorder at any moment, so calling resume on an already-recording trip +/// must be a no-op rather than opening a duplicate segment. +library; + +import '../domain/models.dart'; +import '../stats/ride_statistics.dart'; +import 'database.dart'; + +/// The ids the recorder stamps onto each fix. +class TripHandle { + const TripHandle(this.tripId, this.segmentId); + + final int tripId; + final int segmentId; + + @override + bool operator ==(Object other) => + other is TripHandle && + other.tripId == tripId && + other.segmentId == segmentId; + + @override + int get hashCode => Object.hash(tripId, segmentId); + + @override + String toString() => 'TripHandle(trip: $tripId, segment: $segmentId)'; +} + +class TripRepository { + TripRepository(this._db); + + final AppDatabase _db; + + AppDatabase get db => _db; + + // --- Reads --------------------------------------------------------------- + + Stream watchActiveTrip() => _db.watchActiveTrip(); + + Stream watchTrip(int id) => _db.watchTrip(id); + + Stream> watchCompletedTrips() => _db.watchCompletedTrips(); + + Future activeTrip() => _db.getActiveTrip(); + + Future tripById(int id) => _db.getTrip(id); + + Future> pointsForTrip(int id) => _db.pointsForTrip(id); + + Future> segmentsForTrip(int id) => _db.segmentsForTrip(id); + + Stream watchTripStats(int id) => _db.watchTripStats(id); + + /// The segment currently being written into, if a ride is active and not paused. + Future openSegment() async { + final trip = await _db.getActiveTrip(); + if (trip == null) return null; + return _db.openSegment(trip.id); + } + + // --- Lifecycle ----------------------------------------------------------- + + /// Starts a ride, or adopts one already in progress. + /// + /// Adoption rather than rejection is deliberate: after a process kill the trip row + /// still exists, and the restarted recorder needs to continue it, not start a second. + Future startTrip(int now) => _db.transaction(() async { + final existing = await _db.getActiveTrip(); + if (existing != null) { + return _adoptOrOpenSegment(existing, now); + } + final tripId = await _db + .insertTrip(Trip(startedAt: now, state: TripState.recording)); + final segmentId = await _db.insertSegment(tripId, now); + return TripHandle(tripId, segmentId); + }); + + /// Closes the open segment and marks the trip paused. The trip itself stays open — + /// only [completeTrip] sets `endedAt`. + Future pauseTrip(int now) => _db.transaction(() async { + final trip = await _db.getActiveTrip(); + if (trip == null) return false; + if (trip.state == TripState.paused) return true; + + final open = await _db.openSegment(trip.id); + if (open != null) await _db.closeSegment(open.id, now); + await _db.setTripState(trip.id, TripState.paused); + return true; + }); + + /// Opens a fresh segment so the pause leaves a real gap in the recorded path. + Future resumeTrip(int now) => _db.transaction(() async { + final trip = await _db.getActiveTrip(); + if (trip == null) return null; + final handle = await _adoptOrOpenSegment(trip, now); + await _db.setTripState(trip.id, TripState.recording); + return handle; + }); + + Future completeTrip(int now) => _db.transaction(() async { + final trip = await _db.getActiveTrip(); + if (trip == null) return null; + final open = await _db.openSegment(trip.id); + if (open != null) await _db.closeSegment(open.id, now); + await _db.closeTrip(trip.id, now, state: TripState.completed); + return trip.id; + }); + + /// Deletes the active trip outright. Segments and points follow via CASCADE. + Future discardTrip() => _db.transaction(() async { + final trip = await _db.getActiveTrip(); + if (trip == null) return false; + await _db.deleteTrip(trip.id); + return true; + }); + + // --- Management ---------------------------------------------------------- + + Future renameTrip(int id, String? name) { + // Empty input must collapse to null, or the UI's "derive a label from the date" + // branch and a stored "" would diverge. + final trimmed = name?.trim(); + return _db.renameTrip(id, (trimmed == null || trimmed.isEmpty) ? null : trimmed); + } + + Future deleteTrip(int id) => _db.deleteTrip(id); + + /// Combines two completed rides into one. + /// + /// The earlier trip survives, the later one's segments and points are re-parented onto + /// it, and the later row is deleted. Segments are **never joined** — the boundary + /// between the two rides becomes a segment boundary exactly like a pause, which is + /// correct: the rider genuinely was not recording in between, and joining them would + /// draw a straight line across the gap. + /// + /// Aggregates are recomputed from scratch rather than summed, because distance is not + /// additive across that gap. + /// + /// Returns the surviving trip id, or null if the merge was rejected. + Future mergeTrips(int a, int b) => _db.transaction(() async { + if (a == b) return null; + final first = await _db.getTrip(a); + if (first == null) return null; + final second = await _db.getTrip(b); + if (second == null) return null; + if (first.isActive || second.isActive) { + // Refusing to merge an active trip. + return null; + } + + // Selection order is not ride order. + final (survivor, absorbed) = first.startedAt <= second.startedAt + ? (first, second) + : (second, first); + + await _db.reparentSegments(absorbed.id, survivor.id); + await _db.reparentPoints(absorbed.id, survivor.id); + await _db.deleteTrip(absorbed.id); + + final endedAt = (survivor.endedAt ?? 0) > (absorbed.endedAt ?? 0) + ? (survivor.endedAt ?? 0) + : (absorbed.endedAt ?? 0); + await _db.closeTrip(survivor.id, endedAt, state: TripState.completed); + if (survivor.name == null && absorbed.name != null) { + await _db.renameTrip(survivor.id, absorbed.name); + } + + await recomputeAggregates(survivor.id); + return survivor.id; + }); + + /// Recomputes a trip's stored totals from the points it actually owns. + /// + /// This is the authoritative pass. The recorder's live accumulation is an estimate, so + /// running this on completion means a mid-ride process kill cannot leave permanently + /// skewed totals. + Future recomputeAggregates(int tripId) async { + final summary = computeSummary( + await _db.pointsForTrip(tripId), + segments: await _db.segmentsForTrip(tripId), + ); + await _db.updateAggregates( + id: tripId, + distanceM: summary.distanceM, + movingMillis: summary.movingMillis, + maxSpeedKmh: summary.maxSpeedKmh, + elevationGainM: summary.elevationGainM, + pointCount: summary.pointCount, + ); + } + + // --- Writes from the recorder -------------------------------------------- + + /// Appends a batch of fixes. + /// + /// Points arrive already stamped with their trip and segment ids — see the recorder. + /// They are **never** looked up at write time, so a fix still in flight when a pause + /// happens lands in the segment it actually belongs to. + Future appendPoints(List points) async { + if (points.isEmpty) return; + await _db.insertPoints(points); + } + + Future lastPointInSegment(int segmentId) => + _db.lastInSegment(segmentId); + + Future persistAggregates({ + required int tripId, + required double distanceM, + required int movingMillis, + required double maxSpeedKmh, + required double elevationGainM, + required int pointCount, + }) => + _db.updateAggregates( + id: tripId, + distanceM: distanceM, + movingMillis: movingMillis, + maxSpeedKmh: maxSpeedKmh, + elevationGainM: elevationGainM, + pointCount: pointCount, + ); + + // --- Internals ----------------------------------------------------------- + + Future _adoptOrOpenSegment(Trip trip, int now) async { + final existing = await _db.openSegment(trip.id); + if (existing != null) return TripHandle(trip.id, existing.id); + final segmentId = await _db.insertSegment(trip.id, now); + return TripHandle(trip.id, segmentId); + } +} diff --git a/test/trip_repository_test.dart b/test/trip_repository_test.dart new file mode 100644 index 0000000..727c69f --- /dev/null +++ b/test/trip_repository_test.dart @@ -0,0 +1,352 @@ +import 'package:drift/drift.dart' show driftRuntimeOptions; +import 'package:drift/native.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:rippr/src/data/database.dart'; +import 'package:rippr/src/data/trip_repository.dart'; +import 'package:rippr/src/domain/models.dart'; + +/// Ported from `com.rippr.data.TripRepositoryTest` and `MergeTest`. +/// +/// Both were **instrumented** suites needing a device and a booted emulator. Against +/// Drift on the Dart VM they are ordinary unit tests. +/// +/// In-memory databases only — the native suite once wiped a real device's rides by +/// running against the production singleton. +void main() { + late AppDatabase db; + late TripRepository repo; + + setUp(() { + driftRuntimeOptions.dontWarnAboutMultipleDatabases = true; + db = AppDatabase(NativeDatabase.memory()); + repo = TripRepository(db); + }); + + tearDown(() async => db.close()); + + Future addPoint(TripHandle handle, int ts, + {double lat = 51.0, double speed = 40.0, double alt = 1000.0}) => + repo.appendPoints([ + TrackPoint( + tripId: handle.tripId, + segmentId: handle.segmentId, + timestamp: ts, + latitude: lat, + longitude: -114.0, + speedKmh: speed, + altitudeM: alt, + ), + ]); + + group('lifecycle', () { + test('full lifecycle produces one trip and two segments', () async { + final h1 = await repo.startTrip(1000); + await addPoint(h1, 1000); + await addPoint(h1, 2000); + + await repo.pauseTrip(3000); + final h2 = (await repo.resumeTrip(4000))!; + await addPoint(h2, 5000); + + final tripId = await repo.completeTrip(6000); + + expect(tripId, h1.tripId); + expect(h2.segmentId, isNot(h1.segmentId), + reason: 'resume must open a fresh segment'); + + final segments = await repo.segmentsForTrip(h1.tripId); + expect(segments.length, 2); + expect(segments.every((s) => !s.isOpen), isTrue, + reason: 'completing must close the open segment'); + expect((await repo.tripById(h1.tripId))!.state, TripState.completed); + expect(await repo.activeTrip(), isNull); + }); + + test('pause closes the segment but leaves the trip open', () async { + final h = await repo.startTrip(1000); + await repo.pauseTrip(2000); + + final trip = (await repo.activeTrip())!; + expect(trip.state, TripState.paused); + expect(trip.endedAt, isNull, reason: 'pause must not end the trip'); + expect(await db.openSegment(h.tripId), isNull); + }); + + test('startTrip adopts an already active trip instead of creating a second', + () async { + final first = await repo.startTrip(1000); + final second = await repo.startTrip(2000); + + expect(second.tripId, first.tripId); + expect(second.segmentId, first.segmentId); + expect(await db.countTrips(), 1); + }); + + test('resume while already recording is a no-op', () async { + final h = await repo.startTrip(1000); + final resumed = await repo.resumeTrip(2000); + + expect(resumed!.segmentId, h.segmentId, + reason: 'must not open a duplicate segment'); + expect(await db.countSegmentsForTrip(h.tripId), 1); + }); + + test('double pause is a no-op', () async { + final h = await repo.startTrip(1000); + expect(await repo.pauseTrip(2000), isTrue); + expect(await repo.pauseTrip(3000), isTrue); + expect(await db.countSegmentsForTrip(h.tripId), 1); + }); + + test('transitions with no active trip are safe no-ops', () async { + expect(await repo.pauseTrip(1000), isFalse); + expect(await repo.resumeTrip(1000), isNull); + expect(await repo.completeTrip(1000), isNull); + expect(await repo.discardTrip(), isFalse); + }); + + test('start after complete begins a fresh trip', () async { + final first = await repo.startTrip(1000); + await repo.completeTrip(2000); + final second = await repo.startTrip(3000); + + expect(second.tripId, isNot(first.tripId)); + expect(await db.countTrips(), 2); + }); + }); + + group('discard', () { + test('discard removes the trip and all its points', () async { + final h = await repo.startTrip(1000); + await addPoint(h, 1000); + await addPoint(h, 2000); + + expect(await repo.discardTrip(), isTrue); + + expect(await repo.tripById(h.tripId), isNull); + expect(await db.countPointsForTrip(h.tripId), 0); + expect(await db.countSegmentsForTrip(h.tripId), 0); + }); + + test('discard leaves earlier completed trips alone', () async { + final keep = await repo.startTrip(1000); + await addPoint(keep, 1000); + await repo.completeTrip(2000); + + final throwaway = await repo.startTrip(3000); + await addPoint(throwaway, 3000); + await repo.discardTrip(); + + expect(await repo.tripById(keep.tripId), isNotNull); + expect(await db.countPointsForTrip(keep.tripId), 1); + expect(await db.countTrips(), 1); + }); + }); + + group('streams', () { + test('active trip stream tracks the lifecycle', () async { + expect(await repo.watchActiveTrip().first, isNull); + + final h = await repo.startTrip(1000); + expect((await repo.watchActiveTrip().first)?.id, h.tripId); + + await repo.completeTrip(2000); + expect(await repo.watchActiveTrip().first, isNull); + }); + + test('completed trips stream excludes the active trip', () async { + final done = await repo.startTrip(1000); + await repo.completeTrip(2000); + await repo.startTrip(3000); + + final completed = await repo.watchCompletedTrips().first; + expect(completed.length, 1); + expect(completed.single.id, done.tripId); + }); + }); + + group('rename', () { + test('rename stores null rather than an empty string', () async { + final h = await repo.startTrip(1000); + + await repo.renameTrip(h.tripId, 'Morning loop'); + expect((await repo.tripById(h.tripId))!.name, 'Morning loop'); + + await repo.renameTrip(h.tripId, ''); + expect((await repo.tripById(h.tripId))!.name, isNull, + reason: 'an empty string would diverge from the date-label branch'); + + await repo.renameTrip(h.tripId, ' '); + expect((await repo.tripById(h.tripId))!.name, isNull); + }); + + test('rename trims surrounding whitespace', () async { + final h = await repo.startTrip(1000); + await repo.renameTrip(h.tripId, ' Sunday blast '); + expect((await repo.tripById(h.tripId))!.name, 'Sunday blast'); + }); + }); + + group('process death', () { + test('a fresh repository over the same database sees the active trip', + () async { + final h = await repo.startTrip(1000); + await addPoint(h, 1000); + + // Simulates the process being killed and rebuilt: new repository, same file. + final revived = TripRepository(db); + + final active = await revived.activeTrip(); + expect(active, isNotNull, + reason: 'recording state must survive process death'); + expect(active!.id, h.tripId); + }); + + test('restart after pause resumes into a new segment', () async { + final h = await repo.startTrip(1000); + await repo.pauseTrip(2000); + + final revived = TripRepository(db); + final resumed = (await revived.resumeTrip(3000))!; + + expect(resumed.tripId, h.tripId); + expect(resumed.segmentId, isNot(h.segmentId)); + expect((await revived.activeTrip())!.state, TripState.recording); + }); + }); + + // --------------------------------------------------------------------------- + // Merge — ported from MergeTest + // --------------------------------------------------------------------------- + + group('merge', () { + Future completedTrip({ + required int startedAt, + required int endedAt, + int points = 3, + String? name, + double lat = 51.0, + }) async { + final h = await repo.startTrip(startedAt); + for (var i = 0; i < points; i++) { + await addPoint(h, startedAt + i * 1000, lat: lat + i * 0.0001); + } + if (name != null) await repo.renameTrip(h.tripId, name); + await repo.completeTrip(endedAt); + return h.tripId; + } + + test('merge reparents everything and deletes the absorbed row', () async { + final a = await completedTrip(startedAt: 1000, endedAt: 5000); + final b = await completedTrip(startedAt: 10000, endedAt: 15000); + + final survivor = await repo.mergeTrips(a, b); + + expect(survivor, a); + expect(await repo.tripById(b), isNull); + expect(await db.countPointsForTrip(a), 6); + expect(await db.countSegmentsForTrip(a), 2); + expect(await db.countTrips(), 1); + }); + + test('selection order does not decide the survivor', () async { + final earlier = await completedTrip(startedAt: 1000, endedAt: 5000); + final later = await completedTrip(startedAt: 10000, endedAt: 15000); + + // Passed later-first on purpose. + expect(await repo.mergeTrips(later, earlier), earlier); + }); + + test('merged aggregates are recomputed, not summed', () async { + // The two rides are a degree of latitude apart — ~111 km. If aggregates were + // summed, or if the segments were joined, that gap would appear as distance. + final a = await completedTrip(startedAt: 1000, endedAt: 5000, lat: 51.0); + final b = + await completedTrip(startedAt: 10000, endedAt: 15000, lat: 52.0); + + await repo.mergeTrips(a, b); + + final merged = (await repo.tripById(a))!; + expect(merged.pointCount, 6); + expect(merged.distanceM, lessThan(200.0), + reason: + 'the ~111 km gap leaked into distance: ${merged.distanceM} m'); + }); + + test('the join remains a segment boundary', () async { + final a = await completedTrip(startedAt: 1000, endedAt: 5000); + final b = await completedTrip(startedAt: 10000, endedAt: 15000); + + await repo.mergeTrips(a, b); + + final segments = await repo.segmentsForTrip(a); + expect(segments.length, 2, + reason: 'segments must never be joined by a merge'); + }); + + test('endedAt becomes the later of the two', () async { + final a = await completedTrip(startedAt: 1000, endedAt: 5000); + final b = await completedTrip(startedAt: 10000, endedAt: 15000); + + await repo.mergeTrips(a, b); + + expect((await repo.tripById(a))!.endedAt, 15000); + }); + + test('an unnamed survivor inherits the other name', () async { + final a = await completedTrip(startedAt: 1000, endedAt: 5000); + final b = + await completedTrip(startedAt: 10000, endedAt: 15000, name: 'Part two'); + + await repo.mergeTrips(a, b); + + expect((await repo.tripById(a))!.name, 'Part two'); + }); + + test('an existing survivor name is kept', () async { + final a = await completedTrip( + startedAt: 1000, endedAt: 5000, name: 'The good one'); + final b = + await completedTrip(startedAt: 10000, endedAt: 15000, name: 'Part two'); + + await repo.mergeTrips(a, b); + + expect((await repo.tripById(a))!.name, 'The good one'); + }); + + test('merging a trip with itself is rejected', () async { + final a = await completedTrip(startedAt: 1000, endedAt: 5000); + expect(await repo.mergeTrips(a, a), isNull); + expect(await db.countPointsForTrip(a), 3); + }); + + test('merging an active trip is rejected', () async { + final done = await completedTrip(startedAt: 1000, endedAt: 5000); + final active = await repo.startTrip(10000); + + expect(await repo.mergeTrips(done, active.tripId), isNull); + expect(await db.countTrips(), 2, reason: 'nothing may be deleted'); + }); + + test('merging a missing trip is rejected', () async { + final a = await completedTrip(startedAt: 1000, endedAt: 5000); + expect(await repo.mergeTrips(a, 9999), isNull); + expect(await repo.tripById(a), isNotNull); + }); + + test('merge is atomic and leaves no orphans', () async { + final a = await completedTrip(startedAt: 1000, endedAt: 5000); + final b = await completedTrip(startedAt: 10000, endedAt: 15000); + + await repo.mergeTrips(a, b); + + // Every surviving point and segment must belong to the survivor. + final allPoints = await db.allPoints(); + expect(allPoints.every((p) => p.tripId == a), isTrue); + expect(allPoints.length, 6); + + final segments = await repo.segmentsForTrip(a); + expect(segments.every((s) => s.tripId == a), isTrue); + }); + }); +}