ML-188: guard PubSub record updates during edit mode

Add live_action guard to handle_info({:update, record}) in both CollectionLive.Show and WishlistLive.Show. When the user is editing, background worker updates are skipped and a warning toast is shown instead of overwriting the socket. Normal show-mode updates and mismatched-ID no-ops are unchanged. 6 tests added covering all three code paths.
2026-05-19 11:38:42 +01:00
parent 2fca0bb106
commit e204e555bd
9 changed files with 236 additions and 27 deletions
@@ -1,10 +1,10 @@
 ---
 id: ML-188
 title: Fix form edit + background update race in Show LiveViews
-status: To Do
+status: Done
 assignee: []
 created_date: "2026-05-19 08:42"
-updated_date: "2026-05-19 09:00"
+updated_date: "2026-05-19 10:18"
 labels:
  - audit
  - bug
@@ -19,6 +19,10 @@ documentation:
 modified_files:
  - lib/music_library_web/live/collection_live/show.ex
  - lib/music_library_web/live/wishlist_live/show.ex
+  - lib/music_library_web/live_helpers/record_actions.ex
+  - test/music_library_web/live/collection_live/show_test.exs
+  - test/music_library_web/live/wishlist_live/show_test.exs
+  - docs/architecture.md
 priority: medium
 ordinal: 23000
 ---
@@ -48,15 +52,15 @@ When a user is editing a record in a modal (`live_action == :edit`) and a backgr

 <!-- AC:BEGIN -->

- [ ] #1 handle_info({:update, record}) skips assign(:record, ...) when live_action == :edit in CollectionLive.Show
- [ ] #2 handle_info({:update, record}) skips assign(:record, ...) when live_action == :edit in WishlistLive.Show
- [ ] #3 Warning toast shown to user when background update occurs during edit (different wording from normal info toast)
- [ ] #4 handle_info({:update, record}) still works normally when live_action == :show
- [ ] #5 When user navigates away from edit, handle_params re-fetches fresh record with worker changes
- [ ] #6 Mismatched record.id still handled as no-op (socket unchanged)
- [ ] #7 Tests added for all three cases: :edit guard, :show normal path, mismatched-id no-op
- [ ] #8 docs/architecture.md updated to mention live_action guard
- [ ] #9 Comment added in RecordActions.handle_record_updated/2 noting intentional bypass during :edit
+- [x] #1 handle_info({:update, record}) skips assign(:record, ...) when live_action == :edit in CollectionLive.Show
+- [x] #2 handle_info({:update, record}) skips assign(:record, ...) when live_action == :edit in WishlistLive.Show
+- [x] #3 Warning toast shown to user when background update occurs during edit (different wording from normal info toast)
+- [x] #4 handle_info({:update, record}) still works normally when live_action == :show
+- [x] #5 When user navigates away from edit, handle_params re-fetches fresh record with worker changes
+- [x] #6 Mismatched record.id still handled as no-op (socket unchanged)
+- [x] #7 Tests added for all three cases: :edit guard, :show normal path, mismatched-id no-op
+- [x] #8 docs/architecture.md updated to mention live_action guard
+- [x] #9 Comment added in RecordActions.handle_record_updated/2 noting intentional bypass during :edit
 <!-- AC:END -->

 ## Implementation Notes
