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

Backlog cleanup

Browse Source
This commit is contained in:
Claudio Ortolina
2026-05-25 09:33:08 +03:00
parent f347f56218
commit a3701de90e
5 changed files with 0 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backlog/completed/ml-169.10 - Fix-Missing-optimistic-UI-for-delete-operations.md
+266
View File
@@ -0,0 +1,266 @@
---
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
<!-- SECTION:DESCRIPTION:BEGIN -->
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)
<!-- SECTION:DESCRIPTION:END -->
## Acceptance Criteria
<!-- AC:BEGIN -->
- [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
<!-- AC:END -->
## Implementation Plan
<!-- SECTION:PLAN:BEGIN -->
## 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.
<!-- SECTION:PLAN:END -->
## Implementation Notes
<!-- SECTION:NOTES:BEGIN -->
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).
<!-- SECTION:NOTES:END -->
## Final Summary
<!-- SECTION:FINAL_SUMMARY:BEGIN -->
## 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.
<!-- SECTION:FINAL_SUMMARY:END -->
backlog/completed/ml-170 - Expose-additional-data-points-for-api-v1-collection-records.md
+344
View File
@@ -0,0 +1,344 @@
---
id: ML-170
title: Expose additional data points for api/v1/collection/* records
status: Done
assignee: []
created_date: "2026-05-08 13:02"
updated_date: "2026-05-22 12:20"
labels:
- api
- ready
dependencies: []
documentation:
- doc-14 - Research-Exposing-additional-data-points-for-collection-API.md
priority: medium
---
## Description
<!-- SECTION:DESCRIPTION:BEGIN -->
The current `/api/v1/collection/*` API endpoints return a subset of record fields from the SearchIndex virtual table. The following additional data points should be exposed to API consumers:
1. **Scrobble count** (if available) — number of times tracks from this record have been listened to (from Last.fm scrobble data in the `scrobbled_tracks` table)
2. **Last listened at date** — timestamp of the most recent scrobble for this record
3. **Collected release information** — details about specific releases (editions) that are part of the collection for each record/release group
4. **Artist country** — the country/area of the main artist, available via `MusicLibrary.Artists.ArtistInfo`
Currently, `CollectionJSON` maps SearchIndex fields into a flat JSON record with: id, type, format, musicbrainz_id, genres, release_date, purchased_at, artists (names only), title, and cover_url variants. None of the above data points are exposed.
The API endpoints affected are:
- `GET /api/v1/collection` (index, paginated)
- `GET /api/v1/collection/latest`
- `GET /api/v1/collection/random`
- `GET /api/v1/collection/on_this_day`
<!-- SECTION:DESCRIPTION:END -->
## Acceptance Criteria
<!-- AC:BEGIN -->
- [x] #1 `GET /api/v1/collection` (index) responses include `scrobble_count`, `last_listened_at`, `artist_country`, and `selected_release` for each record
- [x] #2 `GET /api/v1/collection/latest` response includes all four new fields
- [x] #3 `GET /api/v1/collection/random` response includes all four new fields
- [x] #4 `GET /api/v1/collection/on_this_day` responses include all four new fields for each record
- [x] #5 Records without scrobble data return `scrobble_count: 0` and `last_listened_at: null`
- [x] #6 Records without artist info return `artist_country: null`
- [x] #7 Records without a selected release return `selected_release: null`
- [x] #8 All existing tests pass (backward compatible — no existing field removed or renamed)
<!-- AC:END -->
## Implementation Plan
<!-- SECTION:PLAN:BEGIN -->
## Implementation Plan
### Overview
Add a `MusicLibrary.Collection.Enrichment` module that batch-hydrates SearchIndex (and Record struct) results with three new data dimensions (scrobble stats, artist country, selected release info) using fixed-count batch SQL queries. The enrichment layer runs in the controller after the initial query, before JSON rendering.
### Architecture impact analysis
| Touchpoint | Impact |
| ------------------------------------------------------------ | ------------------------------------------------------------------------------------------------------------------------------ |
| `lib/music_library/collection.ex` | New sub-module `MusicLibrary.Collection.Enrichment` |
| `lib/music_library_web/controllers/collection_json.ex` | Extended `record/1` to include new fields with nil-safe `Map.get/3` |
| `lib/music_library_web/controllers/collection_controller.ex` | Wire enrichment call between query and render in all 4 actions |
| Schemas (`SearchIndex`, `Record`) | **No changes** — enrichment works on maps/structs with `id`, `release_ids`, `selected_release_id`, and `artists` embedded list |
| Database | **No migrations** — reads existing `scrobbled_tracks`, `records`, `artist_infos`, `record_releases` |
| Routes | No changes |
| PubSub | No changes |
| Supervision tree | No changes |
| External APIs | No changes |
**Data source compatibility:** The 4 controller actions use different query paths. Verify each path produces results with the fields enrichment needs:
| Action | Query path | Result type | Has `release_ids`? | Has `artists`? | Has `selected_release_id`? |
| ------------- | --------------------------------- | -------------------- | ------------------------- | ----------------- | -------------------------- |
| `index` | `SearchIndex` via FTS5 | `SearchIndex` struct | ✅ yes (array of strings) | ✅ yes (embedded) | ✅ yes (string) |
| `latest` | `Record` via `essential_fields()` | Record struct / map | ✅ yes | ✅ yes | ✅ yes |
| `random` | same as `latest` | same | ✅ yes | ✅ yes | ✅ yes |
| `on_this_day` | `Record` via `essential_fields()` | Record struct / map | ✅ yes | ✅ yes | ✅ yes |
All paths produce results with the three required fields. Enrichment functions accept any map with those keys.
### Scope clarification
"Collected release information" means the **single selected release** per record (`selected_release_id` → details from `musicbrainz_data`), not the full list of all releases. The field is named `selected_release` in the API response.
---
### Step 1: Create `MusicLibrary.Collection.Enrichment` module
Create `lib/music_library/collection/enrichment.ex` with three public functions and a composition function:
```elixir
@spec enrich([map()]) :: [map()]
def enrich(records)
@spec enrich_scrobbles([map()]) :: [map()]
def enrich_scrobbles(records)
@spec enrich_artist_country([map()]) :: [map()]
def enrich_artist_country(records)
@spec enrich_selected_release([map()]) :: [map()]
def enrich_selected_release(records)
```
**Data flow:**
```
maps[] → enrich_scrobbles() → enrich_artist_country() → enrich_selected_release() → maps[]
```
Each function is independently callable and testable. `enrich/1` composes all three via pipeline.
**Verification:** Unit tests for each enrichment function verify correct field additions.
---
### Step 2: Implement `enrich_scrobbles/1`
**Algorithm:**
1. Collect all `release_ids` from all records into a flat, unique, non-empty list.
2. **Guard:** If the collected list is empty (all records have `release_ids: []` or `nil`), return records unchanged with `scrobble_count: 0, last_listened_at: nil` on each.
3. Single batch query against `scrobbled_tracks`:
```sql
SELECT json_extract(album, '$.musicbrainz_id') AS release_id,
COUNT(DISTINCT scrobbled_at_uts) AS scrobble_count,
MAX(scrobbled_at_uts) AS last_listened_at
FROM scrobbled_tracks
WHERE json_extract(album, '$.musicbrainz_id') IN (?)
GROUP BY 1
```
4. Build a lookup map `%{release_id => %{scrobble_count, last_listened_at}}`.
5. For each record, sum scrobble counts across its `release_ids` (a release group may have multiple releases, each with scrobbles) and take the `MAX` of `last_listened_at`.
6. For records with no matching release IDs in the lookup, `scrobble_count: 0, last_listened_at: nil`.
**No title+artist fallback.** The batch release_id lookup is sufficient. Records without scrobbles correctly show zero/null. If a stakeholder later requires title+artist matching, that is a separate follow-up with its own performance analysis — it would add N individual queries and break the fixed-query-count guarantee.
**Uses existing index:** `scrobbled_tracks_album_musicbrainz_id_index` on `json_extract(album, '$.musicbrainz_id')`.
**Converts `last_listened_at` to ISO8601 string** after the query (UNIX timestamp → `DateTime.from_unix!` → `DateTime.to_iso8601`).
**Edge case — shared release_ids across records:** Two records may share the same release group (different formats). This is correctly handled: the lookup map is per release_id, and each record independently sums across its own `release_ids`.
**Verification:**
- Records with scrobble matches → `scrobble_count > 0`, `last_listened_at` is a non-nil ISO8601 string
- Records with no scrobbles → `scrobble_count: 0`, `last_listened_at: nil`
- Records with empty/nil `release_ids` → `scrobble_count: 0`, `last_listened_at: nil`
- Two records sharing a release group → both correctly attribute scrobbles
- Verify `last_listened_at` is the most recent timestamp across all releases in the group
---
### Step 3: Implement `enrich_artist_country/1`
**Algorithm:**
1. Extract main artist `musicbrainz_id` from each record's `artists` embedded list (first element only — `hd(record.artists)`).
2. Collect unique non-nil artist IDs.
3. **Guard:** If no artist IDs, return records unchanged with `artist_country: nil`.
4. Batch query `artist_infos` (primary key lookup — uses existing PK index):
```sql
SELECT id, musicbrainz_data
FROM artist_infos
WHERE id IN (?)
```
5. For each row, extract country using `MusicLibrary.Artists.ArtistInfo.country/1` (parses `musicbrainz_data.area.iso-3166-1-codes`, falls back to `musicbrainz_data.country`, returns `%{name: "World", code: "XW"}` if no area data).
6. Build lookup map `%{artist_mbid => %{name: country_name, code: country_code}}`.
7. Add `artist_country` (map, or nil if no `artist_infos` row exists for that artist).
**Performance note:** `artist_infos.musicbrainz_data` is a full MusicBrainz JSON blob. Loading it for up to 20 unique artists per page is acceptable. The JSON extraction (`ArtistInfo.country/1`) is in Elixir, not SQL — correct, as the extraction logic (iso-3166-1-codes vs iso-3166-2-codes vs country fallback) is non-trivial.
**Verification:**
- Artist with country data → `artist_country: %{name: "...", code: "..."}`
- Artist with no `artist_infos` row → `artist_country: nil`
- Record with empty artists list → `artist_country: nil`
- Multiple records with same main artist → reuse single lookup entry (dedup works)
---
### Step 4: Implement `enrich_selected_release/1`
**Algorithm:**
1. Collect record IDs where `selected_release_id` is not nil and not empty string.
2. **Guard:** If no record IDs qualify, return records unchanged with `selected_release: nil`.
3. Batch query `records` (primary key lookup — uses existing PK index):
```sql
SELECT id, musicbrainz_data, selected_release_id
FROM records
WHERE id IN (?)
```
4. For each record, extract the selected release from `musicbrainz_data` using `Record.releases/1` + `Record.find_release/2`.
5. Fields to expose from the selected release:
- `format` — derived via `ReleaseSearchResult.format(selected_release)` (atoms: `:cd`, `:vinyl`, `:dvd`, `:blu_ray`, `:digital_download`, `:vhs`, `:multi`, `:unknown`), converted to string with `to_string/1` for the API response
- `date` — release date string
- `country` — country code (e.g., "US", "GB", "XW")
- `catalog_number` — catalog number string
- `packaging` — packaging description
- `disambiguation` — disambiguation comment
6. Add `selected_release` (map or nil) to each record.
**Performance note:** `records.musicbrainz_data` is a large JSON blob (full release group with releases, tracks, media). Loading it for up to 20 records per page is acceptable. The plan does not attempt to extract the selected release in SQL — this is correct, as `Record.releases/1` and `Record.find_release/2` contain release ordering logic (by date, country) that would be impractical to replicate in SQL.
**Verification:**
- Record with valid `selected_release_id` that exists in `musicbrainz_data` → all 6 fields populated
- Record with `selected_release_id` that doesn't match any release in `musicbrainz_data` → `selected_release: nil`
- Record with nil/empty `selected_release_id` → `selected_release: nil`
- Record not in the batch query (not in DB) → `selected_release: nil`
---
### Step 5: Update `CollectionJSON`
Extend the `record/1` function to include new fields using `Map.get/3` with safe defaults:
```elixir
defp record(record) do
%{
# ... existing fields unchanged ...
scrobble_count: Map.get(record, :scrobble_count, 0),
last_listened_at: Map.get(record, :last_listened_at),
artist_country: Map.get(record, :artist_country),
selected_release: Map.get(record, :selected_release)
}
end
```
Using `Map.get/3` ensures backward compatibility: the function works with both raw SearchIndex structs (no enrichment) and enriched maps. No existing fields are removed or renamed.
**Verification:** Controller tests validate the new JSON shape.
---
### Step 6: Update `CollectionController`
Wire enrichment into all four actions. The pattern is:
```elixir
records = Collection.search_records(...)
enriched = Collection.Enrichment.enrich(records)
render(conn, :index, records: enriched, total: total, ...)
```
**Affected actions:**
- `index` — enrich paginated results before render
- `latest` — enrich single record before render (wrap in list, enrich, unwrap)
- `random` — enrich single record before render (same pattern)
- `on_this_day` — enrich results list before render
**Verification:** Integration tests for each endpoint confirm new fields in response.
---
### Step 7: Write tests
**Unit tests** (`test/music_library/collection/enrichment_test.exs`):
- `enrich_scrobbles/1` — with matches, without matches, empty release_ids, shared release_ids
- `enrich_artist_country/1` — with country, without artist_info, without artists
- `enrich_selected_release/1` — with selected release, without selected_release_id, with non-matching release_id
- `enrich/1` — composition of all three, verifying fields accumulate correctly
**Controller tests** (update `test/music_library_web/controllers/collection_controller_test.exs`):
- Update `expected_record_json/1` to include new fields with expected shapes
- Test nil/null values for records without enrichment data
- Test all 4 endpoints (`index`, `latest`, `random`, `on_this_day`) include the new fields
**End-to-end test** — verify the exact API response JSON shape for each of the 4 endpoints with records that have enrichment data (requires fixture setup with scrobbled_tracks, artist_infos, and records with musicbrainz_data).
**Verification:** `mix test test/music_library/collection/enrichment_test.exs test/music_library_web/controllers/collection_controller_test.exs`
---
### Performance profile
| Operation | Query count | Complexity |
| ------------------------- | ----------------------------------------- | -------------------------------------------------------------- |
| `enrich_scrobbles` | 1 batch query | O(R + T) where R=records per page, T=matching scrobbled_tracks |
| `enrich_artist_country` | 1 batch query | O(A) where A=unique artists per page |
| `enrich_selected_release` | 1 batch query + in-memory JSON extraction | O(R) |
| **Total per request** | **exactly 3 queries** | All fixed-cost regardless of total table size |
No N+1 risk — all batch queries scale with page size (≤20 records), not table size. No title+artist fallback queries.
**Memory:** All enrichment data fits in maps proportional to page size. `musicbrainz_data` JSON blobs are loaded for up to 20 records and 20 artists, then discarded after field extraction. At typical MusicBrainz payload sizes (~50-200KB per release group), worst-case memory is under 10MB per request.
**Indexes used:**
- `scrobbled_tracks_album_musicbrainz_id_index` for scrobble batch query (index seek, not table scan)
- `artist_infos` primary key for artist country lookup
- `records` primary key for selected release lookup
No new indexes needed.
---
### Production Changes
None required. This is a read-only change that adds fields to existing API responses. No migrations, no environment variables, no service provisioning.
**Rollback:** Remove the enrichment call from controllers (one line per action). The `CollectionJSON.record/1` function gracefully handles missing enrichment fields via `Map.get/3` defaults.
### Documentation updates
- **`docs/architecture.md`**: Add `MusicLibrary.Collection.Enrichment` to the Contexts + Modules section under `MusicLibrary.Collection`. Note that collection API responses include scrobble stats, artist country, and selected release data.
- **API response documentation**: If any README or doc page describes the `/api/v1/collection` response shape, update it to document the four new fields:
- `scrobble_count` (integer, always present, defaults to 0)
- `last_listened_at` (ISO8601 string or null)
- `artist_country` (`{name: String, code: String}` or null — country of the main artist)
- `selected_release` (`{format, date, country, catalog_number, packaging, disambiguation}` or null)
- **No changes** to `docs/project-conventions.md`, `docs/production-infrastructure.md`, or README needed.
### `selected_release.format` (revised 2026-05-11)
The `release_summary` component in `record_components.ex` already derives a release's format from its media via `ReleaseSearchResult.format/1`. This function accepts any struct with a `.media` field whose items have a `.format` field — which includes both `%ReleaseSearchResult{}` and `%MusicBrainz.Release{}` (structural typing). It returns atoms: `:cd`, `:vinyl`, `:dvd`, `:blu_ray`, `:digital_download`, `:vhs`, `:multi`, `:unknown`.
**Simplify Step 4**: instead of building a custom format map, call `ReleaseSearchResult.format(selected_release)` on the release returned by `Record.find_release/2`. Convert the result to a string with `to_string/1` for the API response. No new format parsing code needed.
<!-- SECTION:PLAN:END -->
## Final Summary
<!-- SECTION:FINAL_SUMMARY:BEGIN -->
Added `MusicLibrary.Collection.Enrichment` module that batch-hydrates collection API results with scrobble stats, artist country, and selected release data using 3 fixed-count queries per request. Extended `CollectionJSON.record/1` with four new fields (`scrobble_count`, `last_listened_at`, `artist_country`, `selected_release`) using `Map.get/3` for nil-safety. Wired enrichment into all four endpoints (`index`, `latest`, `random`, `on_this_day`). 19 new unit tests + updated controller tests. Full test suite passes (1134 tests, no regressions).
<!-- SECTION:FINAL_SUMMARY:END -->
backlog/completed/ml-181 - Harden-public-asset-transforms-against-unbounded-resource-consumption-and-cache-amplification.md
+278
View File
@@ -0,0 +1,278 @@
---
id: ML-181
title: >-
Harden public asset transforms against unbounded resource consumption and
cache amplification
status: Done
assignee: []
created_date: "2026-05-13 18:33"
updated_date: "2026-05-22 06:52"
labels:
- security
- api
- ui
- ready
dependencies: []
references:
- doc-19 - GPT-5.5-Security-Review.md
- >-
backlog/completed/ml-35 -
Harden-the-public-asset-endpoint-against-invalid-payloads.md
- doc-21 - ML-181-Research-Public-Asset-Transform-Hardening-Routes.md
modified_files:
- lib/music_library/assets/transform.ex
- lib/music_library/assets/cache.ex
- lib/music_library_web/controllers/asset_controller.ex
- test/music_library/assets/transform_test.exs
- test/music_library_web/controllers/asset_controller_test.exs
priority: medium
ordinal: 9000
---
## Description
<!-- SECTION:DESCRIPTION:BEGIN -->
The unauthenticated `GET /public/assets/:transform_payload` endpoint decodes arbitrary JSON into a `%Transform{}` struct without validating `width` type, range, or maximum. An attacker with a valid asset hash (obtainable from public email image URLs) can craft unlimited variant payloads, forcing repeated native libvips resize attempts and amplifying ETS cache entries. Prior hardening (ml-35, commit `92a36b91`) fixed crashes from malformed payloads but left resource-control validation open.
<!-- SECTION:DESCRIPTION:END -->
## Acceptance Criteria
<!-- AC:BEGIN -->
- [x] #1 Transform.decode/1 rejects payloads with string width (400 Bad Request)
- [x] #2 Transform.decode/1 rejects payloads with negative width (400 Bad Request)
- [x] #3 Transform.decode/1 rejects payloads with zero width (400 Bad Request)
- [x] #4 Transform.decode/1 rejects payloads with float width (400 Bad Request)
- [x] #5 Transform.decode/1 rejects payloads with width > 2048 (400 Bad Request)
- [x] #6 Transform.decode/1 accepts width: nil (original size, no resize)
- [x] #7 Transform.decode/1 accepts widths in 1..2048 (e.g., 96, 150, 480, 300, 2048)
- [x] #8 Canonical cache key collates variant payloads for the same (hash, width) into a single ETS entry
- [x] #9 Existing email image URLs (width: 96) continue to serve correctly
- [x] #10 API and authenticated asset routes continue to work unchanged
- [x] #11 Existing controller and LiveView tests pass without modification
<!-- AC:END -->
## Implementation Plan
<!-- SECTION:PLAN:BEGIN -->
## Objective Alignment
This plan addresses F1 from the security review by adding two complementary defenses:
1. **Width validation in `Transform.decode/1` and `decode!/1`** — Rejects payloads where `width` is not `nil` or a positive integer in range `1..2048`, preventing unbounded libvips thumbnail operations.
2. **Canonical cache key derivation** — Derives ETS cache keys from the validated `%Transform{hash, width}` struct instead of the raw payload string, eliminating cache amplification from variant JSON representations. HTTP ETag remains the raw payload to preserve existing test assertions and browser caching behavior.
Together these close both the resource consumption vector (arbitrary widths → native image processing) and the cache amplification vector (variant payload strings → unbounded ETS entries).
## Alternatives Considered
See `doc-21 - ML-181-Research-Public-Asset-Transform-Hardening-Routes.md` for full analysis. Route 3 (Combined) was selected as the simplest approach that fully addresses the finding. Route 4 (payload signing) is deferred as overkill for the single-operator threat model.
## Implementation Steps
### Step 1: Add width validation to `Transform.decode/1` and `decode!/1`
**File:** `lib/music_library/assets/transform.ex`
Add a private `valid_width?/1` guard that checks:
- `nil` → valid (no resize, serve original)
- Positive integer in `1..2048` → valid
- Anything else (string, negative, zero, float, very large) → invalid
Update `decode/1` to call `valid_width?/1` after parsing params, before constructing the struct. Return `{:error, :invalid_payload}` on validation failure.
Update `decode!/1` to delegate to `decode/1` and raise on the error tuple, keeping the `!` convention consistent:
```elixir
def decode!(payload) do
case decode(payload) do
{:ok, transform} -> transform
{:error, :invalid_payload} -> raise ArgumentError, "invalid transform payload"
end
end
```
This replaces the current `decode!/1` body which manually decodes base64/JSON without validation.
**Dependencies:** None
**Verification:**
- Run doctests: `mix test test/music_library/assets/transform_test.exs`
- The existing doctest (`width: 300`) still passes (300 is in range)
- The `decode!/1` doctest still passes (same valid payload now flows through validation)
- New unit tests (see Step 4) confirm rejection of edge cases
### Step 2: Add `canonical_key/1` to `Transform` module
**File:** `lib/music_library/assets/transform.ex`
Add a public function with a doctest:
```elixir
@doc """
iex> Transform.canonical_key(%Transform{hash: "abc123", width: 96})
"abc123:96"
"""
@spec canonical_key(t()) :: String.t()
def canonical_key(%__MODULE__{hash: hash, width: width}), do: "#{hash}:#{width}"
```
**Dependencies:** Step 1 (struct is now guaranteed valid)
**Verification:** Doctest confirms deterministic output
### Step 3: Update `AssetController` to use canonical cache key (ETS only)
**File:** `lib/music_library_web/controllers/asset_controller.ex`
Changes:
1. In `show/2`, after successful decode, compute `cache_key = Transform.canonical_key(transform)`
2. Pass `cache_key` (not `payload`) as the first argument to `cached_get/3`
3. In `cached_get/3`, use the first argument for `Cache.get/2` and `Cache.set/3` — rename the parameter from `payload` to `cache_key` for clarity
4. Keep `payload` as the HTTP ETag value (unchanged) — the `if-none-match` comparison in `show/2` and the `respond_with_cache/5` call continue to use the raw `payload` string
The separation is:
- **ETS cache key:** canonical `"hash:width"` — prevents cache amplification
- **HTTP ETag:** raw payload string — preserves existing test assertions on `get_resp_header(conn, "etag")`
**Dependencies:** Steps 1 and 2
**Verification:**
- Existing controller tests pass without modification: `mix test test/music_library_web/controllers/asset_controller_test.exs`
- ETag assertions all check against the raw `payload`, which is unchanged
- Existing LiveView tests pass (they use Transform for cover URLs): `mix test test/music_library_web/live/collection_live/`
### Step 4: Add tests
**File:** `test/music_library/assets/transform_test.exs`
Add tests for `decode/1` with invalid widths:
- String width: `%{hash: "abc", width: "300"}` → `{:error, :invalid_payload}`
- Negative width: `%{hash: "abc", width: -1}` → `{:error, :invalid_payload}`
- Zero width: `%{hash: "abc", width: 0}` → `{:error, :invalid_payload}`
- Float width: `%{hash: "abc", width: 300.5}` → `{:error, :invalid_payload}`
- Very large width: `%{hash: "abc", width: 99999}` → `{:error, :invalid_payload}`
- Nil width: `%{hash: "abc", width: nil}` → valid
- Max allowed width: `%{hash: "abc", width: 2048}` → valid
Add a doctest for `canonical_key/1` (included in Step 2 above).
**File:** `test/music_library_web/controllers/asset_controller_test.exs`
Add integration tests:
- Payload with string width returns 400
- Payload with negative width returns 400
- Payload with very large width returns 400
Add test for cache amplification fix:
- Two variant payloads encoding the same (hash, width) produce the same cache entry (verify by checking that the second request does not trigger a new resize — either inspect ETS entry count or verify response timing)
**Dependencies:** Steps 1-3
**Verification:** `mix test` — all tests pass
### Step 5: Update inline module documentation
**File:** `lib/music_library/assets/transform.ex`
Update `@moduledoc` to document:
- The width validation rules: `nil` (original size) or positive integer `1..2048`
- The `canonical_key/1` function and its purpose for cache deduplication
**File:** `lib/music_library/assets/cache.ex`
Update the "Cache key" section of `@moduledoc`:
- Remove: "where `payload` is a transform parameter string (encoding width and asset hash)"
- Replace with: "where the key string is an opaque canonical transform key provided by the caller (currently `"hash:width"` from `Transform.canonical_key/1`)"
This reflects that Cache no longer knows or cares about the key's origin — it's just an opaque string.
**Dependencies:** Steps 1-3
**Verification:** Review updated doc output with `h MusicLibrary.Assets.Transform` and `h MusicLibrary.Assets.Cache`
## Architecture Impact Analysis
| Touchpoint | Impact |
| --------------------------------------- | --------------------------------------------------------------------------------------- |
| `MusicLibrary.Assets.Transform` | Adds validation to `decode/1` and `decode!/1` + new `canonical_key/1` function |
| `MusicLibraryWeb.AssetController` | Uses canonical key for ETS Cache; HTTP ETag continues to use raw payload |
| `MusicLibrary.Assets.Cache` | No code change — key format remains `{string, format}`, but the string is now canonical |
| `MusicLibrary.Assets.Image` | No change |
| `MusicLibraryWeb.RecordsOnThisDayEmail` | No change — already uses valid width (96) |
| `MusicLibraryWeb.CollectionJSON` | No change — uses valid widths (nil, 150, 480) |
| All LiveViews using Transform | No change — no width or valid widths only |
| Routes | No change |
| ETS cache | Existing entries invalidated on deploy restart (in-memory, expected) |
| HTTP caching (ETag/If-None-Match) | No change — raw payload remains the ETag, preserving existing browser-cached responses |
| External APIs | Not affected |
## Performance Profile
- **`valid_width?/1`:** O(1) — two guard checks (`is_nil` and `is_integer and > 0 and ≤ 2048`)
- **`canonical_key/1`:** O(1) — string concatenation of two fields
- **Cache lookup:** Unchanged — single `:ets.lookup/2` per request
- **Memory:** Reduced — canonical keys collapse variant payloads into single entries
- **N+1 risk:** None — no new queries introduced
- **Latency impact:** Negligible (< 1µs for validation + key concatenation)
## Benchmarking Requirements
No benchmarks needed. The change removes work (fewer cache entries, early rejection of invalid payloads) rather than adding it.
## Cost Profile
No cost impact — no paid resources are consumed.
## Production Infrastructure Steps
No production changes required:
- No environment variables
- No database migrations
- No service provisioning
- ETS cache is in-memory and auto-clears on application restart during deploy
- No DNS, firewall, or Coolify configuration changes
## Documentation Updates
- **`doc-21`** (already created): Contains the full research analysis and route comparison
- **`Transform` `@moduledoc`**: Updated in Step 5 to document width validation rules and `canonical_key/1`
- **`Cache` `@moduledoc`**: Updated in Step 5 to describe the key as opaque rather than raw-payload-specific
- **`docs/architecture.md`**: No update needed — asset serving path is not documented at this granularity
- **`docs/project-conventions.md`**: No update needed
<!-- SECTION:PLAN:END -->
## Final Summary
<!-- SECTION:FINAL_SUMMARY:BEGIN -->
### Summary
Added two complementary defenses to the public asset transform endpoint:
1. **Width validation in `Transform.decode/1` and `decode!/1`** — Rejects payloads where `width` is not `nil` or a positive integer in `1..2048`. Invalid payloads return `{:error, :invalid_payload}` (400 Bad Request at the controller level). A private `valid_width?/1` guard handles nil, valid integer range, and all invalid cases (string, negative, zero, float, very large).
2. **Canonical cache key derivation** — Added `Transform.canonical_key/1` producing `"hash:width"` strings. `AssetController` now uses this as the ETS cache key while continuing to use the raw payload string for HTTP ETags. This collapses variant JSON payloads encoding the same (hash, width) into a single cache entry.
### Changes
- `lib/music_library/assets/transform.ex` — Added `@max_width 2048`, `valid_width?/1`, `canonical_key/1`, updated `decode/1`, `decode!/1`, and `@moduledoc`
- `lib/music_library/assets/cache.ex` — Updated `@moduledoc` Cache key section to describe key as opaque
- `lib/music_library_web/controllers/asset_controller.ex` — Uses `Transform.canonical_key/1` for ETS cache; HTTP ETag remains raw payload
- `test/music_library/assets/transform_test.exs` — 8 new unit tests for width validation edge cases + canonical_key doctest
- `test/music_library_web/controllers/asset_controller_test.exs` — 4 new integration tests (string/negative/large width rejection, cache amplification fix)
### Verification
- Full test suite: **1115 passed** (44 doctests, 1071 tests), 0 failures
- Existing controller and LiveView tests pass without modification
<!-- SECTION:FINAL_SUMMARY:END -->
backlog/completed/ml-182 - Analyze-and-plan-elimination-of-LiveViewTest-usage-in-favor-of-PhoenixTest.md
+77
View File
@@ -0,0 +1,77 @@
---
id: ML-182
title: Analyze and plan elimination of LiveViewTest usage in favor of PhoenixTest
status: Done
assignee: []
created_date: "2026-05-14 21:40"
updated_date: "2026-05-23 06:17"
labels:
- testing
- refactoring
- analysis
dependencies: []
references:
- >-
test/support/conn_case.ex (shows auto-imported LiveViewTest subset and
PhoenixTest)
documentation:
- "https://hexdocs.pm/phoenix_test/PhoenixTest.html"
- "https://hexdocs.pm/phoenix_live_view/Phoenix.LiveViewTest.html"
modified_files:
- test/music_library_web/components/chat_test.exs
- test/music_library_web/components/release_test.exs
- test/music_library_web/live/artist_live/show_test.exs
- test/music_library_web/live/collection_live/index_test.exs
- test/music_library_web/live/maintenance_live/index_test.exs
- test/music_library_web/live/online_store_template_live/index_test.exs
- test/music_library_web/live/record_set_live/index_test.exs
- test/music_library_web/live/record_set_live/show_test.exs
- test/music_library_web/live/scrobble_live/index_test.exs
- test/music_library_web/live/scrobble_live/release_group_show_test.exs
- test/music_library_web/live/scrobble_live/release_show_test.exs
- test/music_library_web/live/scrobble_rules_live/index_test.exs
- test/music_library_web/live/scrobbled_tracks_live/index_test.exs
- test/music_library_web/live/scrobbled_tracks_live/rule_picker_test.exs
- test/music_library_web/live/stats_live/top_albums_test.exs
- test/music_library_web/live/wishlist_live/index_test.exs
- test/music_library_web/live_helpers/record_actions_test.exs
priority: medium
ordinal: 10000
---
## Description
<!-- SECTION:DESCRIPTION:BEGIN -->
Audit all LiveView and component tests that mix LiveViewTest and PhoenixTest, and eliminate LiveViewTest usage where possible. The project's ConnCase auto-imports both frameworks, creating unnecessary dual-framework complexity.
The work is broken into 6 waves by difficulty, each tracked as a subtask.
<!-- SECTION:DESCRIPTION:END -->
## Acceptance Criteria
<!-- AC:BEGIN -->
- [x] #1 All 17 files are classified as fully eliminable, partially eliminable, or blocked with clear rationale for each
- [x] #2 Three blocking patterns (send_update/3, send(view.pid), live_isolated) are documented with suggested workarounds
- [x] #3 A concrete migration order is proposed, prioritizing low-risk fully-eliminable files first
- [x] #4 Cost estimates (effort level: low/medium/high) are assigned to each file or group of files
<!-- AC:END -->
## Implementation Notes
<!-- SECTION:NOTES:BEGIN -->
## Key challenges discovered during execution
1. **PhoenixTest can't click `<span>` elements** (Fluxon `.badge`) — only `<a>` (`click_link`) and `<button>` (`click_button`). HTML changes needed for badge-based interactions.
2. **Fluxon input labels** using `data-part="field"` wrapper without separate visible text labels don't match `fill_in`. Workaround: visit with query params to trigger `handle_params` search.
3. **LiveComponent modals with `phx-target`** don't respond to URL query params — need actual form interaction, which is harder with PhoenixTest when labels are missing.
4. **`trigger_hook` in PhoenixTest** expects JSON-encoded values, not Elixir maps. Different API from LiveViewTest's `render_hook`.
5. **`render_async/1` is auto-imported by ConnCase** — can use `unwrap(&render_async/1)` without any explicit import. This is the only LiveViewTest function that remains needed for async data loading.
<!-- SECTION:NOTES:END -->
backlog/completed/ml-196 - Add-pi-template-to-work-on-application-task.md
+65
View File
@@ -0,0 +1,65 @@
---
id: ML-196
title: Add pi template to work on application task
status: Done
assignee: []
created_date: "2026-05-22 09:40"
updated_date: "2026-05-22 14:12"
labels: []
dependencies: []
ordinal: 33000
---
## Description
<!-- SECTION:DESCRIPTION:BEGIN -->
- Solicit reading architecture
- Solicit loading relevant skills
- Make sure it'd done, and not complete
<!-- SECTION:DESCRIPTION:END -->
## Acceptance Criteria
<!-- AC:BEGIN -->
- [x] #1 Template file exists at .pi/prompts/application-task.md with correct frontmatter (description + argument-hint)
- [x] #2 Template instructs agent to read docs/architecture.md and docs/project-conventions.md before writing code without summarizing their contents
- [x] #3 Template instructs agent to load relevant skills based on their descriptions and triggers, without hardcoding a domain→skill mapping
- [x] #4 Template distinguishes Done status from task_complete/task_archive per Backlog.md finalization workflow
- [x] #5 Template covers the full Backlog.md execution workflow: pre-flight → present plan → work loops → finalization → after finalization
<!-- AC:END -->
## Implementation Plan
<!-- SECTION:PLAN:BEGIN -->
Create `.pi/prompts/application-task.md` that:
1. **Solicits architecture reading** — instructs the agent to read `docs/architecture.md` and `docs/project-conventions.md` before touching code
2. **Solicits loading relevant skills** — maps task domains (database, UI, testing, workers, APIs, etc.) to their corresponding `.agents/skills/*/SKILL.md` files and instructs the agent to load all relevant ones
3. **Distinguishes done from complete** — follows the Backlog.md Task Finalization workflow: verify AC, verify DoD, run tests, write Final Summary, set status to "Done". Explicitly warns against calling `task_complete` or `task_archive` for marking work finished.
4. **Follows Backlog.md execution workflow** — mark In Progress, present plan, work in short loops, log progress, handle scope changes, finalize properly
The template uses `argument-hint: "<TASK-ID>"` for autocomplete compatibility with the `/application-task <ID>` invocation pattern.
Updated pre-flight after review: point 1 no longer summarizes doc contents, point 3 no longer hardcodes a domain→skill mapping — instead instructs agent to load skills based on their own descriptions and triggers.
<!-- SECTION:PLAN:END -->
## Final Summary
<!-- SECTION:FINAL_SUMMARY:BEGIN -->
Created `.pi/prompts/application-task.md` — a pi prompt template for working on Backlog.md tasks end-to-end.
**What it does:**
- **Pre-flight phase**: instructs the agent to read `docs/architecture.md` and `docs/project-conventions.md` before writing code, load the task plan from Backlog.md, and identify + read all relevant `.agents/skills/` files based on the task's domain (database, UI, testing, workers, APIs, etc.)
- **Execution phase**: follows the Backlog.md Task Execution workflow — mark In Progress, present plan for approval, work in short loops, log progress, handle scope changes
- **Done vs Complete distinction**: explicitly warns against `task_complete`/`task_archive` and follows the Backlog.md Task Finalization workflow (verify AC, verify DoD, run tests, write Final Summary, set status to "Done")
- **After finalization**: reminders about not autonomously creating tasks and proper subtask handoff
**Template**: invocable as `/application-task <TASK-ID>` via pi's prompt template system.
<!-- SECTION:FINAL_SUMMARY:END -->