feat: consistent, bug-free, higher-performance UI/UX #42

graham · 2026-10-18T17:35:09Z

graham commented

2026-10-18 17:35:09 +00:00

What changed

Cross-cutting UI/UX consistency + performance pass, verified live in the browser and against the full suite.

Server / LiveView

core_components: heroicons loaded once at compile time (was File.read per icon render); caller class now merges into SVGs that already declare one; new <.page_container> gives uniform gutters + max-width; dark-safe flash palette; flash/menu z-index raised above Leaflet.
packets/index: aria-live "Showing N packets" throttled to 5 s (fix: System.monotonic_time(:second) can be negative, which meant the announced count never refreshed on some hosts).
packets/callsign: deterministic row id via stable_packet_key — morphdom was destroying and recreating rows on every flush.
weather/callsign: append-merge history — refetches only samples newer than the newest held row (was rescanning + re-encoding up to 500 rows per broadcast).
map_live: PacketBatcher bounded to 5 000 senders + 500-entry prune margin; DisplayManager/index/subscriptions cleanups.
templates: page_container everywhere; divider tokens standardized to divide-gray-200 / dark:divide-white/10.
user_settings: import ... only: list gains page_container: 1 — a later limited import replaces the earlier full import, so it was actually missing.

Frontend

map.ts: popups only open when data.popup.openPopup === true; hover-open debounced; historical eviction queue.
trail_manager: tryExtendTrail appends geometry instead of rebuilding the polyline per packet.
weather_charts: unique Chart.js instance ids; destroy on hook teardown.
info_map hook: container-scoped DOM + setPopupContent for live text.
error_boundary: bundle-aware retry for map/chart bundles.
mobile.css: .aprs-map full-bleed under mobile nav.
app.css: @source entries so Tailwind emits the dark-mode/component utilities the new classes need.
vendor: dead chart-bundle variants dropped (adapter-wrapper, bundle-complete, combined, with-adapter, date-adapter-fixed, vendor-bundle); chart-bundle-src aligned.

Verified

mix test: 2286 passed · mix credo --strict: clean · mix dialyzer: clean
Browser smoke on /, /packets, /packets/:callsign, /packets/bad, /about, /status, /api, /info/:callsign, /weather/:callsign — zero JS errors; aria-live region announcing correctly.

Built on main @ 181e414f (includes the nix devshell fix).

## What changed Cross-cutting UI/UX consistency + performance pass, verified live in the browser and against the full suite. ### Server / LiveView - **core_components**: heroicons loaded once at compile time (was `File.read` per icon render); caller class now merges into SVGs that already declare one; new `<.page_container>` gives uniform gutters + max-width; dark-safe flash palette; flash/menu z-index raised above Leaflet. - **packets/index**: `aria-live` "Showing N packets" throttled to 5 s (fix: `System.monotonic_time(:second)` can be negative, which meant the announced count *never* refreshed on some hosts). - **packets/callsign**: deterministic row id via `stable_packet_key` — morphdom was destroying and recreating rows on every flush. - **weather/callsign**: append-merge history — refetches only samples newer than the newest held row (was rescanning + re-encoding up to 500 rows per broadcast). - **map_live**: `PacketBatcher` bounded to 5 000 senders + 500-entry prune margin; `DisplayManager`/`index`/`subscriptions` cleanups. - **templates**: `page_container` everywhere; divider tokens standardized to `divide-gray-200` / `dark:divide-white/10`. - **user_settings**: `import ... only:` list gains `page_container: 1` — a later limited import replaces the earlier full import, so it was actually missing. ### Frontend - **map.ts**: popups only open when `data.popup.openPopup === true`; hover-open debounced; historical eviction queue. - **trail_manager**: `tryExtendTrail` appends geometry instead of rebuilding the polyline per packet. - **weather_charts**: unique Chart.js instance ids; destroy on hook teardown. - **info_map** hook: container-scoped DOM + `setPopupContent` for live text. - **error_boundary**: bundle-aware retry for map/chart bundles. - **mobile.css**: `.aprs-map` full-bleed under mobile nav. - **app.css**: `@source` entries so Tailwind emits the dark-mode/component utilities the new classes need. - **vendor**: dead chart-bundle variants dropped (`adapter-wrapper`, `bundle-complete`, `combined`, `with-adapter`, `date-adapter-fixed`, `vendor-bundle`); `chart-bundle-src` aligned. ### Verified - `mix test`: **2286 passed** · `mix credo --strict`: **clean** · `mix dialyzer`: **clean** - Browser smoke on `/`, `/packets`, `/packets/:callsign`, `/packets/bad`, `/about`, `/status`, `/api`, `/info/:callsign`, `/weather/:callsign` — zero JS errors; aria-live region announcing correctly. Built on main @ 181e414f (includes the nix devshell fix).

