Fix LiveView hook lifecycle, chart repaints, and key handling #22
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/liveview-hooks"
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?
Audit LiveView/TS group (part 1).
Note: fix/liveview-guards uses the NavKeys hook introduced here — merge this one first (or rebase).
Verified: 298 targeted tests + tsc + assets.build green.
🤖 Skippy PR review
3 findings — 2 blocking before merge.
test/microwaveprop_web/live/map_live_test.exs:1218lib/microwaveprop_web/live/map_live.ex:385assets/js/nav_keys_hook.ts:12Reviewed
088fb5b51e2d. Commentskippy reviewto re-run.@ -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 =🔵 Suggestion — Ignoring arrows on a/button kills hour stepping after any click
aandbuttonhave no arrow-key contract of their own, so listing them inIGNORED_TARGETmeans arrow navigation stops for as long as focus sits on a control. Concrete path: click a band-rail cell (atabindex="-1"button,map_live.ex:1693) or any link, then press arrows, andnav_keyis never pushed, so the forecast hour and band no longer step (they did withphx-window-keydown). Keep the guard for editable targets and.leaflet-container(the map owns arrows while focused) and dropa,buttonunless a specific control consumes arrows.@ -377,6 +382,7 @@ defmodule MicrowavepropWeb.MapLive doelserestored =points|> Enum.take(@max_saved_points)🟡 Warning — Saved-point cap drops pins 101+ out of localStorage on the next save
Enum.take/2caps only the restore path: a stored list longer than 100 is restored as its first 100, while localStorage still holds all of it. The nextsave_point/remove_savedcallspush_saved_points/1, which mirrors the whole (now 101-entry) server list back through thesaved_pointsevent, 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 insidepush_saved_points/1so the mirror and the server agree. Minor, same line: the cap runs beforerestore_point/1validation, so 100 malformed entries can crowd out valid ones.@ -1219,3 +1218,1 @@queue: "propagation",worker: "Microwaveprop.Workers.PropagationGridWorker",args: %{},struct!(Microwaveprop.Propagation.GridTask, %{🟠 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) queriesstate == "executing"rows in oban_jobs;grid_tasksonly feeds the freshness path (Propagation.latest_chain_completion_time/0). Agrid_tasksrow withstatus: "running"therefore cannot makePipelineStatus.current/0return:running, so both assertions below ("Updating propagation",data-pipeline-state="running") fail no matter what this row says. TheOban.Jobstub this replaces was the one that worked (seetest/microwaveprop/propagation/pipeline_status_test.exs:44-53, which still stubs an executing job for exactly this state). Keep theoban_jobsinsert here, or movePipelineStatustogrid_tasksin the same PR if that is the intended change.088fb5b51edd09d72ced🤖 Skippy PR review
1 finding — none blocking.
assets/js/nav_keys_hook.ts:15Branch was force-pushed:
088fb5b51eis no longer in history, so this is a full re-read of the PR atdd09d72. Resolved 1 earlier finding (pipeline chip test, now moot). 2 still open (saved-point cap, a/button in IGNORED_TARGET).Reviewed
dd09d72ced84. Commentskippy reviewto re-run.@ -0,0 +12,4 @@const IGNORED_TARGET ="input,select,textarea,[contenteditable],a,button,.leaflet-container"export const NavKeys: Partial<NavKeysHook> = {🔵 Suggestion — Suggestion - /rover still binds the raw window handler, so its text inputs keep the arrow-key bug
rover_live.ex:1013still carriesphx-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:1218documents the two console screens as deliberately sharing the binding. Iffix/liveview-guardsdoes not already adoptNavKeyson /rover, wire it there as well: give the rover root div anidplusphx-hook="NavKeys"and dropphx-window-keydown.dd09d72ced07d2c426e7Addressed in
07d2c426:PipelineStatus.running_workers/0queriesgrid_tasks.status == 'running'(pipeline_status.ex:101), notoban_jobs; the moduledoc documents that the Oban seeder exits in ms and the real work is Rust claiming grid_tasks rows.pipeline_status_test.exsasserts the inverse of the review claim: an executing Oban job alone reads:unknown. The test passes as written.pin_locally/3refuses to grow the list past@max_saved_points, andrestore_savednow validates/dedupes before capping so malformed entries can't crowd out valid ones.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,buttonis out ofIGNORED_TARGET, so arrows step the hour/band after a click on a link or control.1 still open:
/roverstill binds the rawphx-window-keydown(rover_live.ex:1013), so its text inputs keep the arrow-key bug.Branch was rebased (
dd09d72is no longer in history), so this was a full re-read at07d2c42: nothing new.