fix: run zoom handler synchronously #33

Merged
graham merged 3 commits from fix/m1-zoom-movement-debounce into main 2026-09-15 16:33:53 -05:00
Owner

Fixes M1 and M2 from bugs.md.

M1: zoomend and moveend shared boundsTimer, and Leaflet fires moveend synchronously after zoomend, so the zoom handler's timer was always cleared microseconds after arming — its body never ran. Trails were only attached at init when the page loaded at zoom >= 11, lastZoom never updated, and the OMS rebuild on crossing the clustering threshold never fired. The zoom logic now runs synchronously in zoomend; only saveMapState stays debounced in moveend.

M2: with the body now actually running, the pushEvent("refresh_markers") on zoom jumps > 2 would crash the LiveView — the server has no handler for it. Deleted the push and the dead client-side refresh_markers registration; the zoom handler now trims out-of-viewport markers locally via removeMarkersOutsideBounds, mirroring what the dead handler did.

Verification: mix esbuild map_js builds (190.4kb), full suite 2235 passed, grep confirms zero remaining refresh_markers references and boundsTimer used only by the moveend debounce.

Fixes M1 and M2 from bugs.md. M1: `zoomend` and `moveend` shared `boundsTimer`, and Leaflet fires `moveend` synchronously after `zoomend`, so the zoom handler's timer was always cleared microseconds after arming — its body never ran. Trails were only attached at init when the page loaded at zoom >= 11, `lastZoom` never updated, and the OMS rebuild on crossing the clustering threshold never fired. The zoom logic now runs synchronously in `zoomend`; only `saveMapState` stays debounced in `moveend`. M2: with the body now actually running, the `pushEvent("refresh_markers")` on zoom jumps > 2 would crash the LiveView — the server has no handler for it. Deleted the push and the dead client-side `refresh_markers` registration; the zoom handler now trims out-of-viewport markers locally via `removeMarkersOutsideBounds`, mirroring what the dead handler did. Verification: `mix esbuild map_js` builds (190.4kb), full suite 2235 passed, grep confirms zero remaining `refresh_markers` references and `boundsTimer` used only by the moveend debounce.
fix: run zoom handler synchronously
Some checks failed
Elixir CI / Dialyzer (pull_request) Successful in 40s
Elixir CI / Build and test (pull_request) Successful in 1m48s
Elixir CI / Build and Push Docker Image (pull_request) Has been skipped
skippy-bot/review Skippy review: 1 blocking finding open — see the PR thread
59dc71f20a
skippy-bot left a comment

🤖 Skippy PR review

1 finding — 1 blocking before merge.

Severity Location Issue
🟡 Warning assets/js/map.ts:564 Programmatic zooms still skip the handler this PR just activated

Reviewed 59dc71f20a91. Comment skippy review to re-run.

