rippr/docs/feedback/FB-05-closed-loop-routes.md

# FB-05 — Closed-loop routes in Route Planner

**Depends on** FB-04 (same file, heavily edited — sequence after it merges) · **Size** M · **Status** Done

## Goal
Route planning currently only supports a one-way A→B→C path. Add the ability to close
the route into a loop that returns to its starting pin, updating the drawn polyline and
the distance stat to include the closing segment.

## Context
Direct user feedback (`docs/FEEDBACK.md`, "Route" section):

> ...there is no way to create a loop with the pins, it only creates a unidirectional
> path.

### Data model — no loop concept exists today

`lib/src/domain/models.dart`, `RoutePlan` (~lines 229-276):

```dart
class RoutePlan {
  const RoutePlan({
    this.id = 0,
    required this.name,
    required this.createdAt,
    this.activity = Activity.motorcycle,
    this.distanceM = 0.0,
    this.estimatedMillis,
    this.geometry,
  });

  final int id;
  final String name;
  final int createdAt;
  final Activity activity;
  final double distanceM;
  final int? estimatedMillis;
  final String? geometry;

  RoutePlan copyWith({
    int? id, String? name, int? createdAt, Activity? activity,
    double? distanceM, int? estimatedMillis, String? geometry,
  }) => RoutePlan(
    id: id ?? this.id, name: name ?? this.name, createdAt: createdAt ?? this.createdAt,
    activity: activity ?? this.activity, distanceM: distanceM ?? this.distanceM,
    estimatedMillis: estimatedMillis ?? this.estimatedMillis, geometry: geometry ?? this.geometry,
  );
}
```

`Waypoint` (~lines 278-309) has an `ordinal` for ordering, and waypoints are only ever
appended at the end (`RoutePlanRepository.addWaypoint`, below) — there is no "return to
start" field or concept anywhere in the model.

### Polyline and distance — strictly open, no wraparound

`lib/src/ui/routes/route_planner_screen.dart`, the `PolylineLayer` (~lines 182-201):

```dart
if (waypoints.length >= 2)
  PolylineLayer(
    polylines: [
      Polyline(
        points: [
          for (final w in waypoints) ll.LatLng(w.latitude, w.longitude),
        ],
        strokeWidth: 4,
        pattern: StrokePattern.dashed(segments: const [8, 6]),
        color: colors.primary,
      ),
    ],
  ),
```

`lib/src/data/route_plan_repository.dart`, distance recomputation (full relevant
section):

```dart
Future<void> addWaypoint(int routeId, double latitude, double longitude) async {
  final existing = await _db.waypointsForRoute(routeId);
  await _db.insertWaypoint(
    Waypoint(routeId: routeId, ordinal: existing.length, latitude: latitude, longitude: longitude),
  );
  await _recomputeDistance(routeId);
}

Future<void> _recomputeDistance(int routeId) async {
  final waypoints = await _db.waypointsForRoute(routeId);
  final distance = geo.pathLengthMeters([
    for (final w in waypoints) geo.LatLon(w.latitude, w.longitude),
  ]);
  await _db.setRoutePlanDistance(routeId, distance);
}
```

`geo.pathLengthMeters` (`lib/src/geo/geo.dart` ~line 185) sums consecutive-pair
distances generically over whatever point list it's given — it needs no changes itself,
just a point list that includes the closing segment when appropriate.

`FloatingPill`'s Distance/Est. Time/Pins stats read straight off `route.distanceM`/
`route.estimatedMillis`/`waypoints.length` (`route_planner_screen.dart` ~lines 280-292)
— Distance updates for free once `_recomputeDistance` accounts for the closing segment;
Pins and Est. Time are unaffected by this ticket.

### Schema — current version and migration pattern to follow

`lib/src/data/database.dart`:

