fix: viewport loading and state info#623
Conversation
AlperKartkaya
left a comment
There was a problem hiding this comment.
Thanks for the fix. Here are some points we can take a look at before merging.
HIGH (1)
-
GatheringAreasScreen.kt:321-370, 439-455
HelpRequestMapScreen.kt:474-517, 568-584Issue: In-flight viewport requests can clear the newly queued update state.
A new pan/zoom sets viewportUpdateQueued = true, but viewportRequestSerial is not advanced until after the 450ms debounce. If an older fetch is
still running, its finally block can still pass the serial check and clear loading/queued state for the newer viewport.Risk: The new on-map Updating visible area... feedback can disappear too early, undermining the main issue requirement.
Fix: Invalidate/advance a viewport generation immediately when queuing a new fetch-worthy viewport or manual retry. Old request catch/finally
blocks should not mutate state for newer queued viewports.
MEDIUM (1)
-
GatheringAreasRepository.kt:56, 68-79, 106-110
Supporting: JsonHttpClient.kt:14-27Issue: Gathering Areas timeout alignment is not fully reliable.
The repository wraps calls in withTimeoutOrNull(15_000L), but JsonHttpClient uses blocking HttpURLConnection with readTimeout = 20_000.
Coroutine timeout may not reliably interrupt the blocking read.Risk: Loading resources in this area... may last longer than the intended 15 seconds.
Fix: Make the HTTP request cancellable, expose/apply a per-request timeout ≤ 15s, or disconnect the connection when coroutine timeout fires.
ARCHITECTURE WATCHLIST
-
GatheringAreasScreen.kt:341-345, retry paths around provider fallback failures
Concern: Fallback-empty provider failures are correctly not marked as fetched, but resetting viewportRefreshNonce while leaving
lastFetchedViewportKey unmatched can cause a second automatic fetch after a manual retry fails again.Status: WATCH
Recommendation: Track failed provider viewport state explicitly, or clear/consume pendingViewport after provider-unavailable results so retries
stay user-driven without accidental double-fetches.
WHAT LOOKS GOOD
- Shared on-map overlay exists in LeafletMapWebView.kt.
- Both maps place the overlay inside the map container.
- Loading map..., Loading resources in this area..., and Updating visible area... are represented.
- Current-location control remains top-right and visible.
- Empty marker messages are hidden during loading/updating.
- Fallback-empty Gathering Areas responses are not permanently marked as fetched.
- No backend/web/migration scope was introduced.
VALIDATION
Passed:
- git diff --check
- Focused Android tests:
- LeafletMapWebViewTest
- GatheringAreasScreenTest
- HelpRequestMapScreenFilterLogicTest
- Merge-tree conflict check against upstream/development: no textual conflicts.
Not verified:
- Manual emulator/device smoke for pan/zoom overlay timing and marker reliability.
SYNTHESIS
- code-reviewer recommendation: REQUEST CHANGES
- architect status: WATCH
- final recommendation: REQUEST CHANGES
The branch is directionally good, but I would not merge until the in-flight viewport state race is fixed.
|
Looks good. Merging. |
Summary
Improves Android resource map loading and update feedback for Help Request Map and Gathering Areas Map.
This PR makes map/resource loading states visible directly on top of the map and improves Gathering Areas viewport refresh reliability, especially when provider-backed results are slow, unavailable, or temporarily empty.
Changes
Loading map...Loading resources in this area...Updating visible area...Verification
Recommended manual checks:
Help Request Map
Updating visible area...appears on top of the map.Gathering Areas
Loading resources in this area...orUpdating visible area...appears on top of the map.Closes #621