From 004d2a23189866e4fd10da27505771d4557d88ef Mon Sep 17 00:00:00 2001 From: Claudio Ortolina Date: Wed, 20 May 2026 11:20:33 +0100 Subject: [PATCH] ML-169.10: refine implementation plan --- ...ing-optimistic-UI-for-delete-operations.md | 144 +++++++++++++++++- 1 file changed, 141 insertions(+), 3 deletions(-) diff --git a/backlog/tasks/ml-169.10 - Fix-Missing-optimistic-UI-for-delete-operations.md b/backlog/tasks/ml-169.10 - Fix-Missing-optimistic-UI-for-delete-operations.md index 9223b9d9..ef366bd4 100644 --- a/backlog/tasks/ml-169.10 - Fix-Missing-optimistic-UI-for-delete-operations.md +++ b/backlog/tasks/ml-169.10 - Fix-Missing-optimistic-UI-for-delete-operations.md @@ -4,7 +4,7 @@ title: "Fix: Async optimistic UI for index/list delete streams" status: To Do assignee: [] created_date: "2026-05-19 11:48" -updated_date: "2026-05-20 09:41" +updated_date: "2026-05-20 10:12" labels: - perf - fix @@ -19,6 +19,9 @@ modified_files: - 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 @@ -32,7 +35,9 @@ Add optimistic UI for delete operations only where the item is rendered from a L ## 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. +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: @@ -47,6 +52,9 @@ Show-page deletes are out of scope. `CollectionLive.Show` and `WishlistLive.Show - `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 @@ -60,8 +68,15 @@ Expected behavior: 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 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 @@ -74,4 +89,127 @@ For error text, use the project’s existing user-facing error conventions (`Err - [ ] #4 Existing delete confirmation prompts still work correctly - [ ] #5 Show-page delete handlers are not changed as part of this task - [ ] #6 Collection and wishlist index delete behavior remains shared through `IndexActions` where practical +- [ ] #7 Existing `data-confirm` browser prompts are not bypassed or altered +- [ ] #8 handle_async {:exit, reason} uses a generic gettext string, never leaks stacktrace reason to user +- [ ] #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. The async closure captures `record` — the LiveView task supervisor holds the reference. 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 +``` + +Note: Both error cases re-fetch via `Records.get_record!(id)`. For `{:error, reason}`, the record was NOT deleted so re-fetch succeeds. For `{:exit, _reason}`, the task crashed — if the record was deleted before the crash, `get_record!` raises `Ecto.NoResultsError` which crashes the LiveView (acceptable: better than silently showing wrong state). + +### 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. + +### 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 +``` + +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 + +Currently **zero delete test coverage** across all three index test files. Add: + +| # | Test | File | +| --- | ------------------------------------------------------ | --------------------------------------------------- | +| 1 | Delete record from collection grid (success) | `collection_live/index_test.exs` | +| 2 | Delete record from wishlist (success) | `wishlist_live/index_test.exs` | +| 3 | Delete scrobbled track (success) | `scrobbled_tracks_live/index_test.exs` | +| 4 | handle_async `{:ok, {:ok, ...}}` is no-op | `collection_live/index_test.exs` (unit test) | +| 5 | handle_async `{:ok, {:error, ...}}` reinserts + toasts | `collection_live/index_test.exs` (unit test) | +| 6 | handle_async `{:exit, ...}` reinserts + toasts | `collection_live/index_test.exs` (unit test) | +| 7 | handle_async exit uses generic message (not reason) | `collection_live/index_test.exs` (unit test) | +| 8 | `data-confirm` attribute present on delete buttons | `collection_live/index_test.exs` (AC #4 regression) | +| 9 | Show page delete unchanged (still synchronous) | `collection_live/show_test.exs` (AC #5 regression) | + +Integration tests (tests 1-3): click delete button inside dropdown, verify `refute_has` for the record/track ID, verify record is actually deleted from DB via context module. + +Unit tests (tests 4-7): 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 7 specifically asserts that `{:exit, :killed}` produces a generic gettext string, not the raw exit reason — this prevents accidental stacktrace leakage to users. + +