```dart
@DataClassName('RoutePlanRow')
class RoutePlans extends Table {
  @override
  String get tableName => 'route_plans';

  IntColumn get id => integer().autoIncrement()();
  TextColumn get name => text()();
  IntColumn get createdAt => integer()();
  TextColumn get activity => textEnum<domain.Activity>().withDefault(const Constant('motorcycle'))();
  RealColumn get distanceM => real().withDefault(const Constant(0))();
  IntColumn get estimatedMillis => integer().nullable()();
  TextColumn get geometry => text().nullable()();
}

@DriftDatabase(tables: [Trips, Segments, TrackPoints, RoutePlans, Waypoints])
class AppDatabase extends _$AppDatabase {
  @override
  int get schemaVersion => 3;

  @override
  MigrationStrategy get migration => MigrationStrategy(
    onCreate: (m) => m.createAll(),
    onUpgrade: (m, from, to) async {
      if (from < 2) {
        await m.addColumn(trips, trips.activity);
      }
      if (from < 3) {
        await m.createTable(routePlans);
        await m.createTable(waypoints);
      }
    },
    ...
```

The existing `if (from < 2) { await m.addColumn(trips, trips.activity); }` is the exact
pattern to follow for a new nullable/defaulted column.

`RoutePlanRepository` insert/conversion glue, in `database.dart`:

```dart
Future<int> insertRoutePlan(domain.RoutePlan route) => into(routePlans).insert(
  RoutePlansCompanion.insert(
    name: route.name, createdAt: route.createdAt, activity: Value(route.activity),
    distanceM: Value(route.distanceM), estimatedMillis: Value(route.estimatedMillis),
    geometry: Value(route.geometry),
  ),
);

Future<void> setRoutePlanDistance(int id, double distanceM) => (update(routePlans)
  ..where((r) => r.id.equals(id))).write(RoutePlansCompanion(distanceM: Value(distanceM)));

domain.RoutePlan _toRoutePlan(RoutePlanRow r) => domain.RoutePlan(
  id: r.id, name: r.name, createdAt: r.createdAt, activity: r.activity,
  distanceM: r.distanceM, estimatedMillis: r.estimatedMillis, geometry: r.geometry,
);
```

### Overflow menu — where the toggle belongs

`route_planner_screen.dart`, `_OverflowMenu` (~lines 493-530-ish) already hosts
Rename/Download/Delete as a `PopupMenuButton` inside a `GlassPanel`:

```dart
class _OverflowMenu extends StatelessWidget {
  const _OverflowMenu({
    required this.hasWaypoints, required this.onRename, required this.onDownload, required this.onDelete,
  });

  final bool hasWaypoints;
  final VoidCallback onRename;
  final VoidCallback? onDownload;
  final VoidCallback onDelete;

  @override
  Widget build(BuildContext context) => GlassPanel(
    borderRadius: const BorderRadius.all(Radius.circular(999)),
    child: PopupMenuButton<String>(
      key: const Key('route-overflow-menu'),
      icon: const Icon(Icons.more_vert),
      onSelected: (value) => switch (value) {
        'rename' => onRename(),
        'download' => onDownload?.call(),
        'delete' => onDelete(),
        _ => null,
      },
      itemBuilder: (context) => [
        const PopupMenuItem(key: Key('rename-route'), value: 'rename', child: Text('Rename')),
        ...
```

And it's constructed at the call site (~lines 298-310):

```dart
_OverflowMenu(
  hasWaypoints: waypoints.isNotEmpty,
  onRename: () => _rename(context, repo, route),
  onDownload: waypoints.isEmpty ? null : () => _downloadOfflineTiles(context, waypoints),
  onDelete: () async {
    await repo.deleteRoutePlan(widget.routeId);
    widget.onBack?.call();
  },
),
```

## Design
- **Schema**: add `BoolColumn get isClosedLoop => boolean().withDefault(const
  Constant(false))();` to `RoutePlans` in `database.dart`. Bump `schemaVersion` from
  `3` to `4`; add `if (from < 4) { await m.addColumn(routePlans, routePlans.isClosedLoop); }`
  to `onUpgrade`.
- **Domain model**: add `final bool isClosedLoop;` to `RoutePlan` (default `false` in
  the constructor), thread it through `copyWith`.
