From 2c8a403cc0e16a064eaf22d5659be1d6647b0ea2 Mon Sep 17 00:00:00 2001
From: Simon <s.thiessen@local-it.org>
Date: Thu, 16 Jul 2026 09:39:18 +0200
Subject: [PATCH] fix(member-live): guard the composite sort event and apply
 review cleanup

---
 assets/css/app.css                            |  3 +-
 assets/js/app.js                              | 52 +++++++++++--------
 lib/mv_web/components/core_components.ex      |  2 +-
 .../components/member_name_sort_header.ex     |  1 +
 lib/mv_web/live/member_live/index.ex          | 17 +++---
 priv/repo/seeds_bulk_members.exs              | 39 ++++++++------
 6 files changed, 66 insertions(+), 48 deletions(-)

diff --git a/assets/css/app.css b/assets/css/app.css
index 8b998608..f13f64c8 100644
--- a/assets/css/app.css
+++ b/assets/css/app.css
@@ -923,8 +923,7 @@
  * pure CSS anchor positioning (the "⋮" trigger carries anchor-name, this popover
  * carries position-anchor). `position-area` drops it just below the trigger,
  * right-aligned, and `position-try-fallbacks` flips it above when there is no
- * room below. The SubfieldMenu hook only toggles it — no coordinate math. This
- * mirrors the payment-badge popover approach (§548) for a consistent look/feel.
+ * room below. The SubfieldMenu hook only toggles it — no coordinate math.
  */
 .popover-menu {
   margin: 0;
diff --git a/assets/js/app.js b/assets/js/app.js
index 85ea1875..745d580f 100644
--- a/assets/js/app.js
+++ b/assets/js/app.js
@@ -352,6 +352,20 @@ Hooks.SortableList = {
   }
 }
 
+// Guarded native Popover API toggles: showPopover()/hidePopover() throw on a
+// detached element or a redundant open/close, so the popover hooks below share
+// these wrappers instead of repeating the try/catch + :popover-open check.
+function openPopover(el) {
+  try {
+    if (el && !el.matches(":popover-open")) el.showPopover()
+  } catch (_e) {}
+}
+function closePopover(el) {
+  try {
+    if (el && el.matches(":popover-open")) el.hidePopover()
+  } catch (_e) {}
+}
+
 // PopoverTooltip hook: shows a hint (referenced by data-tooltip-id) in the
 // browser TOP LAYER via the native Popover API, so it escapes the members
 // table's overflow clipping (overflow-x:auto forces overflow-y:auto, which clips
@@ -386,19 +400,15 @@ Hooks.PopoverTooltip = {
     }
 
     this.show = () => {
+      openPopover(this.tip)
+      // Popover is measurable once open; positioning in the same tick avoids a
+      // visible flash (JS runs before the browser paints).
       try {
-        if (!this.tip.matches(":popover-open")) this.tip.showPopover()
-        // showPopover() makes it measurable; positioning in the same tick avoids
-        // a visible flash (JS runs before the browser paints).
         this.position()
       } catch (_e) {}
     }
 
-    this.hide = () => {
-      try {
-        if (this.tip.matches(":popover-open")) this.tip.hidePopover()
-      } catch (_e) {}
-    }
+    this.hide = () => closePopover(this.tip)
 
     this.onKey = (e) => { if (e.key === "Escape") this.hide() }
 
