Fix LiveView hook lifecycle, chart repaints, and key handling #22

Merged
graham merged 2 commits from fix/liveview-hooks into main 2026-09-22 12:24:56 -05:00
Owner

Audit LiveView/TS group (part 1).

  • lazyHook reconnected() re-ran mounted() on every already-loaded hook — Leaflet double-init, duplicate contacts fetch, second WebGL canvas. Guarded on !this._impl.
  • Skew-T chart never repainted (phx-update=ignore + mounted-only hook) — container now keyed on the rendered svg.
  • ElevationProfile hook gains updated() — contact page profile re-renders when k-factor/sounding data lands.
  • New NavKeys hook replaces phx-window-keydown on /map — ignores arrows in inputs/selects/Leaflet; map hook stopPropagations.
  • observed_at sorts chronologically (DateTime sorter); restore_saved capped at @max_saved_points; coverage banner copy fixed; sounding-fetch failure logged.

Note: fix/liveview-guards uses the NavKeys hook introduced here — merge this one first (or rebase).

Verified: 298 targeted tests + tsc + assets.build green.

Audit LiveView/TS group (part 1). - lazyHook reconnected() re-ran mounted() on every already-loaded hook — Leaflet double-init, duplicate contacts fetch, second WebGL canvas. Guarded on !this._impl. - Skew-T chart never repainted (phx-update=ignore + mounted-only hook) — container now keyed on the rendered svg. - ElevationProfile hook gains updated() — contact page profile re-renders when k-factor/sounding data lands. - New NavKeys hook replaces phx-window-keydown on /map — ignores arrows in inputs/selects/Leaflet; map hook stopPropagations. - observed_at sorts chronologically (DateTime sorter); restore_saved capped at @max_saved_points; coverage banner copy fixed; sounding-fetch failure logged. Note: fix/liveview-guards uses the NavKeys hook introduced here — merge this one first (or rebase). Verified: 298 targeted tests + tsc + assets.build green.
Fix LiveView hook lifecycle, chart repaints, and key handling
Some checks failed
skippy-bot/review Skippy review: 2 blocking findings open — see the PR thread
088fb5b51e
- lazyHook reconnected() no longer re-runs mounted() on hooks that are
  already loaded (Leaflet double-init, duplicate fetches, second WebGL
  canvas on every reconnect)
- Skew-T chart repaints on hour/station change (container keyed on the
  rendered svg so the hook remounts)
- ElevationProfile hook gains updated() so the contact profile canvas
  re-renders when HRRR k-factor/sounding data lands after first paint
- New NavKeys hook replaces phx-window-keydown on /map: ignores arrows
  inside inputs/selects and the Leaflet container; map hook also
  stopPropagation()s its arrow handling
- Contact observation/sounding tables sort observed_at chronologically
  (DateTime sorter) instead of structurally
- restore_saved caps the localStorage point list at @max_saved_points
- /map coverage banner copy matches the actual HRRR+HRDPS coverage
- Log the sounding-fetch enqueue failure instead of dropping it
skippy-bot left a comment

🤖 Skippy PR review

3 findings — 2 blocking before merge.

Severity Location Issue
🟠 High test/microwaveprop_web/live/map_live_test.exs:1218 Chip running-state test stubs a table the chip does not read
🟡 Warning lib/microwaveprop_web/live/map_live.ex:385 Saved-point cap drops pins 101+ out of localStorage on the next save
🔵 Suggestion assets/js/nav_keys_hook.ts:12 Ignoring arrows on a/button kills hour stepping after any click

Reviewed 088fb5b51e2d. Comment skippy review to re-run.

