diff --git a/backlog/tasks/ml-188 - Fix-form-edit-background-update-race-in-Show-LiveViews.md b/backlog/completed/ml-188 - Fix-form-edit-background-update-race-in-Show-LiveViews.md similarity index 74% rename from backlog/tasks/ml-188 - Fix-form-edit-background-update-race-in-Show-LiveViews.md rename to backlog/completed/ml-188 - Fix-form-edit-background-update-race-in-Show-LiveViews.md index fc6e8f7c..f23306f4 100644 --- a/backlog/tasks/ml-188 - Fix-form-edit-background-update-race-in-Show-LiveViews.md +++ b/backlog/completed/ml-188 - Fix-form-edit-background-update-race-in-Show-LiveViews.md @@ -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 -- [ ] #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 ## 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 + +## Final Summary + + + +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. + + diff --git a/docs/architecture.md b/docs/architecture.md index 920f9273..03a1f860 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -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 | --- diff --git a/lib/music_library_web/live/collection_live/show.ex b/lib/music_library_web/live/collection_live/show.ex index 917dc5f1..d87b8072 100644 --- a/lib/music_library_web/live/collection_live/show.ex +++ b/lib/music_library_web/live/collection_live/show.ex @@ -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 diff --git a/lib/music_library_web/live/wishlist_live/show.ex b/lib/music_library_web/live/wishlist_live/show.ex index f2749b7a..2c37885b 100644 --- a/lib/music_library_web/live/wishlist_live/show.ex +++ b/lib/music_library_web/live/wishlist_live/show.ex @@ -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 diff --git a/lib/music_library_web/live_helpers/record_actions.ex b/lib/music_library_web/live_helpers/record_actions.ex index 20eecebe..6b7ccfe3 100644 --- a/lib/music_library_web/live_helpers/record_actions.ex +++ b/lib/music_library_web/live_helpers/record_actions.ex @@ -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 diff --git a/priv/gettext/default.pot b/priv/gettext/default.pot index c6ce4fac..354e98a2 100644 --- a/priv/gettext/default.pot +++ b/priv/gettext/default.pot @@ -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 "" diff --git a/priv/gettext/en/LC_MESSAGES/default.po b/priv/gettext/en/LC_MESSAGES/default.po index 6593a477..112461f1 100644 --- a/priv/gettext/en/LC_MESSAGES/default.po +++ b/priv/gettext/en/LC_MESSAGES/default.po @@ -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 "" diff --git a/test/music_library_web/live/collection_live/show_test.exs b/test/music_library_web/live/collection_live/show_test.exs index b11dd88e..6fed2251 100644 --- a/test/music_library_web/live/collection_live/show_test.exs +++ b/test/music_library_web/live/collection_live/show_test.exs @@ -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() diff --git a/test/music_library_web/live/wishlist_live/show_test.exs b/test/music_library_web/live/wishlist_live/show_test.exs index e053eb7a..c9a87bcc 100644 --- a/test/music_library_web/live/wishlist_live/show_test.exs +++ b/test/music_library_web/live/wishlist_live/show_test.exs @@ -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