graham added 1 commit

2026-10-18 17:35:09 +00:00

feat: consistent, bug-free, higher-performance UI/UX across the app

Elixir CI / Dialyzer (pull_request) Successful in 1m14s

Details

Elixir CI / Build and test (pull_request) Failing after 2m8s

Details

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

Details

skippy-bot/review Skippy review: 4 blocking findings open — see the PR thread

Details

5b68791fd5

Server:
- core_components: load vendored heroicons once at compile time instead of
  File.read per icon render; merge caller class into SVGs that already declare
  one; add <.page_container> for uniform gutters/max-width; dark-safe flash
  palette; raise flash/menu z-index above Leaflet; mark theme controls with
  data-theme-choice so the theme JS can track the active choice
- packets/index: throttle the aria-live 'Showing N packets' announcement to
  5s so the 150ms flush doesn't bury screen readers (fix negative
  System.monotonic_time(:second) never refreshing the announced count)
- packets/callsign: deterministic row id (stable_packet_key) so morphdom
  stops destroying rows on every flush; dual-key access normalized once
- weather/callsign: append-merge weather history — refetch only samples
  newer than the newest held row instead of rescanning + re-encoding up to
  500 rows per broadcast
- map_live: bounded PacketBatcher (5,000 senders, 500-entry prune margin),
  DisplayManager updates, index/subscriptions cleanups
- templates: page_container everywhere; divide-gray-200/dark:divide-white/10
  standardizes the ad-hoc divide-gray-100/300 + divide-white/5/700 mix
- user_settings: import page_container alongside the :only list (a later
  limited import replaces the earlier full CoreComponents import)

Frontend (assets/js, css):
- map.ts: popups only open when data.popup.openPopup === true; hover open
  debounced; historical eviction queue
- trail_manager: tryExtendTrail appends to existing polyline geometry
  instead of rebuilding per packet
- weather_charts: unique Chart.js instance ids via counter; destroy on
  hook teardown
- info_map hook: container-scoped DOM, setPopupContent for live text
- error_boundary: bundle-aware retry for map/chart bundles
- mobile.css: .aprs-map sized full-bleed under the mobile nav
- app.css: @source entries so Tailwind emits dark-mode and component
  utilities used by the new flash/container classes
- chart-bundle-src: vendored chart bundle aligned with wrappers
- drop dead vendored chart bundle variants (adapter-wrapper, bundle-
  complete, combined, with-adapter, date-adapter-fixed, vendor-bundle)

Tests: 2286 passed; credo --strict clean; dialyzer clean.

skippy-bot reviewed

2026-10-18 17:39:27 +00:00

skippy-bot left a comment

🤖 Skippy PR review

4 findings — 4 blocking before merge.

