Raise the tile buffer and retain off-screen markers on mobile #193
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/task-250-mobile-tile-buffer"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Task 250. Two of the two options Rob scoped,
updateWhenIdledeliberately untouched.L.tileLayer'skeepBufferraised from Leaflet's default of 2 to 4.L.markerClusterGroup'sremoveOutsideVisibleBoundsset tofalse, so a marker outside the current viewport stays in the DOM instead of being pulled out and re-added on the nextmoveend.keepBuffer— measured, not estimated, and the finding is more modest than the ticket's own working assumption. Read against the vendored Leaflet 1.9.4 source,keepBufferonly decides which already-fetched tiles survive pruning once they scroll off-screen — the fetch queue for the current view is built from the viewport's own bounds alone, with no buffer term. A Playwright harness (real headless Chromium, real vendoredleaflet.js, tile requests intercepted locally so nothing hit a real OSM server) counted requests for a pan-and-return on a mobile-sized viewport (390×844) atkeepBuffer2, 4 and 6: for every distance tested, up to nearly two screen-widths, the default of 2 already avoided every re-fetch on return, and no distance found any difference between 2, 4 and 6. Push a pan far enough (several screen-widths) and all three values fail identically (full re-fetch). Full table and reasoning in ADR-0165.4 is chosen over 6 because no measured scenario showed 6 requesting fewer tiles than 4 — 6 costs strictly more retained off-screen tiles for zero observed benefit. Both changes proceed anyway, per Rob's own scoped direction, because raising
keepBuffernever causes a request for a tile the reader isn't actively viewing (checked against the OSM tile policy's own definition of "bulk downloading"), so there's no cost being accepted for an absent gain — this PR just doesn't claim a benefit the numbers don't show.What this does not fix.
updateWhenIdlestays unset (Rob's confirmed ruling) — no tile loads during a drag either way. A drag into genuinely new territory still shows empty tiles until the finger lifts, regardless ofkeepBuffer. The marker half of the report is fully addressed; the tile half is partial.Verified: the two Leaflet objects
createMapbuilds carry the intended options (MobileTileAndMarkerRetentionJourneyTests, real headless Chromium), and a marker on the opposite side of the world from the current view stays a real DOM element after the view moves. Not verified, and cannot be from here: how a real drag feels on a real phone (task 156 — no browser). Neither drag-zoom (ADR-0143/0146/0147) nor deferred-click (ADR-0144/0145) code is touched.No
Answered byadded to ADR-0061 or ADR-0130 — neither explicitly deferred this question (ADR-0107's own condition), so this is a fresh record, not an answer to either.c8d79e04698d4ffa1682Verdict: mergeable
updateWhenIdleis confirmed untouched anywhere in the diff — only referenced in comments explaining why it stays alone.The candour on the tile half is exactly what was asked for: the PR body and ADR-0165 both say plainly that the marker half is fully fixed and the tile half is partial (still no fetch during a drag, only more already-fetched tiles surviving pruning around the settled view). That's the single most important thing here and it's handled well.
keepBuffer4-vs-6: the measured table is in ADR-0165, the OSM "bulk downloading" definition is quoted rather than asserted, and the conclusion (no scenario found 6 beating 4) follows from the numbers rather than preference. The finding thatkeepBufferdoesn't affect the fetch queue at all — only pruning of already-fetched tiles — checks out against Leaflet's own documented semantics, and it's good that the PR says its own working assumption in the ticket was wrong rather than quietly discarding it.removeOutsideVisibleBounds: false: justified qualitatively (bounded by places-per-caller at hobby scale, clustering already caps on-screen DOM nodes) rather than from an actual measured marker count/render-cost test. That's a softer form of evidence than the tile-request table got. Not a blocker given the scale argument is reasonable and clustering is already in place, but worth knowing it's reasoning-from-scale rather than a measurement, if a caller ever has an unusually large single group.No regression risk in the gesture code — nothing in the diff touches
DoubleTapDragZoomordeferSingleClick/cancelPendingClick, and ADR-0165's reasoning thatremoveOutsideVisibleBoundsonly affectsmoveendpruning (which the drag-zoom gesture doesn't fire mid-drag) is consistent with the existing code.CI history: the fix commit ("Discard setView's chainable return value") is well-explained and consistent with a real, previously-seen defect class (chainable Leaflet methods returning something Playwright can't serialise) — that part checks out. What I couldn't corroborate from available tooling (no log access) is the two
LeafletControlThemingTestsfailures on the prior failing run. That file is untouched by this PR, and the project has documented CI contention elsewhere around this same time (concurrent E2E runs on a single-threaded runner), so contention is the more likely explanation than a regression from these map-option changes — but neither the PR body nor ADR-0165 mentions that failure or the belief that it was unrelated. Worth a line for traceability, not worth blocking on given the file is untouched and the current head is green.ADR-0165 is correctly allocated with the measured figures in it, and the "no
Answered byon ADR-0061/ADR-0130" reasoning holds — I checked both records and neither nameskeepBuffer/removeOutsideVisibleBoundsas an explicit open question, so ADR-0107's condition 3 is correctly applied. No frozen ADR body is edited.British spelling and no AI attribution both check out.
rob referenced this pull request2026-08-17 11:29:14 +00:00