### 🤖 Skippy PR review **1 finding** — 1 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟡 Warning | `assets/js/map.ts:564` | Programmatic zooms still skip the handler this PR just activated | <sub>Reviewed `59dc71f20a91`. Comment `skippy review` to re-run.</sub> <!-- skippy-pr-review -->
assets/js/map.ts Outdated
@ -556,3 +561,4 @@
// `saveMapState` stays debounced, in `moveend`.
const zoomEndHandler = () => {
// Skip if this is a programmatic move from the server
if (self.programmaticMoveId) {
First-time contributor

🟡 Warning — Programmatic zooms still skip the handler this PR just activated

The programmaticMoveId early return stays in front of the body you just made synchronous, so every server-driven zoom still bypasses it — including the trail-layer sync and lastZoom.

Concretely: load the map at the default zoom 5 (trail layer not attached — trailLayer.addTo only happens at init map.ts:408 and in this handler), then press Locate. set_location pushes zoom_to_location at zoom 12 (events.ex:45), the client handler sets programmaticMoveId and calls setView, so zoomend returns here and self.map.hasLayer(self.trailLayer) is never re-checked. The map sits at zoom 12 with trails permanently hidden until the user zooms by hand — the same symptom M1 was opened for.

lastZoom is also left at 5, so the next real zoom computes zoomDifference and the wasUnclustered/isUnclustered comparison against a zoom the map left long ago, firing the new removeMarkersOutsideBounds prune (and, in the other direction, the OMS rebuild) on a false baseline.

Fix: update self.lastZoom = currentZoom and run the trail-layer sync before the programmaticMoveId check, or clear the flag when the setView animation ends so the handler sees the real zoom.

**🟡 Warning — Programmatic zooms still skip the handler this PR just activated** The `programmaticMoveId` early return stays in front of the body you just made synchronous, so every server-driven zoom still bypasses it — including the trail-layer sync and `lastZoom`. Concretely: load the map at the default zoom 5 (trail layer not attached — `trailLayer.addTo` only happens at init map.ts:408 and in this handler), then press Locate. `set_location` pushes `zoom_to_location` at zoom 12 (`events.ex:45`), the client handler sets `programmaticMoveId` and calls `setView`, so `zoomend` returns here and `self.map.hasLayer(self.trailLayer)` is never re-checked. The map sits at zoom 12 with trails permanently hidden until the user zooms by hand — the same symptom M1 was opened for. `lastZoom` is also left at 5, so the next real zoom computes `zoomDifference` and the `wasUnclustered`/`isUnclustered` comparison against a zoom the map left long ago, firing the new `removeMarkersOutsideBounds` prune (and, in the other direction, the OMS rebuild) on a false baseline. Fix: update `self.lastZoom = currentZoom` and run the trail-layer sync before the `programmaticMoveId` check, or clear the flag when the `setView` animation ends so the handler sees the real zoom.
skippy-bot marked this conversation as resolved
fix: maintain zoom state on programmatic zooms
All checks were successful
Elixir CI / Build and test (pull_request) Successful in 1m4s
skippy-bot/review Skippy review: clean — no open findings
Elixir CI / Dialyzer (pull_request) Successful in 2m42s
Elixir CI / Build and Push Docker Image (pull_request) Has been skipped
733fdb4c8e
First-time contributor

Resolved 1 of 1 earlier findings: programmaticMoveId no longer guards zoomEndHandler, so server-driven zooms now update lastZoom and re-check the trail layer (both axes of the earlier warning). 0 still open.

Nothing new in 59dc71f..733fdb4 - the only change is the comment block documenting the removal. Re-checked the M2 premise independently at this head: no refresh_markers push or registration remains anywhere in the tree (only the explanatory comment), and boundsTimer is used solely by the moveend debounce plus its cleanup.

Resolved 1 of 1 earlier findings: `programmaticMoveId` no longer guards `zoomEndHandler`, so server-driven zooms now update `lastZoom` and re-check the trail layer (both axes of the earlier warning). 0 still open. Nothing new in `59dc71f..733fdb4` - the only change is the comment block documenting the removal. Re-checked the M2 premise independently at this head: no `refresh_markers` push or registration remains anywhere in the tree (only the explanatory comment), and `boundsTimer` is used solely by the `moveend` debounce plus its cleanup. <!-- skippy-pr-review -->
fix: correct suppression comment after zoomend change
All checks were successful
Elixir CI / Dialyzer (pull_request) Successful in 38s
skippy-bot/review Skippy review: clean — no open findings
Elixir CI / Build and test (pull_request) Successful in 1m37s
Elixir CI / Build and Push Docker Image (pull_request) Has been skipped
9945f36edf
First-time contributor

Nothing new in 733fdb4..9945f36 - the only change is the corrected suppression comment at map.ts:983 ("moveend was suppressed", which matches the code: the moveend handler still early-returns on programmaticMoveId while the timeout is what reports the final state).

0 findings open. Re-verified the M2 premise independently at this head: zero refresh_markers pushes or registrations anywhere in the tree (one explanatory comment), boundsTimer only feeds the moveend debounce plus cleanup, and the zoom-time prune at map.ts:626 mirrors the server-driven filter_markers_by_bounds handler at map.ts:1551.

Nothing new in `733fdb4..9945f36` - the only change is the corrected suppression comment at `map.ts:983` ("moveend was suppressed", which matches the code: the `moveend` handler still early-returns on `programmaticMoveId` while the timeout is what reports the final state). 0 findings open. Re-verified the M2 premise independently at this head: zero `refresh_markers` pushes or registrations anywhere in the tree (one explanatory comment), `boundsTimer` only feeds the `moveend` debounce plus cleanup, and the zoom-time prune at `map.ts:626` mirrors the server-driven `filter_markers_by_bounds` handler at `map.ts:1551`. <!-- skippy-pr-review -->
graham merged commit aed1130197 into main 2026-09-15 16:33:53 -05:00
graham deleted branch fix/m1-zoom-movement-debounce 2026-09-15 16:33:53 -05:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
graham/aprs.me!33
No description provided.