fix: normalize client and server map bounds #34

graham · 2026-09-15T00:59:23-05:00

graham commented

2026-09-15 00:59:23 -05:00

Fixes M3 from bugs.md.

With worldCopyJump: true the Leaflet center wraps but the bounds edges don't, so a viewport at the antimeridian arrived as west: 170, east: 190 and zoom 1–2 as west < -180; validate_bounds rejected both and the client froze on stale bounds — no spatial update, no historical load.

client (map_helpers.ts): wrap each edge into [-180, 180) (double-modulo keeps JS remainder signs out); a span ≥ 360° sends west: -180, east: 180
server (bounds_updater.ex): longitudes now wrap modulo 360 instead of being rejected; spans ≥ 360° collapse to the whole world, which the area cap (64,800 = full globe) already permits
tests: the antimeridian/oversized rejection cases now assert normalization; the pending-bounds test uses a genuinely invalid payload (south > north) to keep its intent

Verification: focused map_live suite 378 passed, full suite 2235 passed, credo strict clean, mix esbuild map_js builds.

Fixes M3 from bugs.md. With `worldCopyJump: true` the Leaflet center wraps but the bounds edges don't, so a viewport at the antimeridian arrived as `west: 170, east: 190` and zoom 1–2 as `west < -180`; `validate_bounds` rejected both and the client froze on stale bounds — no spatial update, no historical load. - client (`map_helpers.ts`): wrap each edge into `[-180, 180)` (double-modulo keeps JS remainder signs out); a span ≥ 360° sends `west: -180, east: 180` - server (`bounds_updater.ex`): longitudes now wrap modulo 360 instead of being rejected; spans ≥ 360° collapse to the whole world, which the area cap (64,800 = full globe) already permits - tests: the antimeridian/oversized rejection cases now assert normalization; the pending-bounds test uses a genuinely invalid payload (south > north) to keep its intent Verification: focused map_live suite 378 passed, full suite 2235 passed, credo strict clean, `mix esbuild map_js` builds.

graham added 1 commit

2026-09-15 00:59:23 -05:00

fix: normalize client and server map bounds

Elixir CI / Dialyzer (pull_request) Successful in 40s

Details

Elixir CI / Build and test (pull_request) Failing after 1m53s

Details

Elixir CI / Build and Push Docker Image (pull_request) Has been skipped

Details

skippy-bot/review Skippy review: clean — 1 non-blocking note open

Details

cbdb77aa9a

skippy-bot reviewed

2026-09-15 01:03:28 -05:00

skippy-bot left a comment

🤖 Skippy PR review

1 finding — none blocking.

Severity	Location	Issue
🔵 Suggestion	`test/aprsme_web/live/map_live/bounds_updater_test.exs:194`	Rewritten pending-bounds test passes for the wrong reason

_{Reviewed cbdb77aa9ac2. Comment skippy review to re-run.}

### 🤖 Skippy PR review **1 finding** — none blocking. | Severity | Location | Issue | | --- | --- | --- | | 🔵 Suggestion | `test/aprsme_web/live/map_live/bounds_updater_test.exs:194` | Rewritten pending-bounds test passes for the wrong reason | <sub>Reviewed `cbdb77aa9ac2`. Comment `skippy review` to re-run.</sub>

test/aprsme_web/live/map_live/bounds_updater_test.exs Outdated

					
				@ -175,3 +192,3 @@

				      {:noreply, result} =

				        BoundsUpdater.handle_bounds_update(%{north: 34.0, south: 32.0, east: 1.0e9, west: -97.0}, socket)

				        BoundsUpdater.handle_bounds_update(%{north: 34.0, south: 40.0, east: -90.0, west: -97.0}, socket)

🔵 Suggestion — Rewritten pending-bounds test passes for the wrong reason

%{north: 34.0, south: 40.0, ...} is rejected by validate_bounds/1 (canonical?/1 requires south <= north), so handle_bounds_update/2 returns before it ever reaches the historical_loading branch — delete that branch and this test still passes, so the guard its name describes is now uncovered.

Give it valid bounds (e.g. %{north: 35.0, south: 33.0, east: -94.0, west: -98.0}) and it fails, because the branch assigns pending_bounds: map_bounds: the newest bounds win, which is what HistoricalLoader + {:process_pending_bounds} expect. Assert that instead:

