FB-04: fix Route Planner map failing to render on cold navigation
routePlanProvider's AsyncLoading state collapsed with "route doesn't exist" via .valueOrNull, so a brand-new route's screen briefly rendered "This route no longer exists." with no FlutterMap in the tree before the DB stream's first emission arrived. Add a hasValue guard mirroring waypointsAsync's existing pattern, and replace the route planner's hardcoded initialZoom of 14 with FB-01's ambientZoom constant for street-level parity with the rest of the app.
This commit is contained in:
@@ -1,6 +1,6 @@
|
|||||||
# FB-04 — Fix Route Planner's map failing to render + match street-level zoom
|
# 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
|
## Goal
|
||||||
The Route Planner screen's map doesn't render at all for the user. Find the real cause
|
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
|
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).
|
heavily). Turn-by-turn route following (V3-09, already explicitly deferred by UI-06).
|
||||||
Any change to `RoutesListScreen` itself.
|
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<RoutePlan?>` (`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.
|
||||||
|
|||||||
@@ -30,6 +30,7 @@ import '../components/glass_panel.dart';
|
|||||||
import '../components/ride_map.dart'
|
import '../components/ride_map.dart'
|
||||||
show
|
show
|
||||||
TileAttribution,
|
TileAttribution,
|
||||||
|
ambientZoom,
|
||||||
maxTileZoom,
|
maxTileZoom,
|
||||||
tileMaxNativeZoom,
|
tileMaxNativeZoom,
|
||||||
tileSubdomains,
|
tileSubdomains,
|
||||||
@@ -112,12 +113,27 @@ class _RoutePlannerScreenState extends ConsumerState<RoutePlannerScreen>
|
|||||||
@override
|
@override
|
||||||
Widget build(BuildContext context) {
|
Widget build(BuildContext context) {
|
||||||
final colors = Theme.of(context).colorScheme;
|
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 waypointsAsync = ref.watch(routeWaypointsProvider(widget.routeId));
|
||||||
final waypoints = waypointsAsync.valueOrNull ?? const [];
|
final waypoints = waypointsAsync.valueOrNull ?? const [];
|
||||||
final units = ref.watch(unitSystemProvider);
|
final units = ref.watch(unitSystemProvider);
|
||||||
final repo = ref.read(routePlanRepositoryProvider);
|
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) {
|
if (route == null) {
|
||||||
return Scaffold(
|
return Scaffold(
|
||||||
backgroundColor: Colors.transparent,
|
backgroundColor: Colors.transparent,
|
||||||
@@ -154,7 +170,7 @@ class _RoutePlannerScreenState extends ConsumerState<RoutePlannerScreen>
|
|||||||
initialCenter: waypoints.isEmpty
|
initialCenter: waypoints.isEmpty
|
||||||
? const ll.LatLng(0, 0)
|
? const ll.LatLng(0, 0)
|
||||||
: ll.LatLng(waypoints.first.latitude, waypoints.first.longitude),
|
: ll.LatLng(waypoints.first.latitude, waypoints.first.longitude),
|
||||||
initialZoom: waypoints.length <= 1 ? 14 : maxTileZoom - 3,
|
initialZoom: waypoints.length <= 1 ? ambientZoom : maxTileZoom - 3,
|
||||||
maxZoom: maxTileZoom,
|
maxZoom: maxTileZoom,
|
||||||
onTap: (tapPosition, point) {
|
onTap: (tapPosition, point) {
|
||||||
repo.addWaypoint(widget.routeId, point.latitude, point.longitude);
|
repo.addWaypoint(widget.routeId, point.latitude, point.longitude);
|
||||||
|
|||||||
@@ -9,6 +9,7 @@ import 'package:rippr/src/data/database.dart';
|
|||||||
import 'package:rippr/src/data/route_plan_repository.dart';
|
import 'package:rippr/src/data/route_plan_repository.dart';
|
||||||
import 'package:rippr/src/ui/app_shell.dart';
|
import 'package:rippr/src/ui/app_shell.dart';
|
||||||
import 'package:rippr/src/ui/components/floating_pill.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/format.dart';
|
||||||
import 'package:rippr/src/ui/routes/route_planner_screen.dart';
|
import 'package:rippr/src/ui/routes/route_planner_screen.dart';
|
||||||
import 'package:rippr/src/ui/routes/routes_list_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);
|
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<FlutterMap>(find.byType(FlutterMap));
|
||||||
|
expect(map.options.initialZoom, ambientZoom);
|
||||||
|
});
|
||||||
|
|
||||||
screenTest('the offline-tiles download menu item is disabled with no pins '
|
screenTest('the offline-tiles download menu item is disabled with no pins '
|
||||||
'(V3-11, UI-06: now a PopupMenuItem behind the overflow menu)',
|
'(V3-11, UI-06: now a PopupMenuItem behind the overflow menu)',
|
||||||
(tester) async {
|
(tester) async {
|
||||||
|
|||||||
Reference in New Issue
Block a user