@@ -431,8 +441,9 @@ Hooks.PopoverTooltip = {
 // sort sub-field. Attached to the "⋮" trigger; data-menu-id names the <ul
 // popover> menu. Handles click / keyboard open, roving arrow-key focus,
 // Escape / click-away close, and closes after a selection (the LiveView
-// re-renders on sort_composite anyway). Positioned with getBoundingClientRect
-// (portable; no CSS anchor-positioning needed).
+// re-renders on sort_composite anyway). The menu and its hint are placed via CSS
+// anchor positioning (see .popover-menu / .popover-tooltip in app.css), so this
+// hook does no coordinate math — it only toggles visibility, focus and keyboard.
 Hooks.SubfieldMenu = {
   mounted() {
     this.menu = document.getElementById(this.el.dataset.menuId)
@@ -442,17 +453,11 @@ Hooks.SubfieldMenu = {
     // .popover-tooltip look as the sort-header tooltips; placement is CSS anchor
     // positioning (no coordinate math).
     this.tip = document.getElementById(this.el.dataset.tooltipId)
+    // Only hint while the menu is closed.
     this.showTip = () => {
-      if (!this.tip || this.menu.matches(":popover-open")) return
-      try {
-        if (!this.tip.matches(":popover-open")) this.tip.showPopover()
-      } catch (_e) {}
-    }
-    this.hideTip = () => {
-      try {
-        if (this.tip && this.tip.matches(":popover-open")) this.tip.hidePopover()
-      } catch (_e) {}
+      if (!this.menu.matches(":popover-open")) openPopover(this.tip)
     }
+    this.hideTip = () => closePopover(this.tip)
 
     this.items = () => [...this.menu.querySelectorAll('[role="menuitemradio"]')]
 
@@ -591,10 +596,11 @@ Hooks.SidebarState = {
 // approaches the viewport, rather than only once it is fully scrolled into view.
 // An IntersectionObserver rooted at the scroll container with a positive bottom
 // rootMargin treats the sentinel as visible while it is still that many pixels
-// below the fold, so the next page loads ahead of time. It complements the
-// built-in phx-viewport-bottom binding (which stays as a reliable backstop);
-// duplicate loads are harmless because the server guards on "more?" and the
-// stream de-duplicates rows by id.
+// below the fold, so the next page loads ahead of time. This is the SOLE load
+// trigger: the table deliberately carries no phx-viewport-bottom binding, whose
+// built-in InfiniteScroll would scroll the sentinel row back into view after each
+// load and cascade into loading every page. Duplicate loads are still harmless —
+// the server guards on "more?" and the stream de-duplicates rows by id.
 Hooks.LoadMorePrefetch = {
   mounted() {
     const container = document.getElementById(this.el.dataset.scrollContainer)
diff --git a/lib/mv_web/components/core_components.ex b/lib/mv_web/components/core_components.ex
index 972abdff..7837491f 100644
--- a/lib/mv_web/components/core_components.ex
+++ b/lib/mv_web/components/core_components.ex
@@ -1042,7 +1042,7 @@ defmodule MvWeb.CoreComponents do
 
   slot :footer,
     doc:
-      "optional after-rows content rendered as a full-width row in a separate, non-streamed tbody. Combined with viewport_bottom it acts as the infinite-scroll sentinel (e.g. a loading indicator) that fires the load event and disappears once no more pages remain."
+      "optional after-rows content rendered as a full-width row in a separate, non-streamed tbody. Combined with infinite_scroll it holds the infinite-scroll sentinel (a hook in the slot fires the load event, plus e.g. a loading indicator) and disappears once no more pages remain."
 
   def table(assigns) do
     assigns =
diff --git a/lib/mv_web/live/components/member_name_sort_header.ex b/lib/mv_web/live/components/member_name_sort_header.ex
index 76c233f2..bfc86ea7 100644
--- a/lib/mv_web/live/components/member_name_sort_header.ex
+++ b/lib/mv_web/live/components/member_name_sort_header.ex
@@ -74,6 +74,7 @@ defmodule MvWeb.Components.MemberNameSortHeader do
         aria-label={gettext("Choose sort field")}
         aria-describedby="member-subfield-tooltip"
         aria-haspopup="menu"
+        aria-controls="member-subfield-menu"
         aria-expanded="false"
         phx-hook="SubfieldMenu"
         data-menu-id="member-subfield-menu"
diff --git a/lib/mv_web/live/member_live/index.ex b/lib/mv_web/live/member_live/index.ex
index a7ca8b33..9d859d3e 100644
--- a/lib/mv_web/live/member_live/index.ex
+++ b/lib/mv_web/live/member_live/index.ex
@@ -226,7 +226,7 @@ defmodule MvWeb.MemberLive.Index do
 
   # Number of members fetched per keyset page (matches the :overview action's
   # default_limit). Mount loads one page; further pages arrive via infinite
-  # scroll (phx-viewport-bottom) so the socket never holds the full table.
+  # scroll (an IntersectionObserver sentinel) so the socket never holds the full table.
   @page_limit 50
 
   # -----------------------------------------------------------------
@@ -384,12 +384,17 @@ defmodule MvWeb.MemberLive.Index do
 
   # The composite "Member" column's sub-field menu (first name / last name) always
   # sorts ascending by the chosen field: it switches the sort key rather than
-  # toggling direction (a click on the header itself toggles direction).
-  def handle_event("sort_composite", %{"field" => field_str}, socket) do
-    field = String.to_existing_atom(field_str)
-    {:noreply, apply_sort_change(socket, field, :asc)}
+  # toggling direction (a click on the header itself toggles direction). The field
+  # is allow-listed to the two real sub-fields, so a crafted event payload cannot
+  # reach String.to_existing_atom with an unknown atom (which would crash the LV).
+  @impl true
+  def handle_event("sort_composite", %{"field" => field}, socket)
+      when field in ["first_name", "last_name"] do
+    {:noreply, apply_sort_change(socket, String.to_existing_atom(field), :asc)}
   end
 
+  def handle_event("sort_composite", _params, socket), do: {:noreply, socket}
+
   # Applies a resolved (field, order) sort: updates assigns and the sort-header
   # components, reloads the keyset stream from page 1, scrolls to top, and syncs
   # the URL. Shared by the header-click ("sort") and sub-field menu
@@ -1268,7 +1273,7 @@ defmodule MvWeb.MemberLive.Index do
   end
 
   # Fetches the next keyset page (if any) and appends it to the stream and the
-  # loaded window. Triggered by phx-viewport-bottom; a no-op once the last page
+  # loaded window. Triggered by the IntersectionObserver sentinel; a no-op once the last page
   # has been reached so the bottom sentinel stops fetching.
   defp load_more(%{assigns: %{more?: false}} = socket), do: socket
 
diff --git a/priv/repo/seeds_bulk_members.exs b/priv/repo/seeds_bulk_members.exs
index cf508fe0..9769141e 100644
--- a/priv/repo/seeds_bulk_members.exs
+++ b/priv/repo/seeds_bulk_members.exs
@@ -100,27 +100,34 @@ Enum.each(1..count, fn i ->
     )
 
   # Give the member fee cycles + a round-robin status so the fee-status column is
-  # populated at scale. Only generate when none exist yet (idempotent re-runs).
+  # populated at scale. Cycles are only generated when none exist yet; the status
+  # is (re-)applied on every run so a run interrupted between generation and the
+  # status update still converges (idempotent — the update is a no-op once set).
   if not is_nil(member.membership_fee_type_id) do
     member = Ash.load!(member, :membership_fee_cycles, actor: admin_user_with_role)
 
-    if Enum.empty?(member.membership_fee_cycles) do
-      {:ok, cycles, _} =
-        CycleGenerator.generate_cycles_for_member(member.id,
-          skip_lock?: true,
-          actor: admin_user_with_role
-        )
+    cycles =
+      if Enum.empty?(member.membership_fee_cycles) do
+        {:ok, generated, _} =
+          CycleGenerator.generate_cycles_for_member(member.id,
+            skip_lock?: true,
+            actor: admin_user_with_role
+          )
 
-      status = Enum.at(statuses, rem(i, length(statuses)))
+        generated
+      else
+        member.membership_fee_cycles
+      end
 
-      Enum.each(cycles, fn cycle ->
-        if cycle.status != status do
-          cycle
-          |> Ash.Changeset.for_update(:update, %{status: status})
-          |> Ash.update!(actor: admin_user_with_role, domain: Mv.MembershipFees)
-        end
-      end)
-    end
+    status = Enum.at(statuses, rem(i, length(statuses)))
+
+    Enum.each(cycles, fn cycle ->
+      if cycle.status != status do
+        cycle
+        |> Ash.Changeset.for_update(:update, %{status: status})
+        |> Ash.update!(actor: admin_user_with_role, domain: Mv.MembershipFees)
+      end
+    end)
   end
 
   if rem(i, 50) == 0, do: IO.puts("  … #{i}/#{count}")