new_bounds = %{north: 35.0, south: 33.0, east: -94.0, west: -98.0}
{:noreply, result} = BoundsUpdater.handle_bounds_update(new_bounds, socket)
assert result.assigns.pending_bounds == new_bounds

The deleted rejects south > north case is now only covered incidentally by this test's short-circuit.

**🔵 Suggestion — Rewritten pending-bounds test passes for the wrong reason** `%{north: 34.0, south: 40.0, ...}` is rejected by `validate_bounds/1` (`canonical?/1` requires `south <= north`), so `handle_bounds_update/2` returns before it ever reaches the `historical_loading` branch — delete that branch and this test still passes, so the guard its name describes is now uncovered. Give it valid bounds (e.g. `%{north: 35.0, south: 33.0, east: -94.0, west: -98.0}`) and it fails, because the branch assigns `pending_bounds: map_bounds`: the newest bounds win, which is what `HistoricalLoader` + `{:process_pending_bounds}` expect. Assert that instead: ```elixir new_bounds = %{north: 35.0, south: 33.0, east: -94.0, west: -98.0} {:noreply, result} = BoundsUpdater.handle_bounds_update(new_bounds, socket) assert result.assigns.pending_bounds == new_bounds ``` The deleted `rejects south > north` case is now only covered incidentally by this test's short-circuit.

skippy-bot marked this conversation as resolved

graham added 1 commit

2026-09-15 12:12:59 -05:00

fix: assert newest pending bounds during historical load

Elixir CI / Build and test (pull_request) Failing after 48s

Details

Elixir CI / Dialyzer (pull_request) Successful in 40s

Details

Elixir CI / Build and Push Docker Image (pull_request) Has been skipped

Details

skippy-bot/review Skippy review: 1 blocking finding open — see the PR thread

Details

c71d1bc893

graham force-pushed fix/m3-bounds-normalization from c71d1bc893

Elixir CI / Build and test (pull_request) Failing after 48s

Details

Elixir CI / Dialyzer (pull_request) Successful in 40s

Details

Elixir CI / Build and Push Docker Image (pull_request) Has been skipped

Details

skippy-bot/review Skippy review: 1 blocking finding open — see the PR thread

Details

to 1fab14e91a

Elixir CI / Build and test (pull_request) Failing after 46s

Details

skippy-bot/review Skippy review: clean — 1 non-blocking note open

Details

Elixir CI / Dialyzer (pull_request) Successful in 2m41s

Details

Elixir CI / Build and Push Docker Image (pull_request) Has been skipped

Details

2026-09-15 12:15:23 -05:00

Compare

skippy-bot reviewed

2026-09-15 12:15:36 -05:00

skippy-bot left a comment

🤖 Skippy PR review

2 findings — 1 blocking before merge.

Severity	Location	Issue
🟠 High	`test/aprsme_web/live/map_live/bounds_updater_test.exs:196`	Test file cannot compile: `__omp_magic/2` is undefined
🔵 Suggestion	`test/aprsme_web/live/map_live/bounds_updater_test.exs:204`	Rewritten test still does not pin the `historical_loading` branch

Resolved 1 earlier finding fixed in c71d1bc (the pending-bounds test now uses valid bounds, so it actually reaches that branch); 0 left open. Reviewed c71d1bc..head only.

_{Reviewed c71d1bc8930c. Comment skippy review to re-run.}

### 🤖 Skippy PR review **2 findings** — 1 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟠 High | `test/aprsme_web/live/map_live/bounds_updater_test.exs:196` | Test file cannot compile: `__omp_magic/2` is undefined | | 🔵 Suggestion | `test/aprsme_web/live/map_live/bounds_updater_test.exs:204` | Rewritten test still does not pin the `historical_loading` branch | Resolved 1 earlier finding fixed in c71d1bc (the pending-bounds test now uses valid bounds, so it actually reaches that branch); 0 left open. Reviewed c71d1bc..head only. <sub>Reviewed `c71d1bc8930c`. Comment `skippy review` to re-run.</sub>