- **DB glue**: add `isClosedLoop: Value(route.isClosedLoop)` to `insertRoutePlan`'s
  `RoutePlansCompanion.insert(...)` call, `isClosedLoop: r.isClosedLoop` to
  `_toRoutePlan`, and a new method mirroring `setRoutePlanDistance`:
  ```dart
  Future<void> setRoutePlanClosedLoop(int id, bool value) => (update(routePlans)
    ..where((r) => r.id.equals(id))).write(RoutePlansCompanion(isClosedLoop: Value(value)));
  ```
- **Repository**: add to `RoutePlanRepository`:
  ```dart
  Future<void> setClosedLoop(int routeId, bool value) async {
    await _db.setRoutePlanClosedLoop(routeId, value);
    await _recomputeDistance(routeId); // the closing segment changes the total
  }
  ```
  Update `_recomputeDistance` to fetch the route's own flag and append the first
  waypoint's coordinates when closed, before summing:
  ```dart
  Future<void> _recomputeDistance(int routeId) async {
    final route = await _db.getRoutePlan(routeId);
    final waypoints = await _db.waypointsForRoute(routeId);
    final points = [for (final w in waypoints) geo.LatLon(w.latitude, w.longitude)];
    if (route?.isClosedLoop == true && points.length >= 2) {
      points.add(points.first);
    }
    final distance = geo.pathLengthMeters(points);
    await _db.setRoutePlanDistance(routeId, distance);
  }
  ```
  (`_recomputeDistance` is already called from every waypoint-mutating method —
  `addWaypoint`, `moveWaypoint`, `deleteWaypoint`, `reorderWaypoint` — so the closing
  segment is kept correct automatically as pins are edited, with no other call site
  changes needed.)
- **UI toggle**: add a 4th item to `_OverflowMenu`, enabled only when there are enough
  waypoints for a loop to mean anything (`hasWaypoints` alone isn't enough — need at
  least 2 to form any segment at all; reuse the existing `hasWaypoints` bool but also
  thread through whether there are `>= 2` waypoints, or simplify by passing a new
  `canCloseLoop: waypoints.length >= 2` param):
  ```dart
  class _OverflowMenu extends StatelessWidget {
    const _OverflowMenu({
      required this.hasWaypoints,
      required this.canCloseLoop,
      required this.isClosedLoop,
      required this.onRename,
      required this.onDownload,
      required this.onToggleLoop,
      required this.onDelete,
    });
    ...
    final bool canCloseLoop;
    final bool isClosedLoop;
    final VoidCallback onToggleLoop;

    @override
    Widget build(BuildContext context) => GlassPanel(
      ...
      child: PopupMenuButton<String>(
        ...
        onSelected: (value) => switch (value) {
          'rename' => onRename(),
          'download' => onDownload?.call(),
          'closeLoop' => onToggleLoop(),
          'delete' => onDelete(),
          _ => null,
        },
        itemBuilder: (context) => [
          const PopupMenuItem(key: Key('rename-route'), value: 'rename', child: Text('Rename')),
          ...
          PopupMenuItem(
            key: const Key('toggle-closed-loop'),
            value: 'closeLoop',
            enabled: canCloseLoop,
            child: Text(isClosedLoop ? 'Open the loop' : 'Close the loop'),
          ),
          ...
        ],
      ),
    );
  }
  ```
  Wire it at the call site:
  ```dart
  _OverflowMenu(
    hasWaypoints: waypoints.isNotEmpty,
    canCloseLoop: waypoints.length >= 2,
    isClosedLoop: route.isClosedLoop,
    onRename: () => _rename(context, repo, route),
    onDownload: waypoints.isEmpty ? null : () => _downloadOfflineTiles(context, waypoints),
    onToggleLoop: () => repo.setClosedLoop(widget.routeId, !route.isClosedLoop),
    onDelete: () async { ... },
  ),
  ```