### 🤖 Skippy PR review **3 findings** — 2 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟠 High | `test/microwaveprop_web/live/map_live_test.exs:1218` | Chip running-state test stubs a table the chip does not read | | 🟡 Warning | `lib/microwaveprop_web/live/map_live.ex:385` | Saved-point cap drops pins 101+ out of localStorage on the next save | | 🔵 Suggestion | `assets/js/nav_keys_hook.ts:12` | Ignoring arrows on a/button kills hour stepping after any click | <sub>Reviewed `088fb5b51e2d`. Comment `skippy review` to re-run.</sub> <!-- skippy-pr-review -->
@ -0,0 +9,4 @@
// Keys aimed at a control, an editable region, a link/button, or the
// map (which owns its own arrow-key contract) are not navigation.
const IGNORED_TARGET =
First-time contributor

🔵 Suggestion — Ignoring arrows on a/button kills hour stepping after any click

a and button have no arrow-key contract of their own, so listing them in IGNORED_TARGET means arrow navigation stops for as long as focus sits on a control. Concrete path: click a band-rail cell (a tabindex="-1" button, map_live.ex:1693) or any link, then press arrows, and nav_key is never pushed, so the forecast hour and band no longer step (they did with phx-window-keydown). Keep the guard for editable targets and .leaflet-container (the map owns arrows while focused) and drop a,button unless a specific control consumes arrows.

**🔵 Suggestion — Ignoring arrows on a/button kills hour stepping after any click** `a` and `button` have no arrow-key contract of their own, so listing them in `IGNORED_TARGET` means arrow navigation stops for as long as focus sits on a control. Concrete path: click a band-rail cell (a `tabindex="-1"` button, `map_live.ex:1693`) or any link, then press arrows, and `nav_key` is never pushed, so the forecast hour and band no longer step (they did with `phx-window-keydown`). Keep the guard for editable targets and `.leaflet-container` (the map owns arrows while focused) and drop `a,button` unless a specific control consumes arrows.
skippy-bot marked this conversation as resolved
@ -377,6 +382,7 @@ defmodule MicrowavepropWeb.MapLive do
else
restored =
points
|> Enum.take(@max_saved_points)
First-time contributor

🟡 Warning — Saved-point cap drops pins 101+ out of localStorage on the next save

Enum.take/2 caps only the restore path: a stored list longer than 100 is restored as its first 100, while localStorage still holds all of it. The next save_point/remove_saved calls push_saved_points/1, which mirrors the whole (now 101-entry) server list back through the saved_points event, so the original entries 101+ are overwritten out of localStorage and are unrecoverable for a signed-out visitor. Cap on the write side (pin_locally/3) or trim inside push_saved_points/1 so the mirror and the server agree. Minor, same line: the cap runs before restore_point/1 validation, so 100 malformed entries can crowd out valid ones.

