uhryniuk
cf6b9d1d73
Turns docs/FEEDBACK.md into five self-contained tickets, each grounded in exact current code and a concrete implementation approach, so they can be handed to independent subagents with no prior context.
217 lines
12 KiB
Markdown
217 lines
12 KiB
Markdown
# 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
|
|
|
|
## Goal
|
|
The Route Planner screen's map doesn't render at all for the user. Find the real cause
|
|
and fix it, and bring this screen's zoom in line with the rest of the app's new
|
|
street-level default (FB-01).
|
|
|
|
## Context
|
|
Direct user feedback (`docs/FEEDBACK.md`, "Route" section):
|
|
|
|
> This doesn't work at all, the map doesn't render at all... I want it to be the same
|
|
> zoomed in map view, then the user can zoom out and create the route.
|
|
|
|
### High-confidence root cause
|
|
|
|
`lib/src/ui/routes/route_planner_screen.dart`, `build()` (~lines 112-139):
|
|
|
|
```dart
|
|
@override
|
|
Widget build(BuildContext context) {
|
|
final colors = Theme.of(context).colorScheme;
|
|
final route = ref.watch(routePlanProvider(widget.routeId)).valueOrNull;
|
|
final waypointsAsync = ref.watch(routeWaypointsProvider(widget.routeId));
|
|
final waypoints = waypointsAsync.valueOrNull ?? const [];
|
|
final units = ref.watch(unitSystemProvider);
|
|
final repo = ref.read(routePlanRepositoryProvider);
|
|
|
|
if (route == null) {
|
|
return Scaffold(
|
|
backgroundColor: Colors.transparent,
|
|
body: SafeArea(
|
|
child: Center(
|
|
child: Text(
|
|
'This route no longer exists.',
|
|
style: TextStyle(color: colors.outline),
|
|
),
|
|
),
|
|
),
|
|
);
|
|
}
|
|
|
|
return Scaffold(
|
|
backgroundColor: Colors.transparent,
|
|
body: SafeArea(
|
|
child: !waypointsAsync.hasValue
|
|
? const Center(child: CircularProgressIndicator())
|
|
: Stack(
|
|
children: [
|
|
Positioned.fill(
|
|
child: FlutterMap(
|
|
...
|
|
```
|
|
|
|
Both `route` and `waypointsAsync` come from the same kind of provider —
|
|
`routePlanProvider`/`routeWaypointsProvider` are both `StreamProvider.autoDispose.family`
|
|
(`lib/src/app/providers.dart` ~lines 198-203) — so both start in `AsyncLoading` on a
|
|
fresh navigation (e.g. tapping "+ New route" on `RoutesListScreen`, which calls
|
|
`repo.createRoutePlan` then immediately `onOpenRoute?.call(id)` — a brand-new
|
|
`routePlanProvider(id)` family instance with a DB stream that hasn't delivered its
|
|
first row yet). **`waypointsAsync` is correctly guarded** — `!waypointsAsync.hasValue`
|
|
shows a spinner instead of building `FlutterMap` prematurely, with a comment explaining
|
|
exactly why (`initialCenter`/`initialZoom` are read once at construction; building too
|
|
early freezes the camera at null-island forever). **`route` has no equivalent guard** —
|
|
`.valueOrNull` collapses "still loading" and "genuinely doesn't exist" into the same
|
|
`null`, so on that same cold navigation, this screen renders the **text-only "This route
|
|
no longer exists." fallback with zero `FlutterMap` in the tree** — for a route that is
|
|
completely valid, simply because its stream's first emission hasn't arrived yet. This is
|
|
a real, reproducible-on-a-slow-cold-start bug, and it would read to a user as exactly
|
|
"the map doesn't render at all" — because for however long that loading window lasts,
|
|
there is no map, no pins, nothing to interact with.
|
|
|
|
This is the same shape of bug UI-06's own documented Outcome already found once in this
|
|
same file (a hardcoded, stale tile URL in `_DownloadDialog`): a place where near-
|
|
identical logic exists twice nearby and only one copy got the correct guard.
|
|
|
|
### Zoom special-cases to replace
|
|
|
|
Same `build()` method, ~line 157:
|
|
|
|
```dart
|
|
initialZoom: waypoints.length <= 1 ? 14 : maxTileZoom - 3,
|
|
```
|
|
|
|
`initialZoom` is only ever used by flutter_map when `initialCameraFit` is null.
|
|
`initialCameraFit: _initialFit(waypoints)` (line ~153) is non-null for 2+ waypoints
|
|
(see `_initialFit`, ~lines 92-102, which returns `null` only for `waypoints.length < 2`
|
|
or degenerate bounds) — so `maxTileZoom - 3` is **dead, misleading code today**: it can
|
|
only ever apply when `_initialFit` already returned null, which for 2+ waypoints only
|
|
happens on a degenerate (single-point) bounds, in which case `waypoints.length <= 1` is
|
|
false but the fit is still null — meaning `maxTileZoom - 3` actually *does* fire for
|
|
that one edge case (all waypoints at the same spot). Still, `14` for 0-1 waypoints is
|
|
the literal reason a fresh, empty route (or one with a single pin) opens zoomed out
|
|
far past street level, exactly matching the "same zoomed in map view" ask.
|
|
|
|
## Design
|
|
- **Fix the render bug**: change `route` from `.valueOrNull` to keeping the full
|
|
`AsyncValue<RoutePlan?>`, and add a `hasValue` guard mirroring `waypointsAsync`'s
|
|
existing pattern:
|
|
```dart
|
|
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);
|
|
|
|
if (!routeAsync.hasValue) {
|
|
return const Scaffold(
|
|
backgroundColor: Colors.transparent,
|
|
body: Center(child: CircularProgressIndicator()),
|
|
);
|
|
}
|
|
final route = routeAsync.value;
|
|
if (route == null) {
|
|
return Scaffold( /* unchanged "This route no longer exists." body */ );
|
|
}
|
|
```
|
|
This exactly mirrors the two-step pattern `waypointsAsync` already uses two lines
|
|
below it in the same method, just applied consistently to both providers.
|
|
- **Zoom fix**: replace the `initialZoom` line with FB-01's shared constant. FB-01 adds
|
|
`const double ambientZoom = 17.0;` to `lib/src/ui/components/ride_map.dart` — this
|
|
screen already imports named constants from that file (`show TileAttribution,
|
|
maxTileZoom, tileMaxNativeZoom, tileSubdomains, tileUrlTemplate, tileUserAgent`, near
|
|
the top of the file) — add `ambientZoom` to that same `show` clause and use it:
|
|
```dart
|
|
initialZoom: waypoints.length <= 1 ? ambientZoom : maxTileZoom - 3,
|
|
```
|
|
**If FB-01 has not landed yet when this ticket is implemented**, add the same
|
|
constant directly to `ride_map.dart` yourself first (`const double ambientZoom =
|
|
17.0;` near `maxTileZoom`/`shortRideZoom`) so this ticket doesn't block on ordering —
|
|
whichever of FB-01/FB-04 lands second will find the constant already defined and
|
|
should just reuse it rather than redefining it (grep for `ambientZoom` before adding
|
|
it, to avoid a duplicate-constant compile error).
|
|
- **Investigation order, if the above turns out not to be the whole story** (do this
|
|
first, before assuming the fix above is sufficient — reproduce, then fix):
|
|
1. `flutter run` on the Android emulator (or a device), tap "+ New route" from
|
|
`RoutesListScreen` (`lib/src/ui/routes/routes_list_screen.dart`, `Key('new-route')`
|
|
button), watch closely for a flash of "This route no longer exists." text or a
|
|
fully blank screen right after navigation.
|
|
2. If reproduced and the `hasValue` fix above resolves it, done — write up the root
|
|
cause and fix in the Outcome section.
|
|
3. If it's *not* resolved (map still doesn't render even with the `hasValue` guard in
|
|
place), add a temporary `debugPrint('route=${routeAsync.runtimeType} '
|
|
'waypoints=${waypointsAsync.runtimeType}')` at the top of `build()` and watch the
|
|
console across a fresh navigation to see the actual state sequence — remove it
|
|
before committing.
|
|
4. Check whether the map mounts (i.e. `FlutterMap` is in the tree, visible in the
|
|
Flutter inspector / a screenshot) but tiles themselves are blank — if so, look at
|
|
`cachedTileProviderProvider` (`lib/src/app/providers.dart` ~lines 234-238): it
|
|
returns `null` while `tileCacheProvider`'s underlying `FutureProvider` hasn't
|
|
resolved yet, and `TileLayer(tileProvider: null)` falls back to flutter_map's own
|
|
default network fetcher — verify that fallback actually fetches (it should; if it
|
|
silently doesn't, that's the bug).
|
|
5. Diff this screen's tile setup token-for-token against `RideMap`'s
|
|
(`tileUrlTemplate`, `tileSubdomains`, `tileMaxNativeZoom`, `tileUserAgent` — all
|
|
already imported from the same `ride_map.dart` export, so a stale-URL-style
|
|
regression is unlikely, but `grep -n "tile.openstreetmap\|cartocdn"` across this
|
|
file to be certain nothing reintroduced UI-06's already-fixed bug).
|
|
|
|
## Implementation
|
|
1. Change `route` to `routeAsync` (full `AsyncValue`), add the `hasValue` guard, keep
|
|
the rest of the "no longer exists" branch's body unchanged.
|
|
2. Add `ambientZoom` to this file's `ride_map.dart` import `show` clause (adding the
|
|
constant to `ride_map.dart` first if FB-01 hasn't landed yet — see Design).
|
|
3. Replace `waypoints.length <= 1 ? 14 : maxTileZoom - 3` with
|
|
`waypoints.length <= 1 ? ambientZoom : maxTileZoom - 3`.
|
|
4. Reproduce on an emulator per the Investigation order above; if the root cause is
|
|
something other than the `hasValue` gap, document the real cause and its fix in the
|
|
Outcome section instead of (or in addition to) the above.
|
|
|
|
## Acceptance criteria
|
|
- [ ] Opening a brand-new route ("+ New route" from `RoutesListScreen`) always shows a
|
|
loading spinner briefly (if the stream hasn't emitted yet) and then the real map
|
|
canvas — never the "This route no longer exists." text for a route that
|
|
genuinely exists.
|
|
- [ ] Opening an existing, previously-created route with no waypoints shows the map
|
|
canvas at `ambientZoom` (street level), not zoom 14.
|
|
- [ ] A route that is actually deleted (or never existed — e.g. a stale/invalid id)
|
|
still correctly shows "This route no longer exists." — the fix must not weaken
|
|
this case, only stop it from firing on a merely-still-loading valid route.
|
|
- [ ] `flutter analyze` clean, `flutter test` green, test count only goes up.
|
|
|
|
## Tests
|
|
- Widget test: pump `RoutePlannerScreen` for a route id that has a pending (not-yet-
|
|
resolved) `routePlanProvider` stream — assert a `CircularProgressIndicator` shows,
|
|
never the "no longer exists" text, then once the stream emits the real row, assert
|
|
the map (`FlutterMap`) appears. (Check how existing tests in
|
|
`test/route_planner_screen_test.dart` seed/await the repository — likely via
|
|
`repo.createRoutePlan` then pumping a frame or two before the provider's stream
|
|
delivers, similar to the existing `pumpMap` helper in that file's own comments about
|
|
needing a handful of frames for the waypoints stream's first value.)
|
|
- Widget test: pump `RoutePlannerScreen` for an id that was never created (or was
|
|
deleted) — assert "This route no longer exists." still shows once the stream settles
|
|
(not immediately, if the provider briefly reports loading first).
|
|
- Widget test: a route with zero waypoints renders at `ambientZoom`, not `14` — read
|
|
the `FlutterMap`'s `options.initialZoom` via `tester.widget<FlutterMap>(...)` the same
|
|
way other zoom-related assertions in this test suite already inspect `MapOptions`
|
|
(check existing patterns in `test/ride_map_test.dart`/
|
|
`test/route_planner_screen_test.dart`).
|
|
- If the investigation reveals a different root cause than the `hasValue` gap, write a
|
|
regression test for the actual bug found, not just the hypothesis above.
|
|
|
|
## Risks
|
|
- If the `hasValue` fix doesn't fully explain what the user saw, this ticket's
|
|
Outcome section must say so plainly and document whatever the actual root cause
|
|
turned out to be — don't claim a fix that wasn't verified to address the real
|
|
symptom. Reproducing on a real emulator/device before declaring this done is
|
|
important precisely because the original bug report is "doesn't work at all," a
|
|
strong signal, and a subtle timing bug is an easy thing to fix on paper without
|
|
confirming it was actually the cause.
|
|
|
|
## Out of scope
|
|
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.
|