Severity	Location	Issue
🟠 High	`lib/aprsme_web/components/core_components.ex:37`	Icons are read at compile time from cwd, and the release image compiles before priv is copied
🟠 High	`assets/js/features/trail_manager.ts:597`	Trail fast path appends to the first segment, not the one it validated
🟠 High	`assets/js/features/trail_manager.ts:236`	Extending an already-removed polyline permanently kills a station's trail
🟡 Warning	`assets/js/map.ts:2081`	Eviction queue is append-only and never pruned on marker removal

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

### 🤖 Skippy PR review **4 findings** — 4 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟠 High | `lib/aprsme_web/components/core_components.ex:37` | Icons are read at compile time from cwd, and the release image compiles before priv is copied | | 🟠 High | `assets/js/features/trail_manager.ts:597` | Trail fast path appends to the first segment, not the one it validated | | 🟠 High | `assets/js/features/trail_manager.ts:236` | Extending an already-removed polyline permanently kills a station's trail | | 🟡 Warning | `assets/js/map.ts:2081` | Eviction queue is append-only and never pruned on marker removal | <sub>Reviewed `5b68791fd5d3`. Comment `skippy review` to re-run.</sub>

assets/js/features/trail_manager.ts

					
				@ -231,0 +233,4 @@

				      if (

				        !isDuplicate &&

				        trailState.trail &&

				        this.tryExtendTrail(trailState)

🟠 High — Extending an already-removed polyline permanently kills a station's trail

updateTrailVisualization removes the current polyline at the top and then returns without clearing trailState.trail whenever it decides not to redraw (positions.length < 2, totalDistance < this.minMovementThreshold at line 446, or no drawable segment). This fast path treats that non-nil field as "the trail is on the map": it appends to the detached polyline and returns, so scheduleTrailUpdate never runs again for that callsign. Concretely, a station whose trail was drawn, then parked (or dropped to one retained position), and then moved, keeps extending a polyline that is no longer on the layer - the trail never reappears for the rest of the session. Either verify membership (this.trailLayer.hasLayer(trail)) or set trailState.trail = undefined where the old line is removed.

**🟠 High — Extending an already-removed polyline permanently kills a station's trail** `updateTrailVisualization` removes the current polyline at the top and then returns without clearing `trailState.trail` whenever it decides not to redraw (`positions.length < 2`, `totalDistance < this.minMovementThreshold` at line 446, or no drawable segment). This fast path treats that non-nil field as "the trail is on the map": it appends to the detached polyline and returns, so `scheduleTrailUpdate` never runs again for that callsign. Concretely, a station whose trail was drawn, then parked (or dropped to one retained position), and then moved, keeps extending a polyline that is no longer on the layer - the trail never reappears for the rest of the session. Either verify membership (`this.trailLayer.hasLayer(trail)`) or set `trailState.trail = undefined` where the old line is removed.

skippy-bot marked this conversation as resolved

assets/js/features/trail_manager.ts Outdated

					
				@ -549,0 +594,4 @@

				    const unwrappedLng =

				      last.lng + Math.round((end.lng - last.lng) / 360) * 360;

				    trail.addLatLng([last.lat, unwrappedLng]);

🟠 High — Trail fast path appends to the first segment, not the one it validated

The endpoint check reads the LAST ring (latlngs[latlngs.length - 1]) but the append calls one-argument addLatLng/1, which Leaflet routes to the default shape - _defaultShape/0 is _latlngs[0] once the polyline is multi-segment (addLatLng:function(t,e){return e=e||this._defaultShape(),...} in the vendored assets/vendor/js/leaflet.js). updateTrailVisualization builds L.polyline(drawableSegments, ...), i.e. one ring per >100 km hop, so on any trail containing a hop (a 0,0 or otherwise bad fix, an igate jump) the new point lands on the trail's FIRST segment while the geometry was validated against the last: the map draws a long line from the oldest segment's endpoint to the new position and the final segment stops growing. Pass the ring explicitly: trail.addLatLng([last.lat, unwrappedLng], lastSeg).

**🟠 High — Trail fast path appends to the first segment, not the one it validated** The endpoint check reads the LAST ring (`latlngs[latlngs.length - 1]`) but the append calls one-argument `addLatLng/1`, which Leaflet routes to the default shape - `_defaultShape/0` is `_latlngs[0]` once the polyline is multi-segment (`addLatLng:function(t,e){return e=e||this._defaultShape(),...}` in the vendored `assets/vendor/js/leaflet.js`). `updateTrailVisualization` builds `L.polyline(drawableSegments, ...)`, i.e. one ring per >100 km hop, so on any trail containing a hop (a 0,0 or otherwise bad fix, an igate jump) the new point lands on the trail's FIRST segment while the geometry was validated against the last: the map draws a long line from the oldest segment's endpoint to the new position and the final segment stops growing. Pass the ring explicitly: `trail.addLatLng([last.lat, unwrappedLng], lastSeg)`.

skippy-bot marked this conversation as resolved

assets/js/map.ts

					
				@ -2072,0 +2078,4 @@

				    // remove the OLDEST entries, not arbitrary Map-order ones. Stale queue

				    // entries (already removed) are skipped cheaply at eviction time.

				    if (data.historical && !data.is_most_recent_for_callsign) {

				      (self.historicalEvictionQueue ||= []).push(data.id);

🟡 Warning — Eviction queue is append-only and never pruned on marker removal

historicalEvictionQueue grows monotonically: every historical add pushes an id, each re-add pushes the same id again, and no removal path (removeMarkerWithoutTrail, the expired-marker sweep, a viewport reload) deletes it. The array therefore tracks all historical adds ever seen rather than the markers actually held, which is the opposite of the bounded memory this change exists for, and the eviction loop pays queue.shift() (O(n)) per removal exactly when the array is largest. Drop the id in removeMarkerWithoutTrail, or keep a head offset and compact, so the queue mirrors markers.

**🟡 Warning — Eviction queue is append-only and never pruned on marker removal** `historicalEvictionQueue` grows monotonically: every historical add pushes an id, each re-add pushes the same id again, and no removal path (`removeMarkerWithoutTrail`, the expired-marker sweep, a viewport reload) deletes it. The array therefore tracks all historical adds ever seen rather than the markers actually held, which is the opposite of the bounded memory this change exists for, and the eviction loop pays `queue.shift()` (O(n)) per removal exactly when the array is largest. Drop the id in `removeMarkerWithoutTrail`, or keep a head offset and compact, so the queue mirrors `markers`.

skippy-bot marked this conversation as resolved

lib/aprsme_web/components/core_components.ex Outdated

					
				@ -33,0 +34,4 @@

				  # flash/error/title, so a File.read per call was disk I/O in the render path.

				  icons =

				    for style <- ~w(outline solid), into: %{} do

				      dir = Path.join([File.cwd!(), "priv", "heroicons", "24", style])

🟠 High — Icons are read at compile time from cwd, and the release image compiles before priv is copied

File.cwd!() runs when this module compiles, so @icons is whatever priv/heroicons happened to exist at compile time. The production image has nothing there: Dockerfile copies config then lib and runs mix compile (lines 32-34) before COPY priv priv (line 38), so Path.wildcard/1 returns [], @icons settles as %{"outline" => %{}, "solid" => %{}}, and mix release does not recompile afterwards. CI tests run with priv present, so nothing goes red - every deploy just ships  for the flash info/error icon, the flash close x-mark, and the layout spinner (layouts/app.html.heex:60). Fix by reading the icons at runtime from :code.priv_dir(:aprsme) (memoize in :persistent_term if the per-render I/O is the concern), or at minimum move COPY priv priv above RUN mix compile.

**🟠 High — Icons are read at compile time from cwd, and the release image compiles before priv is copied** `File.cwd!()` runs when this module compiles, so `@icons` is whatever `priv/heroicons` happened to exist at *compile* time. The production image has nothing there: `Dockerfile` copies `config` then `lib` and runs `mix compile` (lines 32-34) **before** `COPY priv priv` (line 38), so `Path.wildcard/1` returns `[]`, `@icons` settles as `%{"outline" => %{}, "solid" => %{}}`, and `mix release` does not recompile afterwards. CI tests run with `priv` present, so nothing goes red - every deploy just ships `` for the flash info/error icon, the flash close `x-mark`, and the layout spinner (`layouts/app.html.heex:60`). Fix by reading the icons at runtime from `:code.priv_dir(:aprsme)` (memoize in `:persistent_term` if the per-render I/O is the concern), or at minimum move `COPY priv priv` above `RUN mix compile`.

skippy-bot marked this conversation as resolved

graham added 1 commit

2026-10-18 17:39:57 +00:00

chore: bump bandit and decimal to clear hex.audit advisories

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

Details

skippy-bot/review Skippy review: 4 blocking findings open — see the PR thread

Details

Elixir CI / Dialyzer (pull_request) Successful in 7m52s

Details

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

Details

925d5b30fb

CI's hex.audit gate is failing on three advisories against the locked
versions — none of them reachable through this diff, all surfaced by a
newly published advisory feed:

- bandit 1.12.5 → 1.13.1 (EEF-CVE-2026-105659 HIGH, EEF-CVE-2026-105660)
- decimal 3.1.1 → 3.2.0 (EEF-CVE-2026-97853)
- thousand_island 1.5.0 → 1.6.0 (transitive via bandit)

mix test: 2286 passed.

graham referenced this pull request from a commit

2026-10-18 21:08:36 +00:00

fix: address skippy review findings on #42

graham added 1 commit

2026-10-18 21:08:36 +00:00

fix: address skippy review findings on #42

Elixir CI / Dialyzer (pull_request) Successful in 1m3s

Details

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

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

8ff6db3c6c

- core_components: icons memoized in :persistent_term at runtime instead of
  a compile-time File.cwd!() read — the release Dockerfile compiles before
  'COPY priv', so compile-time loading built an image with no icons
- trail_manager.tryExtendTrail: append to the validated LAST segment
  (Leaflet's addLatLng targets the first), and require the polyline to
  still be on the layer — an off-layer line would absorb every future
  append and the station's visible trail would never re-draw
- map.ts: historicalEvictionQueue compacted past 20k entries — it's
  append-only and marker removals outside the eviction path left stale
  ids that grew the queue unboundedly

skippy-bot reviewed

2026-10-18 21:11:09 +00:00

skippy-bot left a comment

🤖 Skippy PR review

1 finding — 1 blocking before merge.

Severity	Location	Issue
🟡 Warning	`assets/js/map.ts:2106`	Eviction only sees queued markers, but demoted live markers are eligible and never queued

Resolved 4 earlier findings fixed in 8ff6db3c: icons now memoize from Application.app_dir at runtime, the trail fast path checks hasLayer and appends to the validated ring, and the eviction queue is compacted past 20k entries.

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

### 🤖 Skippy PR review **1 finding** — 1 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟡 Warning | `assets/js/map.ts:2106` | Eviction only sees queued markers, but demoted live markers are eligible and never queued | Resolved 4 earlier findings fixed in 8ff6db3c: icons now memoize from Application.app_dir at runtime, the trail fast path checks hasLayer and appends to the validated ring, and the eviction queue is compacted past 20k entries. <sub>Reviewed `8ff6db3c6c1b`. Comment `skippy review` to re-run.</sub>

assets/js/map.ts

					
				@ -2079,0 +2103,4 @@

				      while (removed < 1000 && queue.length > 0 && self.markers.size > 9000) {

				        const id = queue.shift()!;

				        const state = self.markerStates.get(id);

				        if (state?.historical && !state.is_most_recent_for_callsign) {

🟡 Warning — Eviction only sees queued markers, but demoted live markers are eligible and never queued

This loop now takes ids solely from historicalEvictionQueue, but the queue is only fed at addMarker for markers that ARRIVE historical (data.historical && !data.is_most_recent_for_callsign). Markers that arrive live and are later demoted are flagged historical in place - existingState.historical = true; existingState.is_most_recent_for_callsign = false at lines 1188-1189, 1294-1295 and 1916-1917 - and never pushed, so they satisfy this loop's state.historical && !state.is_most_recent_for_callsign test yet are unreachable through the queue. Base main scanned markerStates for exactly that predicate, so the rewrite silently lost a class of evictable markers: on a map left open on a busy area (24 h trail window), every live packet demotes the previous marker per callsign, markers.size climbs past 10 000, the loop drains the queued historical ids, and then queue.length === 0 leaves self.markers.size > 9000 while the cap does nothing for the rest of the session. Fix at the demotion sites: push the demoted id onto (self.historicalEvictionQueue ||= []) (the compaction at line 2097 dedupes), or fall back to a markerStates scan when the queue empties.

**🟡 Warning — Eviction only sees queued markers, but demoted live markers are eligible and never queued** This loop now takes ids solely from `historicalEvictionQueue`, but the queue is only fed at `addMarker` for markers that ARRIVE historical (`data.historical && !data.is_most_recent_for_callsign`). Markers that arrive live and are later demoted are flagged historical in place - `existingState.historical = true; existingState.is_most_recent_for_callsign = false` at lines 1188-1189, 1294-1295 and 1916-1917 - and never pushed, so they satisfy this loop's `state.historical && !state.is_most_recent_for_callsign` test yet are unreachable through the queue. Base main scanned `markerStates` for exactly that predicate, so the rewrite silently lost a class of evictable markers: on a map left open on a busy area (24 h trail window), every live packet demotes the previous marker per callsign, `markers.size` climbs past 10 000, the loop drains the queued historical ids, and then `queue.length === 0` leaves `self.markers.size > 9000` while the cap does nothing for the rest of the session. Fix at the demotion sites: push the demoted `id` onto `(self.historicalEvictionQueue ||= [])` (the compaction at line 2097 dedupes), or fall back to a `markerStates` scan when the queue empties.

skippy-bot marked this conversation as resolved

graham added 1 commit

2026-10-18 21:12:30 +00:00

fix: enqueue live markers demoted to historical into the eviction queue

Elixir CI / Dialyzer (pull_request) Successful in 1m1s

Details

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

Details

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

Details

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

Details

d457f2e4d1

The cap-based eviction only drained ids pushed at insert time; markers
that started live and were later demoted were eviction-eligible but never
queued, so they accumulated past the cap. Push the id at all three
demotion sites (incoming supersedes, viewport conversion, callsign-group
replace).

skippy-bot commented

2026-10-18 21:13:55 +00:00

First-time contributor

Resolved 1 of 1 earlier findings — all three demotion sites (new_packet, batched new_packets, and the addMarker most-recent conversion) now push the demoted marker id onto historicalEvictionQueue, so the 10 000-marker cap can reach markers that arrived live and were demoted later.

Nothing new in 8ff6db3c..d457f2e4.

Resolved 1 of 1 earlier findings — all three demotion sites (`new_packet`, batched `new_packets`, and the `addMarker` most-recent conversion) now push the demoted marker id onto `historicalEvictionQueue`, so the 10 000-marker cap can reach markers that arrived live and were demoted later. Nothing new in `8ff6db3c`..`d457f2e4`.

graham merged commit 8930623178 into main

2026-10-18 21:25:29 +00:00

graham deleted branch fix/ui-ux-audit

2026-10-18 21:25:29 +00:00

graham referenced this pull request from a commit

2026-10-18 21:25:31 +00:00

feat: consistent, bug-free, higher-performance UI/UX (#42)

Sign in to join this conversation.

No reviewers