**🟡 Warning — Saved-point cap drops pins 101+ out of localStorage on the next save** `Enum.take/2` caps only the restore path: a stored list longer than 100 is restored as its first 100, while localStorage still holds all of it. The next `save_point`/`remove_saved` calls `push_saved_points/1`, which mirrors the whole (now 101-entry) server list back through the `saved_points` event, so the original entries 101+ are overwritten out of localStorage and are unrecoverable for a signed-out visitor. Cap on the write side (`pin_locally/3`) or trim inside `push_saved_points/1` so the mirror and the server agree. Minor, same line: the cap runs before `restore_point/1` validation, so 100 malformed entries can crowd out valid ones.
skippy-bot marked this conversation as resolved
@ -1219,3 +1218,1 @@
queue: "propagation",
worker: "Microwaveprop.Workers.PropagationGridWorker",
args: %{},
struct!(Microwaveprop.Propagation.GridTask, %{
First-time contributor

🟠 High — Chip running-state test stubs a table the chip does not read

PipelineStatus.running_workers/0 (pipeline_status.ex:99-107, stated in its moduledoc) queries state == "executing" rows in oban_jobs; grid_tasks only feeds the freshness path (Propagation.latest_chain_completion_time/0). A grid_tasks row with status: "running" therefore cannot make PipelineStatus.current/0 return :running, so both assertions below ("Updating propagation", data-pipeline-state="running") fail no matter what this row says. The Oban.Job stub this replaces was the one that worked (see test/microwaveprop/propagation/pipeline_status_test.exs:44-53, which still stubs an executing job for exactly this state). Keep the oban_jobs insert here, or move PipelineStatus to grid_tasks in the same PR if that is the intended change.

**🟠 High — Chip running-state test stubs a table the chip does not read** `PipelineStatus.running_workers/0` (`pipeline_status.ex:99-107`, stated in its moduledoc) queries `state == "executing"` rows in **oban_jobs**; `grid_tasks` only feeds the freshness path (`Propagation.latest_chain_completion_time/0`). A `grid_tasks` row with `status: "running"` therefore cannot make `PipelineStatus.current/0` return `:running`, so both assertions below (`"Updating propagation"`, `data-pipeline-state="running"`) fail no matter what this row says. The `Oban.Job` stub this replaces was the one that worked (see `test/microwaveprop/propagation/pipeline_status_test.exs:44-53`, which still stubs an executing job for exactly this state). Keep the `oban_jobs` insert here, or move `PipelineStatus` to `grid_tasks` in the same PR if that is the intended change.
skippy-bot marked this conversation as resolved
graham force-pushed fix/liveview-hooks from 088fb5b51e
Some checks failed
skippy-bot/review Skippy review: 2 blocking findings open — see the PR thread
to dd09d72ced
Some checks failed
skippy-bot/review Skippy review: 1 blocking finding open — see the PR thread
2026-09-22 12:04:18 -05:00
Compare
skippy-bot left a comment

🤖 Skippy PR review

1 finding — none blocking.

Severity Location Issue
🔵 Suggestion assets/js/nav_keys_hook.ts:15 Suggestion - /rover still binds the raw window handler, so its text inputs keep the arrow-key bug

Branch was force-pushed: 088fb5b51e is no longer in history, so this is a full re-read of the PR at dd09d72. Resolved 1 earlier finding (pipeline chip test, now moot). 2 still open (saved-point cap, a/button in IGNORED_TARGET).

Reviewed dd09d72ced84. Comment skippy review to re-run.

### 🤖 Skippy PR review **1 finding** — none blocking. | Severity | Location | Issue | | --- | --- | --- | | 🔵 Suggestion | `assets/js/nav_keys_hook.ts:15` | Suggestion - /rover still binds the raw window handler, so its text inputs keep the arrow-key bug | Branch was force-pushed: 088fb5b51e2d is no longer in history, so this is a full re-read of the PR at dd09d72. Resolved 1 earlier finding (pipeline chip test, now moot). 2 still open (saved-point cap, a/button in IGNORED_TARGET). <sub>Reviewed `dd09d72ced84`. Comment `skippy review` to re-run.</sub> <!-- skippy-pr-review -->
@ -0,0 +12,4 @@
const IGNORED_TARGET =
"input,select,textarea,[contenteditable],a,button,.leaflet-container"
export const NavKeys: Partial<NavKeysHook> = {
First-time contributor

🔵 Suggestion — Suggestion - /rover still binds the raw window handler, so its text inputs keep the arrow-key bug

rover_live.ex:1013 still carries phx-window-keydown="nav_key" on the page root, with the same hour/band stepping handler (rover_live.ex:206), and that binding pushes on every keydown in the window regardless of target. So on /rover the Home grid/lat/lon fields (rover_live.ex:1342, 1372, 1411, 1422) still step the forecast hour and band while the caret moves, which is the defect this hook exists to remove; core_components.ex:1218 documents the two console screens as deliberately sharing the binding. If fix/liveview-guards does not already adopt NavKeys on /rover, wire it there as well: give the rover root div an id plus phx-hook="NavKeys" and drop phx-window-keydown.

**🔵 Suggestion — Suggestion - /rover still binds the raw window handler, so its text inputs keep the arrow-key bug** `rover_live.ex:1013` still carries `phx-window-keydown="nav_key"` on the page root, with the same hour/band stepping handler (`rover_live.ex:206`), and that binding pushes on every keydown in the window regardless of target. So on /rover the Home grid/lat/lon fields (`rover_live.ex:1342`, `1372`, `1411`, `1422`) still step the forecast hour and band while the caret moves, which is the defect this hook exists to remove; `core_components.ex:1218` documents the two console screens as deliberately sharing the binding. If `fix/liveview-guards` does not already adopt `NavKeys` on /rover, wire it there as well: give the rover root div an `id` plus `phx-hook="NavKeys"` and drop `phx-window-keydown`.
graham force-pushed fix/liveview-hooks from dd09d72ced
Some checks failed
skippy-bot/review Skippy review: 1 blocking finding open — see the PR thread
to 07d2c426e7
All checks were successful
skippy-bot/review Skippy review: clean — 1 non-blocking note open
2026-09-22 12:24:20 -05:00
Compare
Author
Owner

Addressed in 07d2c426:

  1. Chip test stubs wrong table — false positive. PipelineStatus.running_workers/0 queries grid_tasks.status == 'running' (pipeline_status.ex:101), not oban_jobs; the moduledoc documents that the Oban seeder exits in ms and the real work is Rust claiming grid_tasks rows. pipeline_status_test.exs asserts the inverse of the review claim: an executing Oban job alone reads :unknown. The test passes as written.
  2. Saved-point cap — moved to the write side: pin_locally/3 refuses to grow the list past @max_saved_points, and restore_saved now validates/dedupes before capping so malformed entries can't crowd out valid ones.
  3. a/button in IGNORED_TARGET — dropped; they have no arrow-key contract and the filter froze nav_key while focus sat on a clicked control.
Addressed in 07d2c426: 1. **Chip test stubs wrong table** — false positive. `PipelineStatus.running_workers/0` queries `grid_tasks.status == 'running'` (pipeline_status.ex:101), not `oban_jobs`; the moduledoc documents that the Oban seeder exits in ms and the real work is Rust claiming grid_tasks rows. `pipeline_status_test.exs` asserts the inverse of the review claim: an executing Oban job alone reads `:unknown`. The test passes as written. 2. **Saved-point cap** — moved to the write side: `pin_locally/3` refuses to grow the list past `@max_saved_points`, and `restore_saved` now validates/dedupes before capping so malformed entries can't crowd out valid ones. 3. **a/button in IGNORED_TARGET** — dropped; they have no arrow-key contract and the filter froze nav_key while focus sat on a clicked control.
graham merged commit 4295add28f into main 2026-09-22 12:24:56 -05:00
First-time contributor

Resolved 2 of 3 earlier findings - the saved-point cap is now on the write side, so the server list and the localStorage mirror agree and the 101st pin is refused instead of silently deleting stored pins; and a,button is out of IGNORED_TARGET, so arrows step the hour/band after a click on a link or control.

1 still open: /rover still binds the raw phx-window-keydown (rover_live.ex:1013), so its text inputs keep the arrow-key bug.

Branch was rebased (dd09d72 is no longer in history), so this was a full re-read at 07d2c42: nothing new.

Resolved 2 of 3 earlier findings - the saved-point cap is now on the write side, so the server list and the localStorage mirror agree and the 101st pin is refused instead of silently deleting stored pins; and `a,button` is out of `IGNORED_TARGET`, so arrows step the hour/band after a click on a link or control. 1 still open: `/rover` still binds the raw `phx-window-keydown` (`rover_live.ex:1013`), so its text inputs keep the arrow-key bug. Branch was rebased (`dd09d72` is no longer in history), so this was a full re-read at `07d2c42`: nothing new. <!-- skippy-pr-review -->
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/prop!22
No description provided.