Progressive rendering during parallel quadrant fetching#133
Open
dougborg wants to merge 4 commits intoFoggedLens:mainfrom
Open
Progressive rendering during parallel quadrant fetching#133dougborg wants to merge 4 commits intoFoggedLens:mainfrom
dougborg wants to merge 4 commits intoFoggedLens:mainfrom
Conversation
When Overpass queries hit the 50k node limit, _fetchSplitAreas() splits the area into 4 quadrants. Previously these were fetched sequentially — at max split depth (3) that's up to 64 serial HTTP requests, which is the primary cause of the slow "splitting" network status users report. This change: - Parallelizes quadrant fetches with Future.wait() instead of a for-loop - Adds a dynamic concurrency limiter (_AsyncSemaphore) sized from the Overpass /api/status slot count, so we never exceed the server's per-IP rate limit - Replaces the "sleep 30s and give up on 429" behavior with smart retry: polls /api/status until a slot is available (modeled after OSMnx and OSMPythonTools), then retries up to 2 times - Resizes the semaphore on retry with the latest observed slot count, adapting to changing server conditions Refactors for testability: - HTTP client injection in OverpassService (following RoutingService pattern) - DI constructors + forTesting() factories on NodeDataManager and NodeSpatialCache - splitBounds made static + @VisibleForTesting for direct unit testing 18 new tests covering: splitBounds geometry, getSlotCount/waitForSlot parsing, fetchWithSplitting (happy path, split, max depth, rate limit retry + cap), partial/total quadrant failure, recursive splitting, and semaphore init deduplication. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When the user pans/zooms mid-fetch, queued sub-requests now bail out instead of blocking the semaphore. Each getNodesFor() call increments _fetchGeneration; fetchWithSplitting checks _isStale(generation) at 6 cooperative checkpoints (before semaphore, inside lambda, before splitting, before/after waitForSlot, top of _fetchSplitAreas). Null generation (offline download, existing tests) is never stale, so there is zero regression risk for non-production-path callers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Each completed quadrant now triggers notifyListeners() immediately so the map renders nodes as they arrive, rather than waiting for all quadrants to finish. NodeProviderWithCache listens to NodeDataManager directly, replacing the manual notifyListeners() after getNodesFor() returns. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Enables progressive map updates while fetching Overpass quadrants in parallel by propagating mid-fetch NodeDataManager cache updates through NodeProviderWithCache, and adds unit tests for the per-quadrant notification behavior.
Changes:
- Emit
NodeDataManager.notifyListeners()as each non-empty quadrant result is cached, enabling progressive rendering. - Have
NodeProviderWithCachesubscribe toNodeDataManagerupdates (and remove the manual post-fetch provider notification). - Add tests (and
fake_async) to verify per-quadrant notification behavior and Overpass status polling behavior.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
lib/services/node_data_manager.dart |
Adds per-quadrant notifyListeners() after caching and threads stale-generation + semaphore logic through split fetches. |
lib/widgets/node_provider_with_cache.dart |
Subscribes provider to NodeDataManager updates; removes redundant provider-side notify after fetch. |
lib/services/overpass_service.dart |
Adds /api/status slot-count parsing and polling wait helper for rate limiting. |
lib/services/node_spatial_cache.dart |
Adds a forTesting constructor to allow non-singleton cache instances in tests. |
lib/services/map_data_provider.dart |
Documents that offline downloads pass null generation (not stale-cancelled) and may occupy semaphore slots. |
test/services/node_data_manager_test.dart |
Adds extensive tests including progressive rendering notification counts. |
pubspec.yaml |
Adds fake_async as a direct dev dependency. |
pubspec.lock |
Updates dependency metadata for fake_async to direct dev. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Collaborator
Author
|
This one could merge in place of #114 - it has all those commits too. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
NodeProviderWithCachelistens toNodeDataManagerdirectly, replacing the manual post-fetchnotifyListeners()callProblem
On
main,_fetchSplitAreas()fetches quadrants sequentially in aforloop. The only intentional UI notification happens aftergetNodesFor()returns — meaning the code is designed so the user sees nothing until every single quadrant (up to 64 at max split depth) has completed. The whole pipeline is:NodeProviderWithCache.fetchAndUpdate()callsawait _nodeDataManager.getNodesFor(...)getNodesFor()callsfetchWithSplitting(), which calls_fetchSplitAreas()_fetchSplitAreas()loops through quadrants one-by-one withawaitgetNodesFor()callnotifyListeners()fetchAndUpdate()then callsnotifyListeners()again on the providerAccidental progressive rendering on main
There is, however, an accidental progressive rendering path that already exists on
main. Users may have noticed partial data appearing during split fetches — but only while actively interacting with the map:onPositionChangedinmap_view.dart(line 408) callssetState(() {})on every frame during a pan/zoom gesturesetStatetriggers a widget rebuild, which callsMapDataManager.getNodesForRendering()→NodeProviderWithCache.getCachedNodesForBounds()→ reads directly from the sharedNodeSpatialCachesingleton_fetchSplitAreasloop writes each completed quadrant's data into that same shared singleton cache via_cache.markAreaAsFetched()setStaterebuilds accidentally pick up partially-written quadrant data from the cacheThis means progressive rendering on
mainis gesture-dependent: if the map is completely still (no touches), nosetStatefires, and nothing re-reads the cache until all quadrants finish. The user only sees partial results if they happen to be panning/zooming during the fetch.Why parallelization alone doesn't fix rendering
PR #114 parallelized the fetching (replacing the
forloop withFuture.wait()), butFuture.wait()still waits for all futures to resolve before returning. The singlenotifyListeners()at the end ofgetNodesFor()still fires only once, after every quadrant has finished. The user still sees the same all-or-nothing behavior when the map is still — just faster overall. The accidental gesture-dependent path still works the same way.Fix
Two small changes make progressive rendering intentional and reliable, regardless of whether the user is interacting with the map:
NodeDataManager.fetchWithSplitting()— callnotifyListeners()right after_cache.markAreaAsFetched()when a quadrant returns non-empty data. Since each quadrant runs as its own async task insideFuture.wait(), this fires as soon as that quadrant's HTTP response arrives — not when all four finish.NodeProviderWithCache— register as a listener onNodeDataManagerso those mid-flight notifications propagate to the map widget. This also removes the now-redundant manualnotifyListeners()afterawait getNodesFor()returns, which was causing a double rebuild.Why this is safe
setStatecalls within the same frame, so even at max split depth (64 quadrants), rapid-fire notifications don't cause 64 separate framesonPositionChangedsetState and our listener-triggered setState coalesce within the same frame — no extra rebuildsnodes.isNotEmptyguard skips empty quadrant resultsgetCachedNodesForBounds()is a cheap in-memory read from the sharedNodeSpatialCachesingletonNodeDataManagerandNodeProviderWithCacheare singletons sharing the same cache, so data written by one is immediately visible to the othernotifyListeners()ingetNodesFor()(line 278) is kept — it's still needed for the non-split happy path and empty-result casesTest plan
flutter test test/services/node_data_manager_test.dart— 26 tests pass (2 new progressive rendering tests)flutter test— 92 tests pass, no regressionsflutter analyze— no issues🤖 Generated with Claude Code