From 3876861f6f9f07e0875ddb79a16dc293de3d8d0e Mon Sep 17 00:00:00 2001 From: Claudio Ortolina Date: Fri, 8 May 2026 07:03:05 +0100 Subject: [PATCH] ML-168: research and plan --- ...x-views-when-background-import-finishes.md | 163 ++++++++++ ...n-index-when-background-import-finishes.md | 281 ++++++++++++++++++ 2 files changed, 444 insertions(+) create mode 100644 backlog/docs/doc-13 - Research-Updating-index-views-when-background-import-finishes.md create mode 100644 backlog/tasks/ml-168 - Update-wishlist-index-and-collection-index-when-background-import-finishes.md diff --git a/backlog/docs/doc-13 - Research-Updating-index-views-when-background-import-finishes.md b/backlog/docs/doc-13 - Research-Updating-index-views-when-background-import-finishes.md new file mode 100644 index 00000000..c4f99d6f --- /dev/null +++ b/backlog/docs/doc-13 - Research-Updating-index-views-when-background-import-finishes.md @@ -0,0 +1,163 @@ +--- +id: doc-13 +title: "Research: Updating index views when background import finishes" +type: specification +created_date: "2026-05-08 05:44" +updated_date: "2026-05-08 05:53" +--- + +# Research: Updating index views when background import finishes + +## Current State + +### Import Workers + +- `ImportFromMusicbrainzRelease` (queue: `music_brainz`, max_attempts: 3) — used by barcode scan batch imports. Calls `Records.import_from_musicbrainz_release/2` which delegates to `Records.Import.import_from_musicbrainz_release_group/2` → `Records.create_record/1`. +- `ImportFromMusicbrainzReleaseGroup` (queue: `music_brainz`, max_attempts: 3, unique: 300s) — used by cart-style multi-record import. Calls `Records.import_from_musicbrainz_release_group/2` → `Records.create_record/1`. + +**Neither worker broadcasts any PubSub event after successful import.** + +### Records Context + +- `Records.create_record/1` — creates a record, extracts colors, triggers artist info refresh. Does NOT call `notify_update/1`. +- `Records.update_record/2` — updates a record, triggers artist info refresh. Does NOT call `notify_update/1`. +- `Records.notify_update/1` — broadcasts `{:update, record}` on topic `"records:#{record.id}"`. Currently called only by enrichment workers (RefreshCover, PopulateGenres, RecordRefreshMusicBrainzData, GenerateRecordEmbedding). + +### Show Pages (per-record) + +- `CollectionLive.Show` and `WishlistLive.Show` subscribe to `"records:#{record.id}"` in `handle_params`, unsubscribe on navigation, and handle `{:update, record}` by updating their assigns. +- This mechanism works for individual record updates but is not applicable to index pages (which display many records). + +### Index Pages (Collection / Wishlist) + +- `CollectionLive.Index` and `WishlistLive.Index` use `LiveHelpers.IndexActions` for shared logic: `apply_index_action/2` loads records via `load_and_assign_records/2` which calls `context_module.search_records(...)` and `stream(:records, records, reset: true)`. +- **Neither index page subscribes to any PubSub topic.** +- They receive component messages: `{:imported_single, record}` (navigates to show), `{:imported_async, count}` (shows toast, patches to index), `{:saved, _record}` (reloads records). +- When `handle_cart_imported_async` fires, it only shows a toast — it does NOT reload the stream. The user sees stale data until they manually trigger a reload (search, sort, navigate away and back). + +### Async Import Flow (AddRecord Component) + +- Single item: synchronous via `start_async` → `handle_async` sends `{:imported_single, record}` → parent navigates to show page. +- Multiple items (2+): enqueues Oban jobs via `Oban.insert_all` → sends `{:imported_async, count}` → parent shows toast and patches to index. The index reloads from scratch via `apply_index_action` on the patch, but this happens immediately (before the Oban jobs complete). New records appear only after the jobs finish and the user manually refreshes. + +### Async Import Flow (BarcodeScanner Component) + +- When `import_releases` fires with 2+ new records: sync records import immediately, async records enqueue `ImportFromMusicbrainzRelease` jobs → shows toast → patches to `/collection?#{qs}`. Same issue: the index reloads before the jobs complete. + +## Problem Summary + +When background import jobs complete, the collection/wishlist index pages don't automatically pick up the new records. Users must manually refresh. The `handle_cart_imported_async` path reloads records, but does so at enqueue time (before jobs complete), not at job completion time. + +## Viable Implementation Routes + +### Route A: Broadcast from import workers on a new index topic + +**What**: Add a new PubSub topic `"records:index_changed"`. Import workers broadcast on it after `{:ok, record}`. Index pages subscribe and reload. + +**Changes**: + +1. Add `Records.broadcast_index_changed/0` — broadcasts `:records_index_changed` on `"records:index_changed"`. +2. In `ImportFromMusicbrainzRelease.perform/1`, after `{:ok, _record}` → `:ok`, call `Records.broadcast_index_changed()` before returning. +3. Same for `ImportFromMusicbrainzReleaseGroup`. +4. In `CollectionLive.Index.mount/3`, subscribe to `"records:index_changed"`. +5. In `WishlistLive.Index.mount/3`, subscribe to `"records:index_changed"`. +6. Add `handle_info(:records_index_changed, socket)` in both index views → calls `load_and_assign_records`. + +**Pros**: + +- Minimal blast radius — only affects import workers and index views. +- Very simple (2 workers, 2 LiveViews, 1 new context function). +- Follows existing PubSub patterns in the project. +- Can be extended later to cover other CRUD paths. + +**Cons**: + +- Only covers background import paths. Other record creation paths (if any are added later) would need separate treatment. +- Index views reload ALL records on every change, even if the user is deep in pagination. But since the `stream(:records, records, reset: true)` is already called on searches/sorts, this matches existing behavior. + +### Route B: Broadcast from `Records.create_record/1` and `Records.update_record/1` + +**What**: Call `broadcast_index_changed/0` inside the centralized CRUD functions, so ANY record creation or update triggers index refresh. + +**Changes**: + +1. Add `Records.broadcast_index_changed/0`. +2. Call it in `Records.create_record/1` after successful insert. +3. Call it in `Records.update_record/2` after successful update. +4. Call it in `Records.delete_record/1` after successful delete. +5. Subscribe in index views (same as Route A). + +**Pros**: + +- Comprehensive — any record mutation anywhere triggers index refresh. +- Covers future changes without extra work. +- Symmetric: create/update/delete all broadcast. + +**Cons**: + +- Larger blast radius. Every record edit on a show page would trigger index reloads in other browser tabs. +- Potential for "noisy" reloads if users have many tabs open. +- Requires careful review of all call sites for `create_record`, `update_record`, `delete_record` to ensure no unexpected side effects. +- The `RecordForm` component already sends `{:saved, _record}` to the index parent, which calls `handle_record_saved` → `load_and_assign_records`. Adding PubSub on top of that would cause double reloads in the same process (though harmless, just wasteful). + +### Route C: Extend `notify_update` to also broadcast on an index topic + +**What**: Modify the existing `notify_update/1` to broadcast on both the per-record topic (existing) and a new index topic. + +**Changes**: + +1. Modify `Records.notify_update/1` to also broadcast `:records_index_changed` on `"records:index_changed"`. +2. Subscribe in index views (same as Route A). + +**Pros**: + +- Leverages existing code — no new call sites needed in workers. +- All enrichment workers (RefreshCover, PopulateGenres, etc.) automatically trigger index refresh. E.g., if a cover refresh changes an image and you're on the index, you'd see the new cover. + +**Cons**: + +- `notify_update` is currently only for per-record show page updates. Changing its semantics to also trigger index reloads is a behavior change for all existing callers. +- Very noisy — every cover refresh, genre population, embedding generation would trigger full index reloads. This may be undesirable. +- Muddies the separation between per-record update notification and index-level change notification. + +### Route D: Use a Phoenix.PubSub topic with per-section filtering (collection vs wishlist) + +**What**: Two separate topics: `"index:collection"` and `"index:wishlist"`. Import workers broadcast only on the relevant topic based on whether the imported record is purchased (collection) or not (wishlist). + +**Changes**: + +1. Add `Records.broadcast_index_changed(:collection)` and `Records.broadcast_index_changed(:wishlist)`. +2. Import workers determine the destination based on `purchased_at` value. +3. CollectionLive.Index subscribes to `"index:collection"`, WishlistLive.Index to `"index:wishlist"`. + +**Pros**: + +- Finer granularity — importing a wishlist record doesn't trigger collection reload. +- More efficient. + +**Cons**: + +- More complex — two topics, logic to determine which to broadcast on. +- A record moving from wishlist to collection (via `add-to-collection` event in WishlistLive.Index) would need to broadcast on BOTH topics. This adds complexity. +- Over-engineering for the current problem scope. + +## Recommendation: Route A + +Route A is the simplest approach that directly solves the stated problem. It follows existing project patterns (PubSub topics, `handle_info` in LiveViews, `Records.notify_update` precedent). It has the smallest blast radius and can be extended later if needed. + +The decision to NOT use Route B or C is deliberate: + +- Route B's comprehensive approach can be adopted later if the need arises. +- Route C changes the semantics of an existing function used by 4 other workers. +- Route D adds complexity that isn't justified by the current problem. + +## Architecture Impact (Route A) + +- **New PubSub topic**: `"records:index_changed"` on `MusicLibrary.PubSub` +- **Schemas affected**: None (no DB changes) +- **Contexts affected**: `Records` (new `broadcast_index_changed/0` function) +- **Workers affected**: `ImportFromMusicbrainzRelease`, `ImportFromMusicbrainzReleaseGroup` +- **Routes/LiveViews affected**: `CollectionLive.Index`, `WishlistLive.Index` +- **Supervision tree**: No changes +- **External APIs**: No changes +- **UI components**: No changes diff --git a/backlog/tasks/ml-168 - Update-wishlist-index-and-collection-index-when-background-import-finishes.md b/backlog/tasks/ml-168 - Update-wishlist-index-and-collection-index-when-background-import-finishes.md new file mode 100644 index 00000000..8494230c --- /dev/null +++ b/backlog/tasks/ml-168 - Update-wishlist-index-and-collection-index-when-background-import-finishes.md @@ -0,0 +1,281 @@ +--- +id: ML-168 +title: Update wishlist index and collection index when background import finishes +status: To Do +assignee: [] +created_date: '2026-05-08 05:40' +updated_date: '2026-05-08 06:04' +labels: + - ready +dependencies: [] +documentation: + - >- + backlog/docs/doc-13 - + Research-Updating-index-views-when-background-import-finishes.md +priority: medium +--- + +## Description + + + +When importing multiple records, the application performs the import in the background. The collection and wishlist index views should automatically pick up the new records via pubsub event fired when a record is successfully imported. + + + +## Acceptance Criteria + + + +- [ ] #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 + + +## Implementation Plan + + + +# Implementation Plan — Route A (Revised) + +## Objective Alignment + +When background import Oban jobs complete, the collection and wishlist index pages must automatically show new records without requiring a manual refresh. This is achieved by broadcasting a PubSub event from the import workers after successful import, and having the index LiveViews subscribe to that event and reload their record streams. + +## Implementation Steps + +### Step 1: Add `broadcast_index_changed/0` to the Records context + +**File**: `lib/music_library/records.ex` + +Add a new public function in the PubSub section (alongside `subscribe/1`, `unsubscribe/1`, `notify_update/1`): + +```elixir +@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 +``` + +**Verification**: Run `mix test test/music_library/records_test.exs` to confirm existing tests still pass. The function is a simple PubSub wrapper — no new schema or DB logic. + +--- + +### Step 2: Add `subscribe_to_index/0` to the Records context + +**File**: `lib/music_library/records.ex` + +Add a convenience function for subscribing, in the same PubSub section: + +```elixir +@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 +``` + +**Verification**: Compile-check. Used by Steps 4 and 5. + +--- + +### Step 3: Add `handle_index_changed/1` to IndexActions + +**File**: `lib/music_library_web/live_helpers/index_actions.ex` + +Add a public helper that index views can call from `handle_info`: + +```elixir +@doc """ +Handles a PubSub notification that records have changed. +Reloads the record stream using the current parameters. +""" +def handle_index_changed(socket) do + load_and_assign_records(socket, socket.assigns.record_list_params) +end +``` + +Note: `load_and_assign_records/2` is already a public function (`def`, not `defp`) in this module, so the wrapper delegates directly to it. + +**Verification**: Used by Steps 4 and 5. The function delegates to the existing `load_and_assign_records/2` which is already well-tested implicitly. + +--- + +### Step 4: Subscribe to `"records:index_changed"` in CollectionLive.Index + +**File**: `lib/music_library_web/live/collection_live/index.ex` + +In `mount/3`, add a subscription (only when connected — skip during dead render): + +```elixir +def mount(_params, _session, socket) do + if connected?(socket) do + Records.subscribe_to_index() + end + + {:ok, + socket + |> assign(:current_section, :collection) + # ... rest of existing assigns ... + } +end +``` + +Add a `handle_info` clause. **Guard on `live_action`** to avoid unnecessary reloads when the user is on sub-routes where the index grid is hidden behind a modal (e.g., `:import`, `:barcode_scan`): + +```elixir +@impl true +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 +``` + +Rationale for the guard: when the user is on `/collection/import` or `/collection/scan`, the index grid is behind a full-screen modal. Reloading it would waste a query and briefly reassign `page_title`, causing a visual flicker in the modal title. The `:index` and `:edit` actions are the only ones where the grid is visible and should be refreshed. When the user returns to `:index` from a sub-route, `handle_params` already reloads — so no records are missed. + +**Verification**: + +- Run `mix test test/music_library_web/live/collection_live/` (if tests exist; create a basic test if not — see Step 7) +- **Manual browser verification (required)**: Import 2+ records via the AddRecord cart, confirm they appear automatically on the collection index without manual refresh. Also verify that the page title in the import modal does not flicker during background import. + +--- + +### Step 5: Subscribe to `"records:index_changed"` in WishlistLive.Index + +**File**: `lib/music_library_web/live/wishlist_live/index.ex` + +Same pattern as Step 4. Add subscription in `mount/3` (only when connected) and a guarded `handle_info` clause. The WishlistLive.Index handles `:index`, `:edit`, and `:import` actions — guard on `:index` and `:edit` only. + +**Verification**: Same as Step 4, but for the wishlist index page. Manual browser verification required. + +--- + +### Step 6: Call `broadcast_index_changed/0` from import workers after success + +**Files**: + +- `lib/music_library/worker/import_from_musicbrainz_release.ex` +- `lib/music_library/worker/import_from_musicbrainz_release_group.ex` + +In each `perform/1`, after the `{:ok, _record}` match before returning `:ok`, insert a call to `Records.broadcast_index_changed()`. + +Example for `ImportFromMusicbrainzRelease`: + +```elixir +def perform(%Oban.Job{args: %{"release_id" => release_id} = args}) do + # ... existing opts setup ... + case MusicLibrary.Records.import_from_musicbrainz_release(release_id, opts) do + {:ok, _record} -> + Records.broadcast_index_changed() + :ok + other -> + ErrorHandler.to_oban_result(other) + end +end +``` + +Same pattern for `ImportFromMusicbrainzReleaseGroup`. + +**Verification**: + +- Run `mix test test/music_library/worker/import_from_musicbrainz_release_test.exs` +- Run `mix test test/music_library/worker/import_from_musicbrainz_release_group_test.exs` +- Existing tests should continue to pass. New assertions added in Step 7. + +--- + +### Step 7: Write tests + +**Tests to add**: + +1. **`test/music_library/records_test.exs`**: Add a test for `broadcast_index_changed/0` and `subscribe_to_index/0`. Subscribe, call `broadcast_index_changed/0`, assert_receive `:records_index_changed`. + +2. **`test/music_library/worker/import_from_musicbrainz_release_test.exs`**: Add test "broadcasts index_changed after successful import". Subscribe to `"records:index_changed"` before `perform_job`, assert_receive `:records_index_changed` after a successful import. + +3. **`test/music_library/worker/import_from_musicbrainz_release_group_test.exs`**: Same pattern as above. + +4. **LiveView integration tests**: If existing index LiveView test files exist, add a test that mounts the LiveView as `:index`, sends the `:records_index_changed` message, and verifies the stream is reloaded. If no index LiveView test files exist, create a basic test file with a record fixture. Additionally, add a test verifying that the message is ignored (no-op) when `live_action` is `:import` (guard clause test). + +5. **End-to-end browser verification (required, manual)**: Import 2+ records via each path (cart and barcode scan) and confirm: + - Collection index auto-updates with new records + - Wishlist index auto-updates with new records + - No visual flicker or title glitch in modals during background import + +**Verification**: `mix test` passes. + +--- + +## Design Decisions & Tradeoffs + +### Why a single topic for both collection and wishlist? + +Both import workers handle both collection records (purchased_at set) and wishlist records (purchased_at nil). There is no clean way to determine which index to notify before the import completes. Using a single `"records:index_changed"` topic means both index pages reload when either type of record is imported. This is slightly wasteful (one extra FTS query on the non-matching index) but the cost is negligible. The alternative — inspecting the record after import and broadcasting to a type-specific topic — adds complexity for no practical gain. + +### Why guard `handle_info` on `live_action`? + +Without the guard, when a background import completes while the user is on `/collection/import` (modal open), `load_and_assign_records` reassigns `page_title` from "Add new Record · Collection" to "Collection", causing a brief visual flicker in the modal title. The guard prevents this by only reloading when the grid is actually visible (`:index` and `:edit` actions). When the user closes the modal and returns to `:index`, `handle_params` triggers a fresh load anyway — no records are missed. + +### Double-reload on async import (harmless) + +When the user triggers a batch import, `handle_cart_imported_async` already calls `push_patch` to the base index path, which triggers `handle_params` → `apply_index_action` → `load_and_assign_records`. When the worker later completes, the PubSub broadcast triggers a second `load_and_assign_records`. This is a harmless double-load (one extra FTS query) — not worth optimizing. + +--- + +## Architecture Impact + +| Touchpoint | Impact | +| ------------------------------ | ----------------------------------------------------------------------------------------------------------- | +| **PubSub topic** | NEW: `"records:index_changed"` on `MusicLibrary.PubSub` with message `:records_index_changed` | +| **Schemas** | None | +| **Records context** | + `broadcast_index_changed/0`, + `subscribe_to_index/0` (2 new functions) | +| **Import workers** | Both modified to call `broadcast_index_changed/0` after success | +| **CollectionLive.Index** | + subscription in `mount`, + two `handle_info(:records_index_changed, ...)` clauses (guarded and catch-all) | +| **WishlistLive.Index** | + subscription in `mount`, + two `handle_info(:records_index_changed, ...)` clauses (guarded and catch-all) | +| **IndexActions (LiveHelpers)** | + `handle_index_changed/1` public function | +| **Supervision tree** | No changes | +| **External APIs** | No changes | +| **UI components** | No changes | +| **Routes** | No changes | + +## Performance Profile + +- **PubSub broadcast**: O(1) — broadcasts to all subscribers of the topic. In production, this is 0–2 subscribers (the two index LiveViews, if the user has those pages open). Dead subscribers are automatically cleaned up by Phoenix.PubSub's process monitoring. +- **Record reload in index**: The index reload calls `context_module.search_records(query, limit: 72, offset: current_offset, order: current_order)`. This is a single FTS5 query + records table join, equivalent to what happens on any search/sort. At most one query per index page open. +- **No N+1 risk**: The `search_records` function already preloads associations (artists, etc.) via the FTS5 join pattern. +- **Memory**: No additional memory footprint. The PubSub message is a single atom `:records_index_changed`. +- **Throttling**: Not needed. Import workers are rate-limited by the `:music_brainz` queue (concurrency: 1), so broadcasts are naturally spaced. Even during batch imports, the broadcast happens once per job completion, not once per record search. +- **Guard clause avoids wasteful work**: When the user is on a sub-route (`:import`, `:barcode_scan`), the reload is skipped entirely, saving one FTS query. + +## Benchmarking Requirements + +No benchmarks needed. The change adds no new database queries beyond what already runs on every search/sort action. The PubSub broadcast is an O(1) in-process message delivery to at most 2 subscribers. + +## Cost Profile + +No paid resources consumed. The change is purely in-process PubSub and existing database queries. No additional API calls, compute, or storage costs. + +## Production Changes + +**None required.** No environment variables, service provisioning, database migrations, DNS changes, or firewall rules needed. This is a pure application code change deployed via the normal CI/CD pipeline. + +## Documentation Updates + +| Document | Changes Needed | +| -------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `docs/architecture.md` | Add `"records:index_changed"` row to the PubSub Topics table (topic: `"records:index_changed"`, message: `:records_index_changed`, used by: CollectionLive.Index, WishlistLive.Index) | +| `.agents/skills/oban-worker/SKILL.md` | No changes needed — existing patterns unchanged | +| `.agents/skills/ui-framework/SKILL.md` | No changes needed — existing patterns unchanged | + +