- **Polyline**: append the closing point when the route is closed and there are at
  least 2 waypoints:
  ```dart
  if (waypoints.length >= 2)
    PolylineLayer(
      polylines: [
        Polyline(
          points: [
            for (final w in waypoints) ll.LatLng(w.latitude, w.longitude),
            if (route.isClosedLoop)
              ll.LatLng(waypoints.first.latitude, waypoints.first.longitude),
          ],
          strokeWidth: 4,
          pattern: StrokePattern.dashed(segments: const [8, 6]),
          color: colors.primary,
        ),
      ],
    ),
  ```
  Same `Polyline` object, same styling — just one more point in the list, so the closing
  segment renders with identical dashed styling to the rest of the route.
- **`FloatingPill`/`_initialFit`**: no changes needed — Distance updates automatically
  via `_recomputeDistance`; Pins/Est. Time are correctly unaffected; `_initialFit`'s
  bounds-fitting is based on waypoint positions regardless of whether the last segment
  loops back (the closing point is always one of the existing waypoints' own
  coordinates, already inside the fitted bounds).

## Implementation
1. Add `isClosedLoop` column to `RoutePlans` in `database.dart`, bump `schemaVersion`
   to `4`, add the `onUpgrade` migration step.
2. Add `isClosedLoop` field to `RoutePlan` domain model + `copyWith`.
3. Add `isClosedLoop` to `insertRoutePlan`'s companion and `_toRoutePlan` in
   `database.dart`; add `setRoutePlanClosedLoop`.
4. Add `RoutePlanRepository.setClosedLoop`; update `_recomputeDistance` to append the
   closing point when `isClosedLoop`.
5. Extend `_OverflowMenu` with the new toggle item and params; wire it at the call
   site in `route_planner_screen.dart`.
6. Extend the `PolylineLayer`'s point list with the conditional closing point.

## Acceptance criteria
- [ ] "Close the loop" appears in the overflow menu, disabled when there are fewer than
      2 waypoints, enabled otherwise.
- [ ] Toggling it on redraws the polyline with a visible segment from the last waypoint
      back to the first, same dashed styling as the rest of the route.
- [ ] Toggling it on increases the Distance stat by the length of the new closing
      segment; toggling it back off returns Distance to the open-path total.
- [ ] The menu label reflects current state ("Close the loop" when open, "Open the
      loop" when already closed).
- [ ] Adding/moving/deleting a waypoint on a closed-loop route keeps the closing segment
      correct automatically (it's recomputed via the existing `_recomputeDistance` call
      already present in every waypoint-mutating method).
- [ ] Deleting waypoints down to fewer than 2 while closed doesn't crash — the
      `waypoints.length >= 2` guards in both the polyline and distance code make the
      closing point simply disappear along with the rest of the route drawing, same as
      today's behavior for an open path with 0-1 waypoints.
- [ ] A pre-existing (schema v3) database opens cleanly post-migration with
      `isClosedLoop` defaulting to `false` on every existing route.
- [ ] `flutter analyze` clean, `flutter test` green, test count only goes up.

## Tests
- **Migration test** (`test/migration_test.dart`, following the exact pattern of the
  existing `'a v2 database (V3-07) gains route_plans/waypoints and keeps its trips'`
  test): seed a v3 database (v1 seed + `activity` column + `route_plans`/`waypoints`
  tables created via raw SQL, `PRAGMA user_version = 3;`), open it with `AppDatabase`,
  insert a route via `db.insertRoutePlan`, and assert `isClosedLoop` reads back
  `false` by default, and that toggling it via `setRoutePlanClosedLoop` persists.
- **Repository tests** (`test/route_plan_repository_test.dart`, alongside the existing
  `'adding waypoints appends in order and updates distance live'` etc.):
  - `setClosedLoop(true)` on a route with 2+ waypoints increases `distanceM` by
    exactly the closing segment's length (compute the expected delta directly with
    `geo.pathLengthMeters`/a manual haversine call between the last and first
    waypoint, matching how other distance tests in this file already assert exact
    values).
  - `setClosedLoop(false)` after `true` returns `distanceM` to the pre-toggle value.
  - Adding a waypoint to an already-closed route keeps the distance correct
    (recomputed including the new closing segment against the new last-added point,
    not the old one).
