cloud/music_library
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

ML-169.10: refine implementation plan

Browse Source
This commit is contained in:
Claudio Ortolina
2026-05-22 15:37:09 +01:00
parent ee4eecd1c6
commit 9f02e12414
1 changed files with 25 additions and 18 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backlog/tasks/ml-169.10 - Fix-Missing-optimistic-UI-for-delete-operations.md
+25 -18
View File
@@ -4,7 +4,7 @@ title: "Fix: Async optimistic UI for index/list delete streams"
status: To Do status: To Do
assignee: [] assignee: []
created_date: "2026-05-19 11:48" created_date: "2026-05-19 11:48"
updated_date: "2026-05-20 10:12" updated_date: "2026-05-22 14:33"
labels: labels:
- perf - perf
- fix - fix
@@ -102,7 +102,7 @@ Per project conventions, `handle_async` always handles three cases:
### 1. `IndexActions.handle_delete/2` — shared path (collection + wishlist) ### 1. `IndexActions.handle_delete/2` — shared path (collection + wishlist)
Replace the synchronous delete with optimistic stream_delete + start_async: Replace the synchronous delete with optimistic `stream_delete` + `start_async`:
```elixir ```elixir
def handle_delete(socket, id) do def handle_delete(socket, id) do
@@ -114,7 +114,7 @@ def handle_delete(socket, id) do
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. 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 ### 2. `CollectionLive.Index` — handle_async callbacks
@@ -143,12 +143,14 @@ def handle_async({:delete_record, id}, {:exit, _reason}, socket) do
end 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). Add `alias MusicLibraryWeb.ErrorMessages` to the module.
### 3. `WishlistLive.Index` — identical handle_async callbacks ### 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. 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 ### 4. `ScrobbledTracksLive.Index` — separate async path
Replace the synchronous delete handler: Replace the synchronous delete handler:
@@ -185,6 +187,8 @@ def handle_async({:delete_track, _uts}, {:exit, _reason}, socket) do
end end
``` ```
Add `alias MusicLibraryWeb.ErrorMessages` to the module.
Key differences from records path: 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) - Passes `stream_element` through async tuple so rollback uses the exact same map shape without re-running correlated subqueries (audit Finding 7)
@@ -192,24 +196,27 @@ Key differences from records path:
### 5. Tests ### 5. Tests
Currently **zero delete test coverage** across all three index test files. Add: **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 | | # | Test | File |
| --- | ------------------------------------------------------ | --------------------------------------------------- | | --- | ------------------------------------------------------ | --------------------------------------------------- |
| 1 | Delete record from collection grid (success) | `collection_live/index_test.exs` | | 1 | handle_async `{:ok, {:ok, ...}}` is no-op | `collection_live/index_test.exs` (unit test) |
| 2 | Delete record from wishlist (success) | `wishlist_live/index_test.exs` | | 2 | handle_async `{:ok, {:error, ...}}` reinserts + toasts | `collection_live/index_test.exs` (unit test) |
| 3 | Delete scrobbled track (success) | `scrobbled_tracks_live/index_test.exs` | | 3 | handle_async `{:exit, ...}` reinserts + toasts | `collection_live/index_test.exs` (unit test) |
| 4 | handle_async `{:ok, {:ok, ...}}` is no-op | `collection_live/index_test.exs` (unit test) | | 4 | handle_async exit uses generic message (not reason) | `collection_live/index_test.exs` (unit test) |
| 5 | handle_async `{:ok, {:error, ...}}` reinserts + toasts | `collection_live/index_test.exs` (unit test) | | 5 | `data-confirm` attribute present on delete buttons | `collection_live/index_test.exs` (AC #4 regression) |
| 6 | handle_async `{:exit, ...}` reinserts + toasts | `collection_live/index_test.exs` (unit test) | | 6 | Show page delete unchanged (still synchronous) | `collection_live/show_test.exs` (AC #5 regression) |
| 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 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.
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 4 specifically asserts that `{:exit, :killed}` produces a generic gettext string, not the raw exit reason — this prevents accidental stacktrace leakage to users.
Test 7 specifically asserts that `{:exit, :killed}` produces a generic gettext string, not the raw exit reason — this prevents accidental stacktrace leakage to users.
<!-- SECTION:PLAN:END --> <!-- SECTION:PLAN:END -->