--- id: ML-169.10 title: "Fix: Async optimistic UI for index/list delete streams" status: Done assignee: [] created_date: "2026-05-19 11:48" updated_date: "2026-05-22 15:00" labels: - perf - fix - ux dependencies: [] references: - >- backlog/documents/doc-27 - Phase-4-Performance-Audit-Low-Hanging-Fruit-Findings.md modified_files: - lib/music_library_web/live_helpers/index_actions.ex - lib/music_library_web/live/collection_live/index.ex - lib/music_library_web/live/wishlist_live/index.ex - lib/music_library_web/live/scrobbled_tracks_live/index.ex - test/music_library_web/live/collection_live/index_test.exs - test/music_library_web/live/wishlist_live/index_test.exs - test/music_library_web/live/scrobbled_tracks_live/index_test.exs parent_task_id: ML-169 priority: medium ordinal: 31000 --- ## Description Add optimistic UI for delete operations only where the item is rendered from a LiveView stream on an index/list page. The item should be removed from the stream immediately, while the database delete runs through LiveView-owned async handling so failures can be reported and rolled back. ## Problem Index/list delete handlers currently wait for the database delete before updating the UI. `stream_delete` is used after the delete succeeds, so the interaction can feel slower than necessary. Additionally, both callers use `{:ok, _} =` pattern match on `Records.delete_record/1` and `ListeningStats.delete_track/1` — if either returns `{:error, changeset}`, the LiveView process crashes. Note: delete buttons already use `JS.hide(@hide_target)` before `JS.push("delete")` (RecordComponents line 708, scrobbled tracks line 145). This provides a client-side optimistic hide with no rollback — if the server delete fails, the row stays hidden despite the record still existing in DB. The server-side stream changes in this task provide the rollback mechanism. This task is intentionally scoped to stream-backed list views only: - Collection index and wishlist index record streams via `IndexActions.handle_delete/2` - Scrobbled tracks index stream Show-page deletes are out of scope. `CollectionLive.Show` and `WishlistLive.Show` navigate away after delete and do not have a list stream to optimistically update. Their failure and rollback behavior should remain synchronous unless a separate navigation-specific design is created. ## Files - `lib/music_library_web/live_helpers/index_actions.ex` — remove record from `:records` immediately and start a LiveView async delete operation - `lib/music_library_web/live/collection_live/index.ex` — handle async record delete success/failure for collection index - `lib/music_library_web/live/wishlist_live/index.ex` — handle async record delete success/failure for wishlist index - `lib/music_library_web/live/scrobbled_tracks_live/index.ex` — remove track from `:tracks` immediately and handle async track delete success/failure - `test/music_library_web/live/collection_live/index_test.exs` — add delete tests (currently zero coverage) - `test/music_library_web/live/wishlist_live/index_test.exs` — add delete tests (currently zero coverage) - `test/music_library_web/live/scrobbled_tracks_live/index_test.exs` — add delete tests (currently zero coverage) ## Fix Use LiveView-owned async handling (`start_async/3` plus `handle_async/3`, or an equivalent pattern that sends results back to the LiveView process). Do not use bare `Task.start/1`, because it cannot update the socket, reinsert the stream item, or show an error toast. Expected behavior: 1. Fetch the item needed for rollback. 2. Immediately call `stream_delete/3` on the relevant stream. 3. Start an async delete operation owned by the LiveView. 4. On success, leave the stream as-is and optionally broadcast/refresh any cross-view state the existing design requires. 5. On failure or async exit, reinsert the original item into the stream and show a user-facing error toast. For error text, use the project's existing user-facing error conventions (`ErrorMessages.friendly_message/1` where an error reason is shown). For `{:exit, reason}`, use a generic gettext string — the exit reason may contain stacktraces. ## handle_async signatures Per project conventions, `handle_async` always handles three cases: - `{:ok, {:ok, result}}` — delete succeeded, stream already clean, no-op - `{:ok, {:error, reason}}` — delete returned error, reinsert + toast with `ErrorMessages.friendly_message/1` - `{:exit, reason}` — task crashed, reinsert + generic error toast (do NOT leak `reason` to user) ## Acceptance Criteria - [x] #1 Index/list delete operations remove the item from the relevant stream before waiting for the database delete - [x] #2 Database delete still executes through LiveView-owned async handling, not bare `Task.start/1` - [x] #3 Async delete failure or exit reinserts the original item into the stream and shows an error toast - [x] #4 Existing delete confirmation prompts still work correctly - [x] #5 Show-page delete handlers are not changed as part of this task - [x] #6 Collection and wishlist index delete behavior remains shared through `IndexActions` where practical - [x] #7 Existing `data-confirm` browser prompts are not bypassed or altered - [x] #8 handle_async {:exit, reason} uses a generic gettext string, never leaks stacktrace reason to user - [x] #9 All three LiveViews have delete tests covering success, error, and exit paths ## Implementation Plan ## Implementation Plan ### 1. `IndexActions.handle_delete/2` — shared path (collection + wishlist) Replace the synchronous delete with optimistic `stream_delete` + `start_async`: ```elixir def handle_delete(socket, id) do record = Records.get_record!(id) socket = stream_delete(socket, :records, record) {:noreply, start_async(socket, {:delete_record, id}, fn -> Records.delete_record(record) end)} end ``` Key: `{:delete_record, id}` as async key ensures uniqueness. Do NOT call `load_and_assign_records` — `stream_delete` already removes the item from the stream; a full reload would negate the performance benefit. ### 2. `CollectionLive.Index` — handle_async callbacks Add three `handle_async` clauses after the existing `handle_async(:collection_summary, ...)` clauses: ```elixir @impl true def handle_async({:delete_record, id}, {:ok, {:ok, _record}}, socket) do {:noreply, socket} end def handle_async({:delete_record, id}, {:ok, {:error, reason}}, socket) do record = Records.get_record!(id) {:noreply, socket |> stream_insert(:records, record) |> put_toast(:error, gettext("Failed to delete: ") <> ErrorMessages.friendly_message(reason))} end def handle_async({:delete_record, id}, {:exit, _reason}, socket) do record = Records.get_record!(id) {:noreply, socket |> stream_insert(:records, record) |> put_toast(:error, gettext("Delete failed, please try again"))} end ``` Add `alias MusicLibraryWeb.ErrorMessages` to the module. ### 3. `WishlistLive.Index` — identical handle_async callbacks Duplicate the same three `handle_async` clauses from `CollectionLive.Index`. Per project conventions, extraction at 2 callers is premature; extract to `IndexActions` only when a third index page needs it. Add `alias MusicLibraryWeb.ErrorMessages` to the module. ### 4. `ScrobbledTracksLive.Index` — separate async path Replace the synchronous delete handler: ```elixir def handle_event("delete", %{"scrobbled-at-uts" => scrobbled_at_uts}, socket) do track = ListeningStats.get_track!(scrobbled_at_uts) stream_element = %{track: track} socket = stream_delete(socket, :tracks, stream_element) {:noreply, start_async(socket, {:delete_track, scrobbled_at_uts}, fn -> {ListeningStats.delete_track(track), stream_element} end)} end ``` Add three `handle_async` clauses: ```elixir @impl true def handle_async({:delete_track, _uts}, {:ok, {{:ok, _track}, _element}}, socket) do {:noreply, socket} end def handle_async({:delete_track, _uts}, {:ok, {{:error, reason}, element}}, socket) do {:noreply, socket |> stream_insert(:tracks, element) |> put_toast(:error, gettext("Failed to delete: ") <> ErrorMessages.friendly_message(reason))} end def handle_async({:delete_track, _uts}, {:exit, _reason}, socket) do {:noreply, put_toast(socket, :error, gettext("Delete failed, please try again"))} end ``` Add `alias MusicLibraryWeb.ErrorMessages` to the module. Key differences from records path: - Passes `stream_element` through async tuple so rollback uses the exact same map shape without re-running correlated subqueries (audit Finding 7) - `{:exit, _reason}` cannot recover the stream element — accept this limitation and show error toast only. A full `load_and_assign_tracks` reload would trigger the expensive correlated subqueries. ### 5. Tests **Existing coverage to update:** Three integration tests exist that test the old synchronous delete path. These must be updated to verify the new async-optimistic behavior (stream_delete + start_async): - `collection_live/index_test.exs` — "deletes a record from the listing" - `wishlist_live/index_test.exs` — "deletes a record from the listing" - `scrobbled_tracks_live/index_test.exs` — "deletes a scrobbled track from the listing" **New tests to add:** | # | Test | File | | --- | ------------------------------------------------------ | --------------------------------------------------- | | 1 | handle_async `{:ok, {:ok, ...}}` is no-op | `collection_live/index_test.exs` (unit test) | | 2 | handle_async `{:ok, {:error, ...}}` reinserts + toasts | `collection_live/index_test.exs` (unit test) | | 3 | handle_async `{:exit, ...}` reinserts + toasts | `collection_live/index_test.exs` (unit test) | | 4 | handle_async exit uses generic message (not reason) | `collection_live/index_test.exs` (unit test) | | 5 | `data-confirm` attribute present on delete buttons | `collection_live/index_test.exs` (AC #4 regression) | | 6 | Show page delete unchanged (still synchronous) | `collection_live/show_test.exs` (AC #5 regression) | Unit tests (tests 1-4): import the LiveView module, call `handle_async/3` directly with simulated results, assert socket state (stream contents, toast message). This is the preferred approach for testing failure paths since mocking `Repo.delete` is invasive. Test 4 specifically asserts that `{:exit, :killed}` produces a generic gettext string, not the raw exit reason — this prevents accidental stacktrace leakage to users. ## Implementation Notes Finished implementation and tests. All 1147 tests pass (0 failures). Changes: 1. IndexActions.handle_delete/2: optimistic stream_delete before start_async 2. CollectionLive.Index: 3 handle_async clauses + ErrorMessages alias 3. WishlistLive.Index: same 3 handle_async clauses + ErrorMessages alias 4. ScrobbledTracksLive.Index: reworked delete handler + 3 handle_async clauses 5. Tests: updated 3 existing delete integration tests (added render_async), added 12 new unit tests (4 per LiveView) covering success/error/exit paths Show page deletes untouched (verified via show_test.exs). ## Final Summary ## What changed Replaced synchronous delete handlers in three index/list LiveViews with optimistic `stream_delete` + LiveView-owned `start_async` / `handle_async` pattern: - **`IndexActions.handle_delete/2`** (shared by Collection + Wishlist): `stream_delete` immediately, then `start_async({:delete_record, id})` to run `Records.delete_record/1` - **`CollectionLive.Index`**: 3 new `handle_async({:delete_record, ...})` clauses — success no-op, error reinsert + toast via `ErrorMessages.friendly_message/1`, exit reinsert + generic gettext - **`WishlistLive.Index`**: identical 3 clauses (duplication intentional per project conventions — extract only at 3+ callers) - **`ScrobbledTracksLive.Index`**: reworked `handle_event("delete")` to pass `stream_element` through async tuple for zero-query rollback; 3 `handle_async({:delete_track, ...})` clauses (exit path can't recover element — toast only) Previously both callers used `{:ok, _} =` pattern match which would crash the LiveView process on `{:error, changeset}`. Now errors are handled gracefully. ## Tests - Updated 3 existing integration delete tests with `render_async()` to wait for async completion - Added 12 unit tests (4 per LiveView) for success/error/exit `handle_async` paths, including verification that `{:exit, reason}` never leaks stacktrace to user - Show-page delete handlers unchanged (verified via `show_test.exs`) - Full suite: 1147 tests pass, 0 failures ## Risks / follow-ups - `{:exit, _}` in record deletes calls `Records.get_record!(id)` for reinsertion — if the DB delete succeeded before the task crashed, this raises `Ecto.NoResultsError` crashing the LiveView. In practice `Records.delete_record/1` is simple (`Repo.delete` + `prune_artist_info_async`) so this is unlikely, but could be hardened with a rescue. - `{:exit, _}` in scrobbled tracks can't recover the stream element (toast-only) — acceptable tradeoff to avoid re-running expensive correlated subqueries.