test/aprsme_web/live/map_live/bounds_updater_test.exs

					
				@ -173,2 +194,3 @@

				    test "does not overwrite pending bounds while historical loading is in flight" do

				    test "newest bounds win while historical loading is in flight" do

				      socket = build_socket(%{map_bounds: @bounds, historical_loading: true, pending_bounds: @bounds})

				      new_bounds = %{north: 35.0, south: 33.0, east: -94.0, west: -98.0}

🟠 High — Test file cannot compile: __omp_magic/2 is undefined

The literal source at this line is __omp_magic("", "{north: 35.0, south: 33.0, east: -94.0, west: -98.0}") (checked against the raw blob at c71d1bc, not just the rendered diff), and __omp_magic is defined nowhere: no def, no import, no alias, no dep. Compiled Elixir resolves that call and fails, so this file does not build and mix test / precommit cannot run at all — the "full suite 2235 passed" verification in the description is not reproducible from this commit. It also looks like a mangled literal: the map lost its % sigil and ended up as a string. Fix and re-run:

new_bounds = %{north: 35.0, south: 33.0, east: -94.0, west: -98.0}

**🟠 High — Test file cannot compile: `__omp_magic/2` is undefined** The literal source at this line is `__omp_magic("", "{north: 35.0, south: 33.0, east: -94.0, west: -98.0}")` (checked against the raw blob at c71d1bc, not just the rendered diff), and `__omp_magic` is defined nowhere: no `def`, no import, no alias, no dep. Compiled Elixir resolves that call and fails, so this file does not build and `mix test` / `precommit` cannot run at all — the "full suite 2235 passed" verification in the description is not reproducible from this commit. It also looks like a mangled literal: the map lost its `%` sigil and ended up as a string. Fix and re-run: ```elixir new_bounds = %{north: 35.0, south: 33.0, east: -94.0, west: -98.0} ```

skippy-bot marked this conversation as resolved

test/aprsme_web/live/map_live/bounds_updater_test.exs

					
				@ -180,0 +201,4 @@

				      # {:process_pending_bounds} reload picks up the newest bounds when it

				      # finishes.

				      assert result.assigns.pending_bounds == new_bounds

				      assert result.assigns.map_bounds == @bounds

🔵 Suggestion — Rewritten test still does not pin the historical_loading branch

The invalid-payload half of my earlier comment is fixed, but this test would still pass with the historical_loading clause in handle_bounds_update/2 deleted: the fallback schedule_bounds_update/2 also assigns pending_bounds: map_bounds (bounds_updater.ex:139) and leaves map_bounds untouched, so both assertions hold either way. The only observable difference is the debounce timer — nil on this path, a Process.send_after ref on the fallback, which also calls HistoricalLoader.cancel_pending_loads/1 and kills the in-flight batches this branch exists to protect. Pin it with one line:

assert result.assigns.bounds_update_timer == nil

**🔵 Suggestion — Rewritten test still does not pin the `historical_loading` branch** The invalid-payload half of my earlier comment is fixed, but this test would still pass with the `historical_loading` clause in `handle_bounds_update/2` deleted: the fallback `schedule_bounds_update/2` also assigns `pending_bounds: map_bounds` (`bounds_updater.ex:139`) and leaves `map_bounds` untouched, so both assertions hold either way. The only observable difference is the debounce timer — `nil` on this path, a `Process.send_after` ref on the fallback, which also calls `HistoricalLoader.cancel_pending_loads/1` and kills the in-flight batches this branch exists to protect. Pin it with one line: ```elixir assert result.assigns.bounds_update_timer == nil ```

skippy-bot marked this conversation as resolved

skippy-bot commented

2026-09-15 12:17:16 -05:00

First-time contributor

Resolved 1 earlier finding — the pending-bounds test now uses valid bounds and asserts newest-wins (bounds_updater_test.exs:194), so the branch it was meant to cover is exercised again.

Force-push note: c71d1bc8 was rewritten (it carried a mangled __omp_magic(...) literal), so the incremental diff was unavailable and I re-read the whole PR diff. The delta since the last reviewed head is that one test line — nothing new.

**Resolved 1 earlier finding** — the pending-bounds test now uses valid bounds and asserts newest-wins (`bounds_updater_test.exs:194`), so the branch it was meant to cover is exercised again. Force-push note: `c71d1bc8` was rewritten (it carried a mangled `__omp_magic(...)` literal), so the incremental diff was unavailable and I re-read the whole PR diff. The delta since the last reviewed head is that one test line — nothing new.

