diff --git a/docs/feedback/FB-04-route-planner-map-render-fix.md b/docs/feedback/FB-04-route-planner-map-render-fix.md index 4f731ab..3a0a566 100644 --- a/docs/feedback/FB-04-route-planner-map-render-fix.md +++ b/docs/feedback/FB-04-route-planner-map-render-fix.md @@ -1,6 +1,6 @@ # FB-04 — Fix Route Planner's map failing to render + match street-level zoom -**Depends on** FB-01 (reuses its `ambientZoom` constant) · **Size** M · **Status** Not started +**Depends on** FB-01 (reuses its `ambientZoom` constant) · **Size** M · **Status** Done ## Goal The Route Planner screen's map doesn't render at all for the user. Find the real cause @@ -214,3 +214,61 @@ far past street level, exactly matching the "same zoomed in map view" ask. Closed-loop routes (FB-05, sequenced after this ticket since it also edits this file heavily). Turn-by-turn route following (V3-09, already explicitly deferred by UI-06). Any change to `RoutesListScreen` itself. + +## Outcome + +The ticket's high-confidence root-cause hypothesis was confirmed exactly as written, no +surprises in investigation. `FB-01`'s `ambientZoom` constant was already present in +`lib/src/ui/components/ride_map.dart` (line 54) when this ticket started, so no +fallback definition was needed. + +**Root cause**, confirmed by reproduction: `route` was read via +`ref.watch(routePlanProvider(widget.routeId)).valueOrNull`, which collapses "stream +still loading" and "route doesn't exist" into the same `null`. On a cold navigation to +a brand-new route (fresh `StreamProvider.autoDispose.family` instance, DB stream not +yet emitted), this made the screen render "This route no longer exists." with zero +`FlutterMap` in the tree for a route that was completely valid — exactly matching the +user's "doesn't work at all" report. `waypointsAsync` already had the correct +`hasValue` guard right next to it; `route` simply never got the same treatment. + +Reproduced first via a widget test (no real emulator available/reliable in this +environment): `repo.createRoutePlan(...)` followed by `pumpWidget` with **no** +additional pump — asserting immediately after the very first frame — reliably showed +the "no longer exists" text and zero `FlutterMap` widgets, confirming the bug. The same +test, run again after applying the fix (and separately, reverted before the fix to +confirm it fails on the old code), passed once `routeAsync.hasValue` guards the +transition, matching the ticket's `Design`/`Implementation` sections precisely: +- `route` was changed to hold the full `AsyncValue` (`routeAsync`), with a + new `if (!routeAsync.hasValue)` branch returning a spinner-only `Scaffold`, mirroring + `waypointsAsync`'s existing pattern two lines below. +- `route = routeAsync.value` is read only after that guard passes, and the existing + "no longer exists" branch (for a genuinely missing/deleted route) is otherwise + unchanged. +- `ambientZoom` was added to this file's `ride_map.dart` `show` clause, and + `initialZoom: waypoints.length <= 1 ? 14 : maxTileZoom - 3` became + `initialZoom: waypoints.length <= 1 ? ambientZoom : maxTileZoom - 3`. + +No investigation steps 3-5 (debugPrint state tracing, checking +`cachedTileProviderProvider`'s null-tileProvider fallback, diffing tile URL setup +against `RideMap`) were needed — the primary hypothesis fully explained the reported +symptom on the first reproduction attempt, and step 5's `tileUrlTemplate`/ +`tileSubdomains`/etc. are already imported from the same `ride_map.dart` export +`RideMap` uses, so no stale-URL-style regression was present. + +Two new regression tests were added to `test/route_planner_screen_test.dart`: +1. A brand-new route's screen is pumped with no settling: asserts the loading spinner + shows and neither the "no longer exists" text nor `FlutterMap` appear on that first + frame, then asserts the real map appears once both streams resolve. Verified this + test fails against the pre-fix code (assertion on the spinner/text) and passes after + the fix. +2. A route with zero waypoints is asserted to open at `MapOptions.initialZoom == + ambientZoom` (17.0), not the old hardcoded `14`. Verified this test fails against + the pre-fix code and passes after the fix. +The existing "a missing route says so instead of a blank map" test (using +`pumpAndSettle`) continues to pass unchanged, confirming the fix doesn't weaken the +genuinely-deleted/never-existed case. + +**Final state**: `flutter analyze` clean (4 pre-existing `info`-level issues in +unrelated files — `crash_reporter.dart`, `map_connectivity.dart` — present on `main` +before this ticket, untouched by this change). `flutter test`: **404 passing** (402 +baseline + 2 new regression tests), zero failures, zero regressions. diff --git a/lib/src/ui/routes/route_planner_screen.dart b/lib/src/ui/routes/route_planner_screen.dart index fe54d7d..f73f940 100644 --- a/lib/src/ui/routes/route_planner_screen.dart +++ b/lib/src/ui/routes/route_planner_screen.dart @@ -30,6 +30,7 @@ import '../components/glass_panel.dart'; import '../components/ride_map.dart' show TileAttribution, + ambientZoom, maxTileZoom, tileMaxNativeZoom, tileSubdomains, @@ -112,12 +113,27 @@ class _RoutePlannerScreenState extends ConsumerState @override Widget build(BuildContext context) { final colors = Theme.of(context).colorScheme; - final route = ref.watch(routePlanProvider(widget.routeId)).valueOrNull; + final routeAsync = ref.watch(routePlanProvider(widget.routeId)); final waypointsAsync = ref.watch(routeWaypointsProvider(widget.routeId)); final waypoints = waypointsAsync.valueOrNull ?? const []; final units = ref.watch(unitSystemProvider); final repo = ref.read(routePlanRepositoryProvider); + // 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 + // provider's DB stream hasn't delivered its first row yet, so without this guard + // the screen would render "This route no longer exists." for a route that is + // completely valid, with zero FlutterMap in the tree, until that first emission + // arrives. + if (!routeAsync.hasValue) { + return const Scaffold( + backgroundColor: Colors.transparent, + body: Center(child: CircularProgressIndicator()), + ); + } + final route = routeAsync.value; + if (route == null) { return Scaffold( backgroundColor: Colors.transparent, @@ -154,7 +170,7 @@ class _RoutePlannerScreenState extends ConsumerState initialCenter: waypoints.isEmpty ? const ll.LatLng(0, 0) : ll.LatLng(waypoints.first.latitude, waypoints.first.longitude), - initialZoom: waypoints.length <= 1 ? 14 : maxTileZoom - 3, + initialZoom: waypoints.length <= 1 ? ambientZoom : maxTileZoom - 3, maxZoom: maxTileZoom, onTap: (tapPosition, point) { repo.addWaypoint(widget.routeId, point.latitude, point.longitude); diff --git a/test/route_planner_screen_test.dart b/test/route_planner_screen_test.dart index 2fa6bec..f17cf21 100644 --- a/test/route_planner_screen_test.dart +++ b/test/route_planner_screen_test.dart @@ -9,6 +9,7 @@ import 'package:rippr/src/data/database.dart'; import 'package:rippr/src/data/route_plan_repository.dart'; 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; import 'package:rippr/src/ui/format.dart'; import 'package:rippr/src/ui/routes/route_planner_screen.dart'; import 'package:rippr/src/ui/routes/routes_list_screen.dart'; @@ -190,6 +191,42 @@ void main() { expect(find.textContaining('no longer exists'), findsOneWidget); }); + screenTest( + 'a brand-new route never flashes "no longer exists" while its stream is ' + 'still resolving, and shows the real map once it settles (FB-04)', + (tester) async { + // Mirrors the real "+ New route" flow: `repo.createRoutePlan` returns as soon as + // the row is written, but `routePlanProvider`'s DB stream hasn't delivered its + // first emission by the very next frame -- both `routePlanProvider` and + // `routeWaypointsProvider` start out `AsyncLoading` on this fresh navigation. + final id = await repo.createRoutePlan(1000, name: 'Fresh route'); + await tester.pumpWidget(host(RoutePlannerScreen(routeId: id))); + + // Immediately after the very first frame -- before the stream has had any chance + // to deliver -- the loading spinner must show, never the "no longer exists" text. + // (Pre-fix, `route` collapsed "still loading" and "doesn't exist" into the same + // null via `.valueOrNull`, so this assertion is exactly the FB-04 regression.) + expect(find.textContaining('no longer exists'), findsNothing); + expect(find.byType(CircularProgressIndicator), findsOneWidget); + expect(find.byType(FlutterMap), findsNothing); + + // Once both streams deliver their first value, the real map canvas appears. + for (var i = 0; i < 10; i++) { + await tester.pump(const Duration(milliseconds: 50)); + } + expect(find.textContaining('no longer exists'), findsNothing); + expect(find.byType(FlutterMap), findsOneWidget); + }); + + screenTest('a route with no waypoints opens at street-level zoom, not zoomed out ' + '(FB-04)', (tester) async { + final id = await repo.createRoutePlan(1000); + await pumpMap(tester, host(RoutePlannerScreen(routeId: id))); + + final map = tester.widget(find.byType(FlutterMap)); + expect(map.options.initialZoom, ambientZoom); + }); + 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 {