- **Widget tests** (`test/route_planner_screen_test.dart`):
  - The "Close the loop" menu item is disabled with 0-1 waypoints, enabled with 2+
    (mirrors the existing "offline-tiles download menu item is disabled with no pins"
    test's structure — open the overflow menu, inspect the `PopupMenuItem.enabled`
    property via its key).
  - Tapping it toggles `route.isClosedLoop` (assert via
    `repo.routePlanById(id)?.isClosedLoop`, the same pattern the existing rename test
    uses to verify writes through to the repository) and the label switches to "Open
    the loop".
  - With the loop closed, `find.byType(Polyline)` (or inspecting the `PolylineLayer`'s
    `polylines` list directly, matching whatever existing pattern this test file uses
    to inspect polyline data) has one more point than the waypoint count.

## Risks
- None significant — this is additive (new column, new optional toggle) and every
  touched call site (`_recomputeDistance`) already runs on every mutation, so there's
  no new code path that could silently skip recomputation.

## Out of scope
Turn-by-turn following of a closed loop (V3-09, already deferred). Any UI indication of
loop direction/rotation. Road-snapped routing for the closing segment (V3-08) — it stays
a straight line like every other segment in this ticket's scope.

## Outcome
Implemented exactly as designed, with one small addition to the migration step beyond
what the ticket spelled out:

- Added `isClosedLoop` (`BoolColumn`, default `false`) to `RoutePlans` in
  `lib/src/data/database.dart`, bumped `schemaVersion` from 3 to 4, and regenerated
  `database.g.dart` via `dart run build_runner build`.
- **Deviation**: the migration guard needed to be `if (from >= 3 && from < 4)` rather
  than the ticket's plain `if (from < 4)`. A v2 → v4 upgrade runs the existing
  `if (from < 3)` branch first, which calls `m.createTable(routePlans)` — and since that
  table definition already includes `isClosedLoop` (Drift generates `CREATE TABLE` from
  the *current* Dart schema, not a historical snapshot), an unconditional `from < 4`
  then tried to `ADD COLUMN isClosedLoop` a second time and hit a duplicate-column
  SQLite error. Restricting the `addColumn` step to routes that already had the table
  before v4 (`from >= 3`) fixes it; a v1/v2 → v4+ upgrade gets the column for free from
  the fresh `createTable` instead. Added a v3-seed migration test
  (`test/migration_test.dart`, `'a v3 database (FB-05) gains isClosedLoop defaulting to
  false'`) covering exactly this path, plus verifying a v2 → v4+ jump still works via the
  existing v2 test.
- Added `isClosedLoop` to the `RoutePlan` domain model, `copyWith`, the
  `insertRoutePlan`/`_toRoutePlan` glue, and a new `setRoutePlanClosedLoop` method in
  `database.dart`.
- Added `RoutePlanRepository.setClosedLoop` and updated `_recomputeDistance` to append
  the first waypoint's coordinates as a closing point when the route is closed and has
  2+ waypoints, exactly per the design.
- Extended `_OverflowMenu` in `route_planner_screen.dart` with `canCloseLoop`/
  `isClosedLoop`/`onToggleLoop` and a "Close the loop" / "Open the loop" menu item,
  wired at the call site.
- Extended the `PolylineLayer`'s point list with the conditional closing point.
- FB-04 had already changed the surrounding `route` access to a guarded `AsyncValue`
  pattern (no more `.valueOrNull`) by the time this ticket landed; the new code reads
  `route.isClosedLoop` off the same already-resolved `route` value the rest of the
  build method uses, so no adjustment to that pattern was needed.

Added migration, repository, and widget tests per the ticket's Tests section (in
`test/migration_test.dart`, `test/route_plan_repository_test.dart`, and
`test/route_planner_screen_test.dart`).

`flutter analyze`: clean (only 4 pre-existing, unrelated `info`-level lints remain).
`flutter test`: all green, **411 tests passing** (up from the 404 baseline).