From 3810dd5a26b4e453ad42e57e3680e6d652669a0a Mon Sep 17 00:00:00 2001 From: uhryniuk Date: Tue, 25 Aug 2026 11:10:23 -0500 Subject: [PATCH] Write FB-10/FB-11 tickets: map pan-recenter race, Route Planner tile rendering --- docs/feedback/FB-10-map-pan-recenter-race.md | 191 ++++++++++++++++++ .../FB-11-route-planner-tiles-still-blank.md | 180 +++++++++++++++++ docs/feedback/README.md | 13 +- 3 files changed, 381 insertions(+), 3 deletions(-) create mode 100644 docs/feedback/FB-10-map-pan-recenter-race.md create mode 100644 docs/feedback/FB-11-route-planner-tiles-still-blank.md diff --git a/docs/feedback/FB-10-map-pan-recenter-race.md b/docs/feedback/FB-10-map-pan-recenter-race.md new file mode 100644 index 0000000..ba8068a --- /dev/null +++ b/docs/feedback/FB-10-map-pan-recenter-race.md @@ -0,0 +1,191 @@ +# FB-10 — Manual map pan loses a race against the follow-recenter timer + +**Depends on** — · **Size** M · **Status** Not started + +## Goal +The rider must be able to pan the map and have it stay where they put it. Today a pan +gesture is silently cancelled a moment after it happens, both idle and while recording. + +## Context +Direct user feedback (`docs/FEEDBACK.md`): + +> Map page still cannot be scrolled/panned around -- it stays locked centered on the +> user, both idle and recording. A previous attempt at this fix (FB-06) did not +> actually resolve it on-device. + +FB-06 already removed the old `hasPoints` gate that fully disabled +`InteractiveFlag.drag`/`InteractiveFlag.pinchZoom`. That part of the fix is correct and +confirmed by widget tests. The map is still stuck because of a **different** bug FB-06 +did not find: a timer-driven recenter that keeps re-arming and wins a race against the +user's own drag. + +`lib/src/ui/app_shell.dart` builds one shared `RideMap` behind every tab, and rebuilds +it on every ambient GPS fix: + +```dart +child: RideMap( + key: const Key('shell-background-map'), + points: points, + segments: segments, + ambientPosition: ambientPosition, + follow: isMapTab, + ... +``` + +`ambientPosition` comes from `ambientPositionProvider`, fed by +`GeolocatorLocationSource` at a fixed one-second interval with no distance filter +(`lib/src/recording/geolocator_location_source.dart`): + +```dart +/// Matches the native `LocationRequest`: high accuracy, 1 s nominal interval, no +/// distance filter (a stationary bike must still produce fixes so elapsed time and the +/// noise floor behave). +const Duration _interval = Duration(seconds: 1); +``` + +Because `distanceFilter` is `0`, almost every one-second tick delivers a fix that is at +least slightly different from the last one -- so `AppShell`, and therefore `RideMap`, +rebuilds roughly once per second, indefinitely, on the Map tab. + +`lib/src/ui/components/ride_map.dart`, `didUpdateWidget` (~line 142): + +```dart +void didUpdateWidget(RideMap old) { + super.didUpdateWidget(old); + if (!_following || _backgrounded) return; + if (widget.points.isNotEmpty) { + final last = widget.points.last; + WidgetsBinding.instance.addPostFrameCallback((_) { + if (!mounted || !_following) return; + _controller.move(ll.LatLng(last.latitude, last.longitude), _controller.camera.zoom); + }); + } else if (widget.ambientPosition != null && + widget.ambientPosition != old.ambientPosition) { + final isFirstFix = old.ambientPosition == null; + WidgetsBinding.instance.addPostFrameCallback((_) { + if (!mounted || !_following) return; + _controller.move( + widget.ambientPosition!, + isFirstFix ? ambientZoom : _controller.camera.zoom, + ); + }); + } +} +``` + +The only thing that stops this recenter is `_following` turning `false`, which only +happens inside `onPositionChanged` (~line 277): + +```dart +onPositionChanged: !widget.follow + ? null + : (position, hasGesture) { + if (hasGesture && _following) { + setState(() => _following = false); + } + }, +``` + +This callback is correct on its own -- a controller-driven `.move()` reports +`hasGesture: false`, so it never cancels itself out. The problem is timing, not logic: +`didUpdateWidget` re-arms a fresh `postFrameCallback` on almost every one-second tick, +for as long as `_following` is still `true`. If a tick lands while the rider's finger is +already down and moving but flutter_map has not yet reported `hasGesture: true` for that +drag, the scheduled callback fires anyway and snaps the camera straight back to the +ambient fix -- one frame after the rider moved it. The next tick does the same thing a +second later. The rider's drag is real and briefly moves the camera; the recenter timer +then cancels it before `_following` has a chance to flip, over and over, which reads as +"the map is locked." + +`test/ride_map_test.dart` never actually reproduces this, because every "manual pan" +test drives the bug-free half of the code by calling the callback directly instead of +performing a real drag: + +```dart +var map = tester.widget(find.byType(FlutterMap)); +map.options.onPositionChanged!(map.mapController!.camera, true); +``` + +A test built this way cannot observe the race: it sets `hasGesture: true` instantly, +with no competing `postFrameCallback` scheduled in between. This is why FB-06's own +tests passed while the on-device bug remained. + +## Design +Track whether a real pointer gesture is currently in progress on the map, and skip the +follow-recenter entirely while one is. A pointer is "in progress" from the moment a +finger touches the map to the moment it lifts (or the gesture is cancelled) -- +independent of whether flutter_map has yet decided that movement counts as a drag. + +- Add a private `bool _gestureInProgress = false;` field to `_RideMapState`. +- Wrap the `FlutterMap` widget in a `Listener` that only observes raw pointer events, + it must not consume or claim them, so flutter_map's own gesture recognizers keep + working exactly as they do today: + ```dart + Listener( + onPointerDown: (_) => _gestureInProgress = true, + onPointerUp: (_) => _gestureInProgress = false, + onPointerCancel: (_) => _gestureInProgress = false, + child: FlutterMap(...), + ) + ``` +- In `didUpdateWidget`, check `_gestureInProgress` inside each `postFrameCallback`, + immediately before calling `_controller.move(...)`, in addition to the existing + `mounted`/`_following` checks. Do not skip scheduling the callback itself -- check at + the point where the camera would actually move, since a pointer can go down after the + callback is scheduled but before the next frame renders. +- Leave `onPositionChanged`'s existing `hasGesture` check exactly as it is. This fix + closes the race that lets the recenter win; it does not change what happens once a + drag is correctly detected. + +## Implementation +1. Add the `_gestureInProgress` field to `_RideMapState` in + `lib/src/ui/components/ride_map.dart`. +2. Wrap the existing `FlutterMap` widget in a `Listener` with `onPointerDown`, + `onPointerUp`, and `onPointerCancel` handlers that set `_gestureInProgress`. +3. Add `if (_gestureInProgress) return;` to both `postFrameCallback` bodies in + `didUpdateWidget`, alongside the existing `if (!mounted || !_following) return;` + check. +4. Add a widget test to `test/ride_map_test.dart` that reproduces the race directly: + start a real drag with `tester.startGesture(...)`, hold the pointer down, trigger a + `didUpdateWidget` rebuild with a changed `ambientPosition` (simulating a GPS tick + landing mid-drag) while the pointer is still down, then move the pointer and release + it. Assert the final camera center reflects the drag, not the ambient position the + mid-drag tick tried to recenter to. +5. Run the app on the Android emulator. Set a mock GPS fix so ambient ticks keep + arriving. Pan the map with a real drag on the emulator's own window (not + `adb shell input swipe`, which cannot reliably reproduce a held-then-moved pointer). + Confirm the camera stays where it was dragged to and does not snap back. + +## Acceptance criteria +- [ ] A pan gesture on the idle Map tab moves the camera and it stays moved, even while + ambient GPS fixes keep arriving once per second. +- [ ] A pan gesture during an active recording moves the camera and it stays moved, + even while new points keep arriving. +- [ ] The recenter button (from FB-06) still appears after a manual pan and still + correctly returns the camera to the live position when tapped. +- [ ] The new widget test reproduces the race and fails without the fix, passes with it. +- [ ] `flutter analyze` clean, `flutter test` green, test count only goes up. + +## Tests +- New test in `test/ride_map_test.dart`: a real `tester.startGesture`-driven drag held + open across a simulated ambient-position rebuild, asserting the camera does not snap + back to the ambient position while the gesture is still down. +- Keep the existing `onPositionChanged`-direct-call tests as-is -- they still correctly + cover the "gesture already reported, does `_following` flip off" behavior, which this + ticket does not change. + +## Risks +`Listener`'s `onPointerDown`/`onPointerUp` fire for every pointer, including a tap that +never becomes a drag (e.g. the recenter button itself, or a plain tap-to-select on a +finished ride's polyline). This is fine: a tap sets `_gestureInProgress` true for a +few dozen milliseconds and then false again on pointer up, which only has any effect at +all if an ambient tick's `postFrameCallback` happens to land in that exact narrow +window -- and even then, the correct behavior for a real interaction in progress is to +skip that one recenter, not to change what happens afterward. + +## Out of scope +Any change to how often ambient position ticks arrive (`_interval` in +`geolocator_location_source.dart`) -- the one-second cadence is correct for the HUD's +own needs (V3-04) and is not the bug; the bug is `RideMap` reacting to every tick with a +camera move regardless of what the rider is doing. FB-11's Route Planner tile issue, +tracked separately. diff --git a/docs/feedback/FB-11-route-planner-tiles-still-blank.md b/docs/feedback/FB-11-route-planner-tiles-still-blank.md new file mode 100644 index 0000000..fd365f9 --- /dev/null +++ b/docs/feedback/FB-11-route-planner-tiles-still-blank.md @@ -0,0 +1,180 @@ +# FB-11 — Route Planner map still shows no tiles + +**Depends on** — · **Size** M/L · **Status** Not started + +## Goal +Opening the Route Planner (the "+" button, or tapping any existing route) must show a +real, detailed map underneath the pins. Today it shows a flat, featureless area. + +## Context +Direct user feedback (`docs/FEEDBACK.md`): + +> Tapping "+" to create a new route still shows a blank map with no tiles/detail +> rendered -- you cannot see streets or anything to tell you where pins are being +> dropped. A previous attempt at this fix (FB-07) did not actually resolve it +> on-device. + +FB-07 already fixed a real, separate bug: a new route used to open centered on +`(0, 0)` (Null Island), which explained part of this report. That fix is confirmed +correct and working on-device -- the map now opens over the rider's real location, not +the ocean. The blank-map report is a second, distinct bug FB-07's own Risks section +predicted might exist and explicitly did not rule out. + +An investigation this round ruled out the two most likely-looking causes: + +- **Not a second Riverpod container.** `lib/main.dart` has exactly one `ProviderScope` + for the whole app. `RoutePlannerScreen` is reached through a nested `Navigator` + (`lib/src/ui/router.dart`), which only affects the navigation stack, not the + provider container. `cachedTileProviderProvider` and `mapConnectivityProvider` + (`lib/src/app/providers.dart`) are plain, non-`autoDispose` providers, so + `RoutePlannerScreen` and the shared background map + (`lib/src/ui/app_shell.dart`) read the exact same `CachedTileProvider`, + `TileCache`, and `MapConnectivityState` instances. There is no isolation between + them. +- **Not stuck skeleton mode.** `MapConnectivityState` (`lib/src/tiles/map_connectivity.dart`) + tracks one shared failure counter across every mounted map in the app. + `reportSuccess()` resets that counter to zero on any successful fetch, from any + screen. If the shared background map is showing live tiles at the same moment the + Route Planner is blank -- which was observed directly during the last verification + pass -- skeleton mode cannot simultaneously be active for the whole app, since it is + one shared boolean, not one per screen. Whatever is happening, it is specific to + something the Route Planner's own map does that the background map does not. + +One real, evidenced hazard was found in `lib/src/tiles/tile_cache.dart`, `put()`: + +```dart +@override +Future put(TileKey key, Uint8List bytes) async { + await _ensureLoaded(); + final name = _fileName(key); + _manifest.remove(name); + if (_totalBytes() + bytes.length > maxBytes) { + await _evictUntilFits(bytes.length); + } + await _tileFile(key).writeAsBytes(bytes); + _manifest[name] = _Entry(bytes: bytes.length, lastAccess: _clock++); + await _saveManifest(); +} +``` + +`FileTileCache` is one shared instance (`tileCacheProvider` in `providers.dart`), used +by every `TileLayer` in the app. `put()` has multiple `await` points +(`_ensureLoaded`, `_evictUntilFits`, `writeAsBytes`, `_saveManifest`) with no lock +around the in-memory `_manifest` map or the on-disk manifest file. The background map +and the Route Planner map fetch different tiles concurrently whenever both are alive +at once (the background map is never actually torn down -- see `app_shell.dart`'s +comment on why it is one persistent instance). Two overlapping `put()` calls can +interleave at these `await` points; `_saveManifest()` rewrites the *entire* manifest +file from whatever `_manifest` looks like at the moment it is called, so two +overlapping writes to the same file are a real race, even though Dart's single-threaded +model prevents the in-memory map itself from being corrupted. + +`lib/src/tiles/cached_tile_provider.dart`'s fetch failure path currently discards the +actual cause before rethrowing: + +```dart +Future _fetchAndStore() async { + try { + final response = await client.get(Uri.parse(url), headers: headers); + if (response.statusCode != 200) { + throw Exception('Tile fetch failed: ${response.statusCode} for $url'); + } + ... + } catch (_) { + connectivity?.reportFailure(); + rethrow; + } +} +``` + +The `catch (_)` block never records the URL, status code, or exception anywhere a +developer could see it -- so there is currently no way to tell, from a real device, +whether a Route Planner tile fetch is failing outright (and why), succeeding but +failing to render, or something else entirely. `test/route_planner_screen_test.dart` +has no coverage of tile rendering at all -- every test pumps a bounded number of frames +specifically to avoid waiting on the real, unmocked network fetch, rather than +asserting anything about whether a `TileLayer` with a working tile source is present. + +## Design +Two independent changes, both worth making regardless of which one turns out to be the +actual fix, because both are real defects in their own right: + +1. **Make `FileTileCache` safe under concurrent use.** Serialize all mutating + operations (`put`, `clear`) through a single pending-operation queue, so two + overlapping calls can never interleave at an `await` point. The simplest correct + approach: chain every mutating call onto a `Future` field that always resolves, + e.g.: + ```dart + Future _queue = Future.value(); + + Future _serialized(Future Function() op) { + final result = _queue.then((_) => op()); + _queue = result.then((_) {}, onError: (_) {}); + return result; + } + ``` + Wrap the bodies of `put()` and `clear()` in `_serialized(...)`. Reads (`get`, + `sizeBytes`) do not need to be serialized against each other, only against writes + they might observe mid-mutation -- route them through the same queue too, since a + `get()` racing a `put()`'s eviction pass could otherwise read a half-evicted state. +2. **Log real tile-fetch failures.** In `cached_tile_provider.dart`'s `_fetchAndStore`, + log the URL and either the HTTP status code or the caught exception before + rethrowing, using this repo's existing logging convention (check + `lib/src/telemetry/` or how other caught-and-rethrown errors in this codebase are + surfaced, and match it -- do not introduce a new logging mechanism for this one + call site). + +## Implementation +1. Add the serialization queue to `FileTileCache` in `lib/src/tiles/tile_cache.dart`. + Wrap `put()` and `clear()` bodies in it. Route `get()` and `sizeBytes()` through it + too. +2. Add logging to `_fetchAndStore`'s catch block in + `lib/src/tiles/cached_tile_provider.dart`, matching this repo's existing logging + pattern. +3. Add a concurrency test to `test/tile_cache_test.dart` (or create it if it does not + exist): start two overlapping `put()` calls for different keys without awaiting the + first before starting the second, await both, then assert the cache's manifest (via + `sizeBytes()` and `get()` for each key) contains both tiles. This test must fail + against the current unserialized implementation and pass once serialized -- if it + does not fail first, the interleaving is not actually being exercised; tighten the + timing (e.g. an artificial delay in a fake `Directory`/file layer) until it does. +4. Run the app on the Android emulator with the new logging in place. Set a mock GPS + fix. Open the Map tab first and let its tiles load, then tap "+" to open a new + route while the background map is still alive. Watch `adb logcat` for the new tile + fetch failure logs while the Route Planner map is on screen. +5. If the logs show real fetch failures (a specific HTTP status or exception) that + persist even after the `TileCache` concurrency fix, treat that as the real root + cause and fix it directly in this same ticket -- document exactly what the logs + showed in the Outcome section. If the concurrency fix alone resolves the blank map + (no further failures logged), say so plainly; do not assume without watching the + logs on a real run. +6. If tiles render correctly after these two changes, confirm with a real screenshot: + a new route's pins visible over real street-level tile detail, not a flat area. + +## Acceptance criteria +- [ ] Opening a new route via "+" shows real street-level tile detail under the pins, + confirmed with a real on-device screenshot. +- [ ] Opening an existing route with waypoints also shows real tile detail. +- [ ] The new `TileCache` concurrency test fails without the serialization fix and + passes with it. +- [ ] The Outcome section states plainly what the on-device logs showed, and whether + the concurrency fix alone resolved the blank map or a further root cause was + found and fixed. +- [ ] `flutter analyze` clean, `flutter test` green, test count only goes up. + +## Tests +- `test/tile_cache_test.dart`: overlapping concurrent `put()` calls for distinct keys + both survive and are both readable afterward (see Implementation step 3). +- If a further root cause is found via the on-device logs (e.g. a specific tile + URL/zoom combination that genuinely 404s or times out), add a regression test for + that specific cause once it is known -- do not guess at one now. + +## Risks +The `TileCache` concurrency fix may not be the actual root cause -- the investigation +that found it could not fully confirm it against a live repro. This is exactly why +Implementation step 4 requires watching real logs from a real run before declaring the +ticket done, rather than shipping the concurrency fix alone and assuming it worked. + +## Out of scope +FB-10's map-pan race, tracked separately. Any change to which tile provider or map +style this app uses. diff --git a/docs/feedback/README.md b/docs/feedback/README.md index 55e6422..e880c4f 100644 --- a/docs/feedback/README.md +++ b/docs/feedback/README.md @@ -5,9 +5,10 @@ Implementation · Acceptance criteria · Tests · Risks · Out of scope, written implementing, with an Outcome section appended after. Source: `docs/FEEDBACK.md` — hands-on feedback after using the redesigned app. Turned -into 9 tickets so far (5 from the first pass, 4 from a second round of feedback after -FB-01..FB-05 shipped), each independently completable (self-contained enough for a -fresh subagent with no prior context to implement correctly). +into 11 tickets so far (5 from the first pass, 4 from a second round of feedback after +FB-01..FB-05 shipped, 2 from a third round after FB-06/FB-07 turned out not to actually +resolve on-device), each independently completable (self-contained enough for a fresh +subagent with no prior context to implement correctly). ## The tickets @@ -22,6 +23,8 @@ fresh subagent with no prior context to implement correctly). | [FB-07](FB-07-route-planner-null-island.md) | Route Planner opens on Null Island instead of your real location | S/M | FB-01 (ambient location provider) | Done | | [FB-08](FB-08-hud-reflow-on-hide.md) | HUD widgets reflow to fill the gap when one is hidden | M | — | Done | | [FB-09](FB-09-hud-title-driven-sizing.md) | HUD default sizing follows the title, value adapts | L | FB-08 (same files) | Done | +| [FB-10](FB-10-map-pan-recenter-race.md) | Manual map pan loses a race against the follow-recenter timer | M | — | Not started | +| [FB-11](FB-11-route-planner-tiles-still-blank.md) | Route Planner map still shows no tiles | M/L | — | Not started | ## Dependencies / dispatch order @@ -32,6 +35,10 @@ Wave 3 (after Wave 2 lands): FB-05 — heavily edits the same file FB-04 just to Wave 4 (parallel): FB-06, FB-07, FB-08 — no file overlap between these three Wave 5 (after Wave 4 lands): FB-09 — heavily edits the same files FB-08 just touched + +Wave 6 (parallel): FB-10, FB-11 — no file overlap between these two. Both are follow-up +fixes for real bugs FB-06/FB-07 left unresolved on-device; see each ticket's Context +for what the earlier fix did and did not solve. ``` FB-02 and FB-03 both touch `lib/src/ui/record/record_screen.dart`, but disjoint