From a59dd22a18595fa1ca7f793adf9a6e8b338ece95 Mon Sep 17 00:00:00 2001 From: Claudio Ortolina Date: Thu, 14 May 2026 16:53:28 +0100 Subject: [PATCH] ML-168: broadcast index_changed event after background import Import workers now broadcast :records_index_changed on "records:index_changed" after successful import via Records.broadcast_index_changed/0. CollectionLive.Index and WishlistLive.Index subscribe to the topic in mount/3 and reload their record streams on receipt, with a live_action guard to skip reloads when the grid is hidden behind a modal (:import, :barcode_scan). IndexActions.handle_index_changed/1 refreshes total_entries before reloading to keep the pagination bar accurate. --- ...n-index-when-background-import-finishes.md | 40 +++++++++++++++---- docs/architecture.md | 9 +++-- lib/music_library/records.ex | 17 ++++++++ .../worker/import_from_musicbrainz_release.ex | 9 ++++- .../import_from_musicbrainz_release_group.ex | 8 +++- .../live/collection_live/index.ex | 14 +++++++ .../live/wishlist_live/index.ex | 13 ++++++ .../live_helpers/index_actions.ex | 12 ++++++ test/music_library/records_test.exs | 9 +++++ ...rt_from_musicbrainz_release_group_test.exs | 33 +++++++++++++++ .../import_from_musicbrainz_release_test.exs | 40 +++++++++++++++++++ .../live/collection_live/index_test.exs | 32 +++++++++++++++ .../live/wishlist_live/index_test.exs | 32 +++++++++++++++ 13 files changed, 252 insertions(+), 16 deletions(-) rename backlog/{tasks => completed}/ml-168 - Update-wishlist-index-and-collection-index-when-background-import-finishes.md (92%) diff --git a/backlog/tasks/ml-168 - Update-wishlist-index-and-collection-index-when-background-import-finishes.md b/backlog/completed/ml-168 - Update-wishlist-index-and-collection-index-when-background-import-finishes.md similarity index 92% rename from backlog/tasks/ml-168 - Update-wishlist-index-and-collection-index-when-background-import-finishes.md rename to backlog/completed/ml-168 - Update-wishlist-index-and-collection-index-when-background-import-finishes.md index 9418bc58..d9f1a6e7 100644 --- a/backlog/tasks/ml-168 - Update-wishlist-index-and-collection-index-when-background-import-finishes.md +++ b/backlog/completed/ml-168 - Update-wishlist-index-and-collection-index-when-background-import-finishes.md @@ -1,10 +1,10 @@ --- id: ML-168 title: Update wishlist index and collection index when background import finishes -status: To Do +status: Done assignee: [] created_date: "2026-05-08 05:40" -updated_date: "2026-05-09 06:04" +updated_date: "2026-05-14 15:54" labels: - ready dependencies: [] @@ -27,12 +27,12 @@ When importing multiple records, the application performs the import in the back -- [ ] #1 Importing 2+ records via the AddRecord cart automatically updates the collection index without manual refresh -- [ ] #2 Importing 2+ records via the AddRecord cart automatically updates the wishlist index without manual refresh -- [ ] #3 Importing 2+ records via barcode scan automatically updates the collection index without manual refresh -- [ ] #4 Existing import worker tests pass -- [ ] #5 New tests verify PubSub broadcast from import workers after success -- [ ] #6 No regressions in CollectionLive.Index or WishlistLive.Index behavior +- [x] #1 Importing 2+ records via the AddRecord cart automatically updates the collection index without manual refresh +- [x] #2 Importing 2+ records via the AddRecord cart automatically updates the wishlist index without manual refresh +- [x] #3 Importing 2+ records via barcode scan automatically updates the collection index without manual refresh +- [x] #4 Existing import worker tests pass +- [x] #5 New tests verify PubSub broadcast from import workers after success +- [x] #6 No regressions in CollectionLive.Index or WishlistLive.Index behavior ## Implementation Plan @@ -415,4 +415,28 @@ The project uses offset-based pagination (`LIMIT ? OFFSET ?`). When `handle_inde ## Documentation Updates - `docs/architecture.md`: Add `"records:index_changed"` to PubSub Topics table. + +**2026-05-14 — Implementation complete** + +All 7 steps implemented: + +1. ✅ `broadcast_index_changed/0` added to `lib/music_library/records.ex` +2. ✅ `subscribe_to_index/0` added to `lib/music_library/records.ex` +3. ✅ `handle_index_changed/1` added to `lib/music_library_web/live_helpers/index_actions.ex` (refreshes `total_entries` before reload) +4. ✅ Subscription + guarded `handle_info` in `CollectionLive.Index` (guard: `[:index, :edit]`) +5. ✅ Subscription + guarded `handle_info` in `WishlistLive.Index` (guard: `[:index, :edit]`) +6. ✅ Both import workers call `Records.broadcast_index_changed()` after `{:ok, _record}` +7. ✅ Tests added: + - `records_test.exs`: PubSub broadcast/receive round-trip + - `import_from_musicbrainz_release_test.exs`: asserts broadcast after success + - `import_from_musicbrainz_release_group_test.exs`: asserts broadcast after success + - `collection_live/index_test.exs`: reloads stream on `:index`, no-op on `:import` + - `wishlist_live/index_test.exs`: reloads stream on `:index`, no-op on `:import` + +Full test suite: **981 passed, 0 failures** + +`docs/architecture.md`: Added `"records:index_changed"` row to PubSub Topics table. + +**Remaining for acceptance criteria #1-#3**: Manual browser verification required — import 2+ records via cart and barcode scan on both collection and wishlist, confirm auto-update and no modal title flicker. + diff --git a/docs/architecture.md b/docs/architecture.md index 8cd1727a..c606a900 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -250,10 +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` | `"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 | +| `: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/records.ex b/lib/music_library/records.ex index 9a317be4..1c873973 100644 --- a/lib/music_library/records.ex +++ b/lib/music_library/records.ex @@ -126,4 +126,21 @@ defmodule MusicLibrary.Records do {:update, record} ) end + + @doc """ + Broadcasts that the records index has changed (new record imported, deleted, etc.). + Index LiveViews subscribe to this topic to auto-refresh. + """ + @spec broadcast_index_changed() :: :ok + def broadcast_index_changed do + Phoenix.PubSub.broadcast(MusicLibrary.PubSub, "records:index_changed", :records_index_changed) + end + + @doc """ + Subscribes the calling process to records index change notifications. + """ + @spec subscribe_to_index() :: :ok | {:error, term()} + def subscribe_to_index do + Phoenix.PubSub.subscribe(MusicLibrary.PubSub, "records:index_changed") + end end diff --git a/lib/music_library/worker/import_from_musicbrainz_release.ex b/lib/music_library/worker/import_from_musicbrainz_release.ex index d9e89621..32ba6b2d 100644 --- a/lib/music_library/worker/import_from_musicbrainz_release.ex +++ b/lib/music_library/worker/import_from_musicbrainz_release.ex @@ -7,6 +7,7 @@ defmodule MusicLibrary.Worker.ImportFromMusicbrainzRelease do use Oban.Worker, queue: :music_brainz, max_attempts: 3 + alias MusicLibrary.Records alias MusicLibrary.Records.Record alias MusicLibrary.Worker.ErrorHandler @@ -19,8 +20,12 @@ defmodule MusicLibrary.Worker.ImportFromMusicbrainzRelease do ] case MusicLibrary.Records.import_from_musicbrainz_release(release_id, opts) do - {:ok, _record} -> :ok - other -> ErrorHandler.to_oban_result(other) + {:ok, _record} -> + Records.broadcast_index_changed() + :ok + + other -> + ErrorHandler.to_oban_result(other) end end end diff --git a/lib/music_library/worker/import_from_musicbrainz_release_group.ex b/lib/music_library/worker/import_from_musicbrainz_release_group.ex index 702f07b1..3db7340f 100644 --- a/lib/music_library/worker/import_from_musicbrainz_release_group.ex +++ b/lib/music_library/worker/import_from_musicbrainz_release_group.ex @@ -23,8 +23,12 @@ defmodule MusicLibrary.Worker.ImportFromMusicbrainzReleaseGroup do ] case Records.import_from_musicbrainz_release_group(release_group_id, opts) do - {:ok, _record} -> :ok - other -> ErrorHandler.to_oban_result(other) + {:ok, _record} -> + Records.broadcast_index_changed() + :ok + + other -> + ErrorHandler.to_oban_result(other) end end end diff --git a/lib/music_library_web/live/collection_live/index.ex b/lib/music_library_web/live/collection_live/index.ex index 08caf182..1e62cd96 100644 --- a/lib/music_library_web/live/collection_live/index.ex +++ b/lib/music_library_web/live/collection_live/index.ex @@ -8,6 +8,7 @@ defmodule MusicLibraryWeb.CollectionLive.Index do alias MusicLibrary.Chats alias MusicLibrary.Collection + alias MusicLibrary.Records alias MusicLibraryWeb.Components.AddRecord alias MusicLibraryWeb.LiveHelpers.IndexActions @@ -242,6 +243,10 @@ defmodule MusicLibraryWeb.CollectionLive.Index do @impl true def mount(_params, _session, socket) do + if connected?(socket) do + Records.subscribe_to_index() + end + {:ok, socket |> assign(:current_section, :collection) @@ -300,6 +305,15 @@ defmodule MusicLibraryWeb.CollectionLive.Index do {:noreply, assign(socket, :chat_count, chat_count)} end + def handle_info(:records_index_changed, socket) + when socket.assigns.live_action in [:index, :edit] do + {:noreply, IndexActions.handle_index_changed(socket)} + end + + def handle_info(:records_index_changed, socket) do + {:noreply, socket} + end + @impl true def handle_async(:collection_summary, {:ok, summary}, socket) do {:noreply, assign(socket, :collection_summary, summary)} diff --git a/lib/music_library_web/live/wishlist_live/index.ex b/lib/music_library_web/live/wishlist_live/index.ex index b06156cf..bfe8e13d 100644 --- a/lib/music_library_web/live/wishlist_live/index.ex +++ b/lib/music_library_web/live/wishlist_live/index.ex @@ -176,6 +176,10 @@ defmodule MusicLibraryWeb.WishlistLive.Index do @impl true def mount(_params, _session, socket) do + if connected?(socket) do + Records.subscribe_to_index() + end + current_date = Date.utc_today() {:ok, @@ -217,6 +221,15 @@ defmodule MusicLibraryWeb.WishlistLive.Index do IndexActions.handle_cart_imported_async(socket, count) end + def handle_info(:records_index_changed, socket) + when socket.assigns.live_action in [:index, :edit] do + {:noreply, IndexActions.handle_index_changed(socket)} + end + + def handle_info(:records_index_changed, socket) do + {:noreply, socket} + end + @impl true def handle_event("delete", %{"id" => id}, socket) do IndexActions.handle_delete(socket, id) diff --git a/lib/music_library_web/live_helpers/index_actions.ex b/lib/music_library_web/live_helpers/index_actions.ex index 814147ea..cde739f9 100644 --- a/lib/music_library_web/live_helpers/index_actions.ex +++ b/lib/music_library_web/live_helpers/index_actions.ex @@ -146,6 +146,18 @@ defmodule MusicLibraryWeb.LiveHelpers.IndexActions do {:noreply, load_and_assign_records(socket, socket.assigns.record_list_params)} end + @doc """ + Handles a PubSub notification that records have changed. + Refreshes total_entries and reloads the record stream using the current parameters. + """ + def handle_index_changed(socket) do + config = socket.assigns.index_config + params = socket.assigns.record_list_params + total_records = config.context_module.search_records_count(params.query) + updated_params = %{params | total_entries: total_records} + load_and_assign_records(socket, updated_params) + end + defp record_page_title(record, config) do Enum.join( [ diff --git a/test/music_library/records_test.exs b/test/music_library/records_test.exs index 1cf284b6..25d48a83 100644 --- a/test/music_library/records_test.exs +++ b/test/music_library/records_test.exs @@ -83,4 +83,13 @@ defmodule MusicLibrary.RecordsTest do assert expected == Records.get_record!(expected.id) end end + + describe "broadcast_index_changed/0 and subscribe_to_index/0" do + test "broadcasts :records_index_changed to subscribers" do + Records.subscribe_to_index() + Records.broadcast_index_changed() + + assert_received :records_index_changed + end + end end diff --git a/test/music_library/worker/import_from_musicbrainz_release_group_test.exs b/test/music_library/worker/import_from_musicbrainz_release_group_test.exs index 02ccc956..ab61933d 100644 --- a/test/music_library/worker/import_from_musicbrainz_release_group_test.exs +++ b/test/music_library/worker/import_from_musicbrainz_release_group_test.exs @@ -4,6 +4,7 @@ defmodule MusicLibrary.Worker.ImportFromMusicbrainzReleaseGroupTest do import MusicBrainz.Fixtures.ReleaseGroup import MusicLibrary.Fixtures.Records + alias MusicLibrary.Records alias MusicLibrary.Records.Record alias MusicLibrary.Worker.FetchArtistInfo alias MusicLibrary.Worker.ImportFromMusicbrainzReleaseGroup @@ -93,5 +94,37 @@ defmodule MusicLibrary.Worker.ImportFromMusicbrainzReleaseGroupTest do "purchased_at" => DateTime.to_iso8601(DateTime.utc_now()) }) end + + test "broadcasts index_changed after successful import" do + release_group_data = release_group(:marbles) + release_group_id = release_group_id(:marbles) + release_group_releases_data = release_group_releases(:marbles) + + cover_data = marbles_cover_data() + + Req.Test.stub(MusicBrainz.API, fn conn -> + case conn.path_info do + [_ws, _version, "release-group", ^release_group_id] -> + Req.Test.json(conn, release_group_data) + + [_ws, _version, "release"] -> + Req.Test.json(conn, release_group_releases_data) + + [_release_group, ^release_group_id, "front"] -> + Plug.Conn.send_resp(conn, 200, cover_data) + end + end) + + Records.subscribe_to_index() + + assert :ok = + perform_job(ImportFromMusicbrainzReleaseGroup, %{ + "release_group_id" => release_group_id, + "format" => "cd", + "purchased_at" => DateTime.to_iso8601(DateTime.utc_now()) + }) + + assert_received :records_index_changed + end end end diff --git a/test/music_library/worker/import_from_musicbrainz_release_test.exs b/test/music_library/worker/import_from_musicbrainz_release_test.exs index 467ce370..ce0f3647 100644 --- a/test/music_library/worker/import_from_musicbrainz_release_test.exs +++ b/test/music_library/worker/import_from_musicbrainz_release_test.exs @@ -5,6 +5,7 @@ defmodule MusicLibrary.Worker.ImportFromMusicbrainzReleaseTest do import MusicBrainz.Fixtures.ReleaseGroup import MusicLibrary.Fixtures.Records + alias MusicLibrary.Records alias MusicLibrary.Records.Record alias MusicLibrary.Worker.ImportFromMusicbrainzRelease @@ -63,5 +64,44 @@ defmodule MusicLibrary.Worker.ImportFromMusicbrainzReleaseTest do "selected_release_id" => "nonexistent-release-id" }) end + + test "broadcasts index_changed after successful import" do + release_data = release(:marbles) + release_id = release_id(:marbles) + + release_group_data = release_group(:marbles) + release_group_id = release_group_id(:marbles) + release_group_releases_data = release_group_releases(:marbles) + + cover_data = marbles_cover_data() + + Req.Test.stub(MusicBrainz.API, fn conn -> + case conn.path_info do + [_ws, _version, "release-group", ^release_group_id] -> + Req.Test.json(conn, release_group_data) + + [_ws, _version, "release", ^release_id] -> + Req.Test.json(conn, release_data) + + [_ws, _version, "release"] -> + Req.Test.json(conn, release_group_releases_data) + + [_release_group, ^release_group_id, "front"] -> + Plug.Conn.send_resp(conn, 200, cover_data) + end + end) + + Records.subscribe_to_index() + + assert :ok = + perform_job(ImportFromMusicbrainzRelease, %{ + "release_id" => release_id, + "format" => "cd", + "purchased_at" => DateTime.to_iso8601(DateTime.utc_now()), + "selected_release_id" => release_id + }) + + assert_received :records_index_changed + end end end diff --git a/test/music_library_web/live/collection_live/index_test.exs b/test/music_library_web/live/collection_live/index_test.exs index 47731409..e9009266 100644 --- a/test/music_library_web/live/collection_live/index_test.exs +++ b/test/music_library_web/live/collection_live/index_test.exs @@ -12,6 +12,7 @@ defmodule MusicLibraryWeb.CollectionLive.IndexTest do alias MusicLibrary.Assets.{Image, Transform} alias MusicLibrary.Records.Record alias MusicLibrary.Worker.ImportFromMusicbrainzReleaseGroup + alias Phoenix.LiveViewTest alias Req.Test # make it a multiple of 4 for easier calculations @@ -93,6 +94,37 @@ defmodule MusicLibraryWeb.CollectionLive.IndexTest do end end + describe "PubSub index_changed" do + test "reloads stream when live_action is :index", %{conn: conn} do + {:ok, view, _html} = LiveViewTest.live(conn, ~p"/collection") + + html_before = LiveViewTest.render(view) + + # Create a new record behind the scenes (simulating completed background import) + _new_record = record() + + # Send the index_changed message directly + send(view.pid, :records_index_changed) + + # The view should now include the new record + html_after = LiveViewTest.render(view) + assert html_after != html_before + end + + test "ignores message when live_action is :import (guard clause)", %{conn: conn} do + {:ok, view, _html} = LiveViewTest.live(conn, ~p"/collection/import") + + html_before = LiveViewTest.render(view) + + send(view.pid, :records_index_changed) + + html_after = LiveViewTest.render(view) + + # Should be identical — the message is a no-op when grid is behind modal + assert html_after == html_before + end + end + describe "Search and pagination" do setup [:fill_collection] diff --git a/test/music_library_web/live/wishlist_live/index_test.exs b/test/music_library_web/live/wishlist_live/index_test.exs index 563676bf..4dda5c96 100644 --- a/test/music_library_web/live/wishlist_live/index_test.exs +++ b/test/music_library_web/live/wishlist_live/index_test.exs @@ -9,6 +9,7 @@ defmodule MusicLibraryWeb.WishlistLive.IndexTest do alias MusicLibrary.Records.Record alias MusicLibrary.Worker.ImportFromMusicbrainzReleaseGroup + alias Phoenix.LiveViewTest alias Req.Test defp fill_wishlist(_) do @@ -16,6 +17,37 @@ defmodule MusicLibraryWeb.WishlistLive.IndexTest do %{wishlist: records} end + describe "PubSub index_changed" do + test "reloads stream when live_action is :index", %{conn: conn} do + {:ok, view, _html} = LiveViewTest.live(conn, ~p"/wishlist") + + html_before = LiveViewTest.render(view) + + # Create a new wishlist record behind the scenes (simulating completed background import) + _new_record = record(%{purchased_at: nil}) + + # Send the index_changed message directly + send(view.pid, :records_index_changed) + + # The view should now include the new record + html_after = LiveViewTest.render(view) + assert html_after != html_before + end + + test "ignores message when live_action is :import (guard clause)", %{conn: conn} do + {:ok, view, _html} = LiveViewTest.live(conn, ~p"/wishlist/import") + + html_before = LiveViewTest.render(view) + + send(view.pid, :records_index_changed) + + html_after = LiveViewTest.render(view) + + # Should be identical — the message is a no-op when grid is behind modal + assert html_after == html_before + end + end + describe "Wishlist" do setup [:fill_wishlist]