diff --git a/docs/feedback/FB-07-route-planner-null-island.md b/docs/feedback/FB-07-route-planner-null-island.md new file mode 100644 index 0000000..9667d7b --- /dev/null +++ b/docs/feedback/FB-07-route-planner-null-island.md @@ -0,0 +1,202 @@ +# FB-07 — Route Planner opens on Null Island instead of the rider's real location + +**Depends on** FB-01 (reuses its ambient-location provider) · **Size** S/M · **Status** Done + +## Goal +A new route, or a route with fewer than two pins, must open the map on the rider's real +location. Today it opens on the middle of the ocean. + +## Context +Direct user feedback (`docs/FEEDBACK.md`): + +> Pin drops still do not work at all, the map when placing the pins doesn't render at a +> all, it's just a blank canvas that you can zoom in and out of but no detail appears. +> The pins can also be placed but nothing is rendered on the map so you have no clue +> where they are. + +`lib/src/ui/routes/route_planner_screen.dart`, `build()` (~line 165): + +```dart +child: FlutterMap( + mapController: _mapController, + options: MapOptions( + initialCameraFit: _initialFit(waypoints), + initialCenter: waypoints.isEmpty + ? const ll.LatLng(0, 0) + : ll.LatLng(waypoints.first.latitude, waypoints.first.longitude), + initialZoom: waypoints.length <= 1 ? ambientZoom : maxTileZoom - 3, + maxZoom: maxTileZoom, + onTap: (tapPosition, point) { + repo.addWaypoint(widget.routeId, point.latitude, point.longitude); + ... +``` + +`ll.LatLng(0, 0)` is Null Island — a single point in the Atlantic Ocean with no land +and no map detail at any zoom level. A brand-new route (0 pins), or a route with exactly +1 pin dropped near that point, opens the map centered there. At street-level zoom, an +area of open ocean legitimately renders as a flat, featureless expanse — this matches +the report exactly: "a blank canvas... no detail appears," and a pin dropped anywhere +near that same spot lands in the same empty area, with nothing else on screen to show +where it is relative to. + +`RoutePlannerScreen` manages its own `FlutterMap` directly — it does not use `RideMap` +and does not currently read `ambientPositionProvider` at all (confirmed by grep: no +reference to `ambientPositionProvider` anywhere in `route_planner_screen.dart`). FB-01 +already built exactly the provider this ticket needs — `lib/src/app/providers.dart` +(~line 66): + +```dart +final ambientPositionProvider = StreamProvider.autoDispose((ref) async* { + if (!ref.watch(mapEnabledProvider)) { + yield null; + return; + } + final source = ref.watch(locationSourceProvider); + try { + await source.start(); // idempotent; safe even if a recording already started it + } on LocationException { + yield null; // permission denied / service disabled — ambient mode is best-effort + return; + } + yield* source.fixes.map((fix) => fix); +}); +``` + +`LocationFix` is defined in `lib/src/recording/location_source.dart`. + +**`initialCenter`/`initialZoom` are read exactly once, at `FlutterMap` construction — +not on every rebuild.** The existing comment in this file already documents this +constraint for the waypoints stream (~line 158): the screen waits for +`waypointsAsync.hasValue` before building `FlutterMap` at all, specifically so the +first real camera position is correct from the start. The same constraint applies here: +reading `ambientPositionProvider` must happen before `FlutterMap` is constructed, not +patched in afterward with a controller move — mirror the existing wait-for-first-value +pattern, do not invent a new one. + +## Design +- Watch `ambientPositionProvider` in `build()`, the same way `ShellScaffold` already + does (`lib/src/ui/app_shell.dart` ~line 74): + ```dart + final ambientFix = ref.watch(ambientPositionProvider).valueOrNull; + final ambientPosition = ambientFix == null + ? null + : ll.LatLng(ambientFix.latitude, ambientFix.longitude); + ``` +- Use `ambientPosition` for `initialCenter` when there are fewer than 2 waypoints, + falling back to `(0, 0)` only when no ambient fix is available yet (permission denied, + service disabled, or the fix has not arrived yet): + ```dart + initialCenter: waypoints.isEmpty + ? (ambientPosition ?? const ll.LatLng(0, 0)) + : ll.LatLng(waypoints.first.latitude, waypoints.first.longitude), + ``` +- Do **not** block the map on waiting for the ambient fix the way `waypointsAsync` is + blocked on its own first value. A missing ambient fix must fall back to `(0, 0)` + immediately, not show a spinner — the rider must always eventually reach a usable map, + even with location permission denied. This matches the existing `RideMap` fallback + behavior in FB-01 exactly (see `ride_map.dart`'s `bounds == null` branch). +- Leave the 1-waypoint and 2-plus-waypoint camera logic unchanged — a route that already + has a real pin should still center on that pin, not on the ambient position, since the + pin is a stronger signal of where the rider actually wants to look. +- **Confirm tiles genuinely render once centered on a real location.** After this fix, + drop a pin near a real city on the Android emulator (use `adb emu geo fix` to set a + real location, as FB-01's own verification pass did). Take a screenshot. Confirm + street-level map detail actually appears, not just a differently-colored blank area. + If tiles still do not render even at a real location, that is a second, distinct bug — + investigate `TileLayer`'s setup in this file (`urlTemplate`, `tileProvider`, + `mapConnectivityProvider`'s `skeletonMode`) before assuming this ticket's fix is + sufficient, and document the real cause in the Outcome section. + +## Implementation +1. Add `final ambientFix = ref.watch(ambientPositionProvider).valueOrNull;` and the + `ambientPosition` conversion to `build()`. +2. Change the `initialCenter` fallback for the empty-waypoints case from `const + ll.LatLng(0, 0)` to `ambientPosition ?? const ll.LatLng(0, 0)`. +3. Run the app on the Android emulator. Set a real mock location. Open a new route. + Confirm the map opens on that location with visible street detail, not open ocean. + +## Acceptance criteria +- [ ] A brand-new route (0 pins) opens the map centered on the rider's real location, + when a location fix is available. +- [ ] A brand-new route still opens on `(0, 0)` when no location fix is available + (permission denied, service disabled, or no fix yet) — no crash, no infinite + spinner. +- [ ] A route with 1 or more pins still centers on the pin, unchanged from today. +- [ ] Dropping a pin near a real city on the emulator shows visible street-level map + detail underneath it, confirmed by a real screenshot. +- [ ] `flutter analyze` clean, `flutter test` green, test count only goes up. + +## Tests +- Widget test: pump `RoutePlannerScreen` for a route with 0 waypoints, with + `ambientPositionProvider` overridden to a fixed `LocationFix` — assert the + `FlutterMap`'s `options.initialCenter` matches that fix, not `(0, 0)`. +- Widget test: pump `RoutePlannerScreen` for a route with 0 waypoints, with + `ambientPositionProvider` overridden to emit `null` (no fix available) — assert + `initialCenter` falls back to `(0, 0)`, matching today's existing behavior. +- Widget test: pump `RoutePlannerScreen` for a route with 1 real waypoint — assert + `initialCenter` still matches that waypoint's own coordinates, not the ambient + position, even when `ambientPositionProvider` emits a different fix. + +## Risks +- If the on-device check in this ticket's Implementation step 3 finds tiles still do + not render at a real location, this ticket's fix alone is not sufficient — document + the real root cause found and either fix it in this same ticket or state plainly in + the Outcome section that a further ticket is needed. Do not claim this ticket is done + without confirming real map detail actually appears on a screenshot. + +## Out of scope +Any change to the Map tab's own map (FB-06 covers its remaining issues). Turn-by-turn +route following (V3-09, already deferred elsewhere). + +## Outcome +Implemented the Design section exactly, in `lib/src/ui/routes/route_planner_screen.dart`'s +`build()`: +- Added `final ambientFix = ref.watch(ambientPositionProvider).valueOrNull;` and the + `ambientPosition` conversion to `ll.LatLng?`, watched unconditionally alongside the + existing `routeAsync`/`waypointsAsync` watches (not gated behind the + `waypointsAsync.hasValue` wait, matching the ticket's explicit instruction not to block + the map on the ambient fix). +- Changed the empty-waypoints `initialCenter` fallback from `const ll.LatLng(0, 0)` to + `ambientPosition ?? const ll.LatLng(0, 0)`. The 1-waypoint and 2-plus-waypoint camera + logic is untouched. + +Added three widget tests to `test/route_planner_screen_test.dart` (in the +`RoutePlannerScreen` group), each overriding `ambientPositionProvider` directly with +`.overrideWith((ref) => Stream.value(...))` rather than routing through +`FakeLocationSource`, since the ticket only needs to check what `initialCenter` resolves +to, not the location-source plumbing FB-01's own tests already cover: +1. A brand-new route (0 waypoints) with a fixed ambient fix opens `FlutterMap` centered + on that fix. +2. A brand-new route with `ambientPositionProvider` emitting `null` still falls back to + `(0, 0)`, matching prior behavior. +3. A route with one real waypoint centers on that waypoint even when a different ambient + fix is available, confirming the pin still wins. + +`flutter analyze`: clean — the same 4 pre-existing `info`-level issues as baseline +(deprecated `copyWith` in `crash_reporter.dart`, `prefer_initializing_formals` in +`map_connectivity.dart`), none in the touched files. + +`flutter test`: all passing, **415 tests** (412 baseline + 3 new), no regressions. + +**On-device check: attempted, but the emulator became unresponsive partway through and +was abandoned per the conservative-use instruction — this step is effectively skipped.** +Booted `Medium_Phone_API_35`, it came up and reported `sys.boot_completed=1` quickly, +`adb devices` showed it connected. Installed and launched the app +(`com.rippr.port`), granted `ACCESS_FINE_LOCATION`/`ACCESS_COARSE_LOCATION`, and set +`adb emu geo fix -114.07 51.05`. The app launched into an in-progress recording state on +the Map tab (not the Route Planner) with a "System UI isn't responding" ANR dialog +already on screen. Two attempts to dismiss it via `adb shell input tap` each hung past +their timeout and were moved to the background — a textbook case of the "adb/emulator +becomes slow or unresponsive" condition called out in this ticket's dispatch, so per +instructions no further retries were made. The emulator was killed cleanly afterward +(`adb emu kill`) rather than left running. Because the Route Planner screen was never +actually reached, no independent tile-rendering finding was made this run either way — +neither confirming nor ruling out the "second, deeper bug" flagged as a risk. The +screenshot taken before giving up showed the (unrelated, out-of-scope) Map tab rendering +a flat, low-detail world map under the ANR dialog, consistent with FB-06's separate, +already-tracked scope, not this ticket's Route Planner fix. + +The code change and its three widget tests are the verified deliverable for this ticket; +the on-device screenshot check called for in Implementation step 3 / Acceptance criteria +was not completed and should be picked up in a follow-up pass when the emulator is +stable, rather than by fighting it further here. diff --git a/lib/src/ui/routes/route_planner_screen.dart b/lib/src/ui/routes/route_planner_screen.dart index 00e9328..9714415 100644 --- a/lib/src/ui/routes/route_planner_screen.dart +++ b/lib/src/ui/routes/route_planner_screen.dart @@ -119,6 +119,16 @@ class _RoutePlannerScreenState extends ConsumerState final units = ref.watch(unitSystemProvider); final repo = ref.read(routePlanRepositoryProvider); + // FB-07: a brand-new route (or one with <2 pins) should open on the rider's real + // location rather than Null Island (0, 0). This mirrors ShellScaffold's own use of + // ambientPositionProvider (lib/src/ui/app_shell.dart). A missing fix (permission + // denied, service disabled, or not arrived yet) falls back to (0, 0) immediately -- + // it must not block the map the way waypointsAsync is blocked above. + final ambientFix = ref.watch(ambientPositionProvider).valueOrNull; + final ambientPosition = ambientFix == null + ? null + : ll.LatLng(ambientFix.latitude, ambientFix.longitude); + // Mirrors waypointsAsync's own guard below: `.valueOrNull` alone can't tell "still // loading" apart from "genuinely doesn't exist" -- both collapse to null. On a // brand-new route (fresh navigation right after `repo.createRoutePlan`), this @@ -168,7 +178,7 @@ class _RoutePlannerScreenState extends ConsumerState options: MapOptions( initialCameraFit: _initialFit(waypoints), initialCenter: waypoints.isEmpty - ? const ll.LatLng(0, 0) + ? (ambientPosition ?? const ll.LatLng(0, 0)) : ll.LatLng(waypoints.first.latitude, waypoints.first.longitude), initialZoom: waypoints.length <= 1 ? ambientZoom : maxTileZoom - 3, maxZoom: maxTileZoom, diff --git a/test/route_planner_screen_test.dart b/test/route_planner_screen_test.dart index e0fecc9..ce81b0a 100644 --- a/test/route_planner_screen_test.dart +++ b/test/route_planner_screen_test.dart @@ -7,6 +7,7 @@ import 'package:flutter_test/flutter_test.dart'; import 'package:rippr/src/app/providers.dart'; import 'package:rippr/src/data/database.dart'; import 'package:rippr/src/data/route_plan_repository.dart'; +import 'package:rippr/src/recording/location_source.dart' show LocationFix; import 'package:rippr/src/ui/app_shell.dart'; import 'package:rippr/src/ui/components/floating_pill.dart'; import 'package:rippr/src/ui/components/ride_map.dart' show ambientZoom; @@ -30,8 +31,8 @@ void main() { tearDown(() async => db.close()); - Widget host(Widget child) => ProviderScope( - overrides: [databaseProvider.overrideWithValue(db)], + Widget host(Widget child, {List overrides = const []}) => ProviderScope( + overrides: [databaseProvider.overrideWithValue(db), ...overrides], child: MaterialApp(theme: ripprTheme(), home: child), ); @@ -227,6 +228,79 @@ void main() { expect(map.options.initialZoom, ambientZoom); }); + screenTest('a brand-new route opens centered on the rider\'s real location, ' + 'not Null Island (FB-07)', (tester) async { + const fix = LocationFix( + timestamp: 1000, + latitude: 51.05, + longitude: -114.07, + speedMps: 0, + altitudeM: 0, + accuracyM: 5, + bearingDeg: 0, + ); + final id = await repo.createRoutePlan(1000); + await pumpMap( + tester, + host( + RoutePlannerScreen(routeId: id), + overrides: [ + ambientPositionProvider.overrideWith((ref) => Stream.value(fix)), + ], + ), + ); + + final map = tester.widget(find.byType(FlutterMap)); + expect(map.options.initialCenter.latitude, closeTo(51.05, 1e-9)); + expect(map.options.initialCenter.longitude, closeTo(-114.07, 1e-9)); + }); + + screenTest('a brand-new route falls back to (0, 0) when no ambient fix is ' + 'available (FB-07)', (tester) async { + final id = await repo.createRoutePlan(1000); + await pumpMap( + tester, + host( + RoutePlannerScreen(routeId: id), + overrides: [ + ambientPositionProvider.overrideWith((ref) => Stream.value(null)), + ], + ), + ); + + final map = tester.widget(find.byType(FlutterMap)); + expect(map.options.initialCenter.latitude, 0); + expect(map.options.initialCenter.longitude, 0); + }); + + screenTest('a route with a real waypoint still centers on the pin, not the ' + 'ambient position (FB-07)', (tester) async { + const fix = LocationFix( + timestamp: 1000, + latitude: 51.05, + longitude: -114.07, + speedMps: 0, + altitudeM: 0, + accuracyM: 5, + bearingDeg: 0, + ); + final id = await repo.createRoutePlan(1000); + await repo.addWaypoint(id, 51.0, -114.0); + await pumpMap( + tester, + host( + RoutePlannerScreen(routeId: id), + overrides: [ + ambientPositionProvider.overrideWith((ref) => Stream.value(fix)), + ], + ), + ); + + final map = tester.widget(find.byType(FlutterMap)); + expect(map.options.initialCenter.latitude, closeTo(51.0, 1e-9)); + expect(map.options.initialCenter.longitude, closeTo(-114.0, 1e-9)); + }); + screenTest('the offline-tiles download menu item is disabled with no pins ' '(V3-11, UI-06: now a PopupMenuItem behind the overflow menu)', (tester) async {