skippy-bot commented

2026-09-15 12:17:36 -05:00

First-time contributor

Also closing 4716 (high — test file does not compile): at 1fab14e9 that line reads new_bounds = %{north: 35.0, south: 33.0, east: -94.0, west: -98.0} and __omp_magic appears nowhere in the tree — c71d1bc8..1fab14e9 is exactly that one line, so the file builds again. That commit also answers the earlier pending-bounds finding (valid bounds, newest-wins assertion), so it is resolved too.

4717 stays open (suggestion, not blocking): the test still passes if the historical_loading clause is deleted, since the debounce fallback also assigns pending_bounds and leaves map_bounds alone. assert result.assigns.bounds_update_timer == nil pins the branch.

Also closing **4716** (high — test file does not compile): at `1fab14e9` that line reads `new_bounds = %{north: 35.0, south: 33.0, east: -94.0, west: -98.0}` and `__omp_magic` appears nowhere in the tree — `c71d1bc8..1fab14e9` is exactly that one line, so the file builds again. That commit also answers the earlier pending-bounds finding (valid bounds, newest-wins assertion), so it is resolved too. **4717 stays open** (suggestion, not blocking): the test still passes if the `historical_loading` clause is deleted, since the debounce fallback also assigns `pending_bounds` and leaves `map_bounds` alone. `assert result.assigns.bounds_update_timer == nil` pins the branch.

graham added 1 commit

2026-09-15 16:54:38 -05:00

test: pin the historical-loading bounds branch via its timer

skippy-bot/review Skippy review: clean — no open findings

Details

Elixir CI / Build and test (pull_request) Has been cancelled

Details

Elixir CI / Dialyzer (pull_request) Has been cancelled

Details

Elixir CI / Build and Push Docker Image (pull_request) Has been cancelled

Details

d8652ba814

The fallback schedule_bounds_update also assigns pending_bounds and
leaves map_bounds alone, so deleting the historical_loading clause kept
both existing assertions green. Assert no debounce timer is scheduled
mid-load — the fallback would arm one and cancel the in-flight
historical batches this branch exists to protect.

skippy-bot commented

2026-09-15 16:55:07 -05:00

First-time contributor

Resolved 1 of 1 earlier finding. d8652ba adds assert result.assigns.bounds_update_timer == nil (bounds_updater_test.exs:207), which is exactly the pin that was missing: build_socket/1 defaults the key to nil, the historical_loading clause never assigns it, and the debounce fallback would assign a Process.send_after ref, so deleting that branch now fails the test. 0 still open.

Nothing new in 1fab14e9..d8652ba (test-only assertion: a comment plus the assert).

**Resolved 1 of 1 earlier finding.** `d8652ba` adds `assert result.assigns.bounds_update_timer == nil` (`bounds_updater_test.exs:207`), which is exactly the pin that was missing: `build_socket/1` defaults the key to `nil`, the `historical_loading` clause never assigns it, and the debounce fallback would assign a `Process.send_after` ref, so deleting that branch now fails the test. 0 still open. Nothing new in `1fab14e9..d8652ba` (test-only assertion: a comment plus the assert).

graham added 1 commit

2026-09-15 16:58:32 -05:00

refactor: group validate_bounds clauses

skippy-bot/review Skippy review: clean — no open findings

Details

Elixir CI / Build and test (pull_request) Successful in 1m0s

Details

Elixir CI / Dialyzer (pull_request) Successful in 2m42s

Details

Elixir CI / Build and Push Docker Image (pull_request) Has been skipped

Details

6ffbea54e7

The normalize helpers landed between the two validate_bounds/1
clauses, tripping the ungrouped-clauses compiler warning. Move them
below the final clause; no behavior change.

graham merged commit a27895f6b3 into main

2026-09-15 17:02:58 -05:00

graham deleted branch fix/m3-bounds-normalization

2026-09-15 17:02:58 -05:00

graham referenced this pull request from a commit

2026-09-15 17:02:59 -05:00

fix: normalize client and server map bounds (#34)

Sign in to join this conversation.

No reviewers

No labels

No milestone

No project