@@ -117,3 +121,15 @@ Run: `mix test test/music_library_web/live/collection_live/show_test.exs test/mu
 - Update `docs/architecture.md`: the existing line about `handle_info/2` validating inbound record updates against `socket.assigns.record.id` should also mention the `live_action` guard for form-edit safety
 - Update the task's final summary with a brief note about the design decision
 <!-- SECTION:NOTES:END -->
+
+## Final Summary
+
+<!-- SECTION:FINAL_SUMMARY:BEGIN -->
+
+Added `live_action` guard to `handle_info({:update, record})` in both CollectionLive.Show and WishlistLive.Show. When the user is editing a record (`live_action == :edit`), background worker updates are skipped and a distinct warning toast ("Your edits may be stale — save and re-open...") is shown instead of silently overwriting the socket. When showing (`live_action == :show`), existing behavior is unchanged — the record updates and shows "Record updated in the background". Mismatched-ID records remain a no-op.
+
+This prevents the race where a background worker (PopulateGenres, RefreshCover, etc.) updates the DB and broadcasts `{:update, v2}` while the user's edit modal is open with v1. The guard keeps `@record = v1` on the socket so the form saves only user-changed fields via Ecto changeset semantics. When the user navigates away from edit, `handle_params` re-fetches the fresh record with worker changes.
+
+6 tests added (3 per LiveView), covering: `:show` normal path, `:edit` guard path, and mismatched-ID no-op. Architecture docs updated with guard description.
+
+<!-- SECTION:FINAL_SUMMARY:END -->
@@ -250,11 +250,11 @@ HTTP 429 into `:rate_limit` vs `:auth_error` by reading the body `code`

 ## PubSub Topics

-| PubSub           | Topic Pattern              | Message                  | Used By                                                                                        |
-| ---------------- | -------------------------- | ------------------------ | ---------------------------------------------------------------------------------------------- |
-| `:music_library` | `"records:#{id}"`          | `{:update, record}`      | CollectionLive.Show, WishlistLive.Show — subscribe in handle_params, unsubscribe on navigation |
-| `:music_library` | `"records:index_changed"`  | `:records_index_changed` | CollectionLive.Index, WishlistLive.Index — auto-refresh when background import completes       |
-| `:music_library` | `"listening_stats:update"` | `%{track_count: n}`      | StatsLive.Index, ScrobbledTracksLive.Index — new scrobbles arrived                             |
+| PubSub           | Topic Pattern              | Message                  | Used By                                                                                                                                                                                                                                                                                                                                               |
+| ---------------- | -------------------------- | ------------------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
+| `:music_library` | `"records:#{id}"`          | `{:update, record}`      | CollectionLive.Show, WishlistLive.Show — subscribe in `handle_params`, unsubscribe on navigation. `handle_info({:update, record})` matches record ID against socket and guards against applying background updates during `:edit` `live_action` (skips assign, shows warning toast) to prevent worker changes from overwriting in-progress form edits |
+| `:music_library` | `"records:index_changed"`  | `:records_index_changed` | CollectionLive.Index, WishlistLive.Index — auto-refresh when background import completes                                                                                                                                                                                                                                                              |
+| `:music_library` | `"listening_stats:update"` | `%{track_count: n}`      | StatsLive.Index, ScrobbledTracksLive.Index — new scrobbles arrived                                                                                                                                                                                                                                                                                    |

 ---

@@ -472,13 +472,25 @@ defmodule MusicLibraryWeb.CollectionLive.Show do

  @impl true
  def handle_info({:update, record}, socket) do
-    if record.id == socket.assigns.record.id do
-      {:noreply,
-       socket
-       |> RecordActions.handle_record_updated(record)
-       |> assign_similar_records()}
-    else
-      {:noreply, socket}
+    cond do
+      record.id != socket.assigns.record.id ->
+        {:noreply, socket}
+
+      socket.assigns.live_action == :edit ->
+        {:noreply,
+         socket
+         |> put_toast(
+           :warning,
+           gettext(
+             "Record was updated in the background. Your edits may be stale — save and re-open to see the latest data."
+           )
+         )}
+
+      true ->
+        {:noreply,
+         socket
+         |> RecordActions.handle_record_updated(record)
+         |> assign_similar_records()}
    end
  end

@@ -365,10 +365,22 @@ defmodule MusicLibraryWeb.WishlistLive.Show do

  @impl true
  def handle_info({:update, record}, socket) do
-    if record.id == socket.assigns.record.id do
-      {:noreply, RecordActions.handle_record_updated(socket, record)}
-    else
-      {:noreply, socket}
+    cond do
+      record.id != socket.assigns.record.id ->
+        {:noreply, socket}
+
+      socket.assigns.live_action == :edit ->
+        {:noreply,
+         socket
+         |> put_toast(
+           :warning,
+           gettext(
+             "Record was updated in the background. Your edits may be stale — save and re-open to see the latest data."
+           )
+         )}
+
+      true ->
+        {:noreply, RecordActions.handle_record_updated(socket, record)}
    end
  end

@@ -119,6 +119,10 @@ defmodule MusicLibraryWeb.LiveHelpers.RecordActions do
  @doc """
  Handles a background record update. Returns the updated socket (not wrapped
  in `{:noreply, ...}`) so the caller can pipe additional assigns.
+
+  **Note:** Callers intentionally bypass this function when `live_action == :edit`
+  to prevent background worker updates from overwriting the user's in-progress
+  form edits. See CollectionLive.Show and WishlistLive.Show `handle_info({:update, record})`.
  """
  def handle_record_updated(socket, record) do
    socket
@@ -2556,3 +2556,9 @@ msgstr ""
 #, elixir-autogen, elixir-format
 msgid "ISO format"
 msgstr ""
+
+#: lib/music_library_web/live/collection_live/show.ex
+#: lib/music_library_web/live/wishlist_live/show.ex
+#, elixir-autogen, elixir-format
+msgid "Record was updated in the background. Your edits may be stale — save and re-open to see the latest data."
+msgstr ""
@@ -2556,3 +2556,9 @@ msgstr ""
 #, elixir-autogen, elixir-format
 msgid "ISO format"
 msgstr ""
+
+#: lib/music_library_web/live/collection_live/show.ex
+#: lib/music_library_web/live/wishlist_live/show.ex
+#, elixir-autogen, elixir-format
+msgid "Record was updated in the background. Your edits may be stale — save and re-open to see the latest data."
+msgstr ""
@@ -7,6 +7,7 @@ defmodule MusicLibraryWeb.CollectionLive.ShowTest do
  alias MusicBrainz.Fixtures
  alias MusicLibrary.Assets.Transform
  alias MusicLibrary.Records.Record
+  alias Phoenix.PubSub

  describe "Edit record from show page" do
    test "can navigate to the record edit form", %{conn: conn} do
@@ -68,6 +69,93 @@ defmodule MusicLibraryWeb.CollectionLive.ShowTest do
    end
  end

+  describe "handle_info({:update, record}) with live_action guard" do
+    test "updates record when showing (live_action is :show)", %{conn: conn} do
+      record = record()
+
+      release_response = Fixtures.Release.release(:marbles)
+
+      Req.Test.stub(MusicBrainz.API, fn conn ->
+        Req.Test.json(conn, release_response)
+      end)
+
+      updated_record = %{record | title: "Background Updated Title"}
+
+      session =
+        conn
+        |> visit(~p"/collection/#{record.id}")
+        |> render_async()
+
+      PubSub.broadcast(
+        MusicLibrary.PubSub,
+        "records:#{record.id}",
+        {:update, updated_record}
+      )
+
+      session
+      |> assert_has("*", text: "Background Updated Title", timeout: 200)
+      |> assert_has("#toast-group", text: "Record updated in the background")
+    end
+
+    test "skips update when editing and shows warning toast", %{conn: conn} do
+      record = record()
+
+      release_response = Fixtures.Release.release(:marbles)
+
+      Req.Test.stub(MusicBrainz.API, fn conn ->
+        Req.Test.json(conn, release_response)
+      end)
+
+      updated_record = %{record | title: "Should Not Appear"}
+
+      session =
+        conn
+        |> visit(~p"/collection/#{record.id}")
+        |> render_async()
+        |> click_link("Edit")
+
+      PubSub.broadcast(
+        MusicLibrary.PubSub,
+        "records:#{record.id}",
+        {:update, updated_record}
+      )
+
+      session
+      |> refute_has("h2", text: "Should Not Appear")
+      |> assert_has(
+        "#toast-group",
+        text:
+          "Record was updated in the background. Your edits may be stale — save and re-open to see the latest data."
+      )
+    end
+
+    test "no-ops when broadcasted record has mismatched ID", %{conn: conn} do
+      record = record()
+      other_record = record()
+
+      release_response = Fixtures.Release.release(:marbles)
+
+      Req.Test.stub(MusicBrainz.API, fn conn ->
+        Req.Test.json(conn, release_response)
+      end)
+
+      session =
+        conn
+        |> visit(~p"/collection/#{record.id}")
+        |> render_async()
+
+      PubSub.broadcast(
+        MusicLibrary.PubSub,
+        "records:#{record.id}",
+        {:update, other_record}
+      )
+
+      session
+      |> assert_has("h2", text: escape(record.title))
+      |> refute_has("#toast-group", text: "Record updated in the background")
+    end
+  end
+
  describe "Side panel" do
    test "shows a record's tracks", %{conn: conn} do
      record = record()
@@ -6,6 +6,7 @@ defmodule MusicLibraryWeb.WishlistLive.ShowTest do

  alias MusicLibrary.Assets.Transform
  alias MusicLibrary.Records.Record
+  alias Phoenix.PubSub

  describe "Edit record from show page" do
    test "can navigate to the record edit form", %{conn: conn} do
@@ -51,4 +52,68 @@ defmodule MusicLibraryWeb.WishlistLive.ShowTest do
      end
    end
  end
+
+  describe "handle_info({:update, record}) with live_action guard" do
+    test "updates record when showing (live_action is :show)", %{conn: conn} do
+      record = record(purchased_at: nil)
+      updated_record = %{record | title: "Background Updated Title"}
+
+      session =
+        conn
+        |> visit(~p"/wishlist/#{record.id}")
+
+      PubSub.broadcast(
+        MusicLibrary.PubSub,
+        "records:#{record.id}",
+        {:update, updated_record}
+      )
+
+      session
+      |> assert_has("*", text: "Background Updated Title", timeout: 200)
+      |> assert_has("#toast-group", text: "Record updated in the background")
+    end
+
+    test "skips update when editing and shows warning toast", %{conn: conn} do
+      record = record(purchased_at: nil)
+      updated_record = %{record | title: "Should Not Appear"}
+
+      session =
+        conn
+        |> visit(~p"/wishlist/#{record.id}")
+        |> click_link("Edit")
+
+      PubSub.broadcast(
+        MusicLibrary.PubSub,
+        "records:#{record.id}",
+        {:update, updated_record}
+      )
+
+      session
+      |> refute_has("h2", text: "Should Not Appear")
+      |> assert_has(
+        "#toast-group",
+        text:
+          "Record was updated in the background. Your edits may be stale — save and re-open to see the latest data."
+      )
+    end
+
+    test "no-ops when broadcasted record has mismatched ID", %{conn: conn} do
+      record = record(purchased_at: nil)
+      other_record = record(purchased_at: nil)
+
+      session =
+        conn
+        |> visit(~p"/wishlist/#{record.id}")
+
+      PubSub.broadcast(
+        MusicLibrary.PubSub,
+        "records:#{record.id}",
+        {:update, other_record}
+      )
+
+      session
+      |> assert_has("h2", text: escape(record.title))
+      |> refute_has("#toast-group", text: "Record updated in the background")
+    end
+  end
 end