--- id: ML-169 title: "Comprehensive application audit: functional correctness and performance" status: Done assignee: [] created_date: "2026-05-08 08:46" updated_date: "2026-05-19 08:50" labels: - audit dependencies: [] references: - docs/architecture.md - docs/project-conventions.md - >- backlog/tasks/ml-168 - Update-wishlist-index-and-collection-index-when-background-import-finishes.md priority: medium ordinal: 1000 --- ## Description Large-scale audit of the application covering two dimensions: **1. Functional Issues**: Unhandled async messages from LiveComponents, stale PubSub subscriptions, unexpected state changes from concurrent operations (background workers updating records while user is viewing them). **2. Performance Low-Hanging Fruit**: Unnecessary server round trips, missing opportunities for optimistic updates, redundant data reloads, and synchronous blocking work in mount. The audit is divided into 4 independent phases that can be tackled in parallel or sequentially. Each phase produces findings that feed into fix tasks. Full implementation plan in the task plan section. ## Acceptance Criteria - [x] #1 Phase 1 (async messages): all LiveComponents that send messages to parent LiveViews have matching handle_info clauses documented; missing handlers identified with severity - [x] #2 Phase 2 (PubSub lifecycle): all subscribe/unsubscribe pairs verified correct; stale subscription risks identified; ML-168 gap separately accounted for - [x] #3 Phase 3 (state change safety): concurrent record-mutation patterns audited; {:update, record} guards and manage_subscription patterns verified across all navigation paths - [x] #4 Phase 4 (performance): unnecessary round trips, redundant full-stream reloads, and synchronous mount work identified with estimated impact and fix approach - [x] #5 Each phase produces a written finding report with file paths, line references, severity ratings, and recommended fixes ## Implementation Plan # Audit Implementation Plan ## Audit Scope This audit covers all 14 LiveViews, all LiveComponents that send messages to parents, all PubSub subscription points, and all Oban worker-LiveView interaction patterns. The full architecture is documented in `docs/architecture.md`. ## Phase Structure Each phase is a standalone investigation that produces a findings document. Fixes are NOT implemented in these phases — only identification, severity rating, and fix recommendations. --- ## Phase 1: Async Message Audit (LiveComponent → Parent) **Goal**: Verify every LiveComponent message has a matching `handle_info` in its parent LiveView. **Known message producers and their parents**: | Component | Messages sent | Consumer LiveViews | Status | | ------------------------------ | -------------------------------------------------------- | -------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------- | | `RecordForm` | `{:saved, record}` | CollectionLive.Index, WishlistLive.Index, CollectionLive.Show, WishlistLive.Show | ✅ All four have handlers | | `AddRecord` | `{:imported_single, record}`, `{:imported_async, count}` | CollectionLive.Index, WishlistLive.Index | ✅ Both have handlers | | `Chat` | `:chats_changed` | CollectionLive.Index, CollectionLive.Show, WishlistLive.Show, ArtistLive.Show | ✅ All four have handlers | | `Release` | `{tag, release}` (dynamic tag) | ScrobbleLive.ReleaseShow, CollectionLive.Show | ⚠️ CollectionLive.Show renders Release but has NO `handle_info({:release_loaded, _})` — needs investigation | | `ArtistLive.Form` | `{:saved, artist_info}` | ArtistLive.Show | ✅ Has handler | | `RecordSetLive.Form` | `{:created, _}`, `{:updated, _}` | RecordSetLive.Index | ✅ Has handlers | | `RecordSetLive.RecordPicker` | `{:added, record_set}` | RecordSetLive.Index | ✅ Has handler | | `ScrobbleRulesLive.Form` | `{:created, _}`, `{:updated, _}` | ScrobbleRulesLive.Index | ✅ Has handlers | | `ScrobbledTracksLive.Form` | `{:saved, _track}` | ScrobbledTracksLive.Index | ✅ Has handler | | `OnlineStoreTemplateLive.Form` | `{:saved, template}` | OnlineStoreTemplateLive.Index | ✅ Has handler | | `ScrobbleRulePicker` | `{:rule_created, _rule}` | ScrobbledTracksLive.Index, StatsLive.Index | ✅ Both have handlers | **Investigation steps**: 1. Cross-reference every `send(self(), ...)` call site (found via `grep -rn "send(self()"`) with `handle_info` clauses in the parent LiveView 2. Specifically trace the `Release` component usage in `CollectionLive.Show` — it renders `Release` with `on_release_loaded={:release_loaded}`, but `CollectionLive.Show` has no `handle_info({:release_loaded, _})` clause. Determine if this is a dead message or a missing handler 3. Verify no component sends messages when rendered in a context that doesn't handle them (e.g., modal closed, wrong live_action) 4. Look for `send/2` calls using patterns NOT matching `{__MODULE__, _}` **Files to check**: All `lib/music_library_web/components/*.ex`, all `lib/music_library_web/live/**/*.ex` --- ## Phase 2: PubSub Subscription Lifecycle Audit **Goal**: Verify all PubSub subscriptions are properly paired with unsubscriptions; identify stale subscription risks. **Subscription inventory**: | Subscribe | Topic | Where | Unsubscribe | Risk | | ---------------------------- | -------------------------- | -------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------- | | `Records.subscribe(id)` | `"records:#{id}"` | CollectionLive.Show, WishlistLive.Show (via `manage_subscription/2`) | ✅ `manage_subscription/2` unsubscribes old ID before subscribing new — called on every `handle_params` | Low | | `ListeningStats.subscribe()` | `"listening_stats:update"` | StatsLive.Index (`mount`), ScrobbledTracksLive.Index (`mount`) | ⚠️ Never unsubscribed. Only called when connected. | Medium — PID monitoring handles cleanup but double-subscription on reconnect possible | **Broadcast inventory**: | Broadcast | Topic | Message | Called from | | ----------------------------------- | -------------------------- | ------------------- | ----------------------------------------- | | `Records.notify_update/1` | `"records:#{id}"` | `{:update, record}` | Workers (needs audit — see Phase 3) | | `ListeningStats.broadcast_update/1` | `"listening_stats:update"` | `%{track_count: n}` | `lib/music_library/listening_stats.ex:68` | **Investigation steps**: 1. Verify `manage_subscription/2` is called on ALL navigation paths: browser back/forward, direct URL, push_navigate, push_patch 2. Check that `Records.subscribe/1` is NEVER called outside of `manage_subscription/2` (only two Show LiveViews should call it) 3. Verify `ListeningStats.subscribe()` doesn't double-subscribe on LiveView reconnect (check if Phoenix.LiveView re-mounts fully or reuses process) 4. Check that `Records.unsubscribe/1` is called on terminate — currently it's only called on navigation, not process termination. Phoenix.PubSub auto-cleans when PID dies, but verify 5. Audit `Records.notify_update/1` broadcast sites against intended behavior (see Phase 3) **Files to check**: `lib/music_library/records.ex`, `lib/music_library/listening_stats.ex`, `lib/music_library_web/live_helpers/record_actions.ex`, all Show LiveViews --- ## Phase 3: Concurrent State Change Safety Audit **Goal**: Verify that background worker updates to records don't corrupt LiveView state. **Pattern**: Oban worker modifies record → calls `Records.notify_update/1` → PubSub broadcasts `{:update, record}` → Show LiveView's `handle_info({:update, record}, socket)` applies update via `RecordActions.handle_record_updated/2`. **Guard**: `record.id == socket.assigns.record.id` in both CollectionLive.Show and WishlistLive.Show ✅ **Worker-to-broadcast audit** (every worker that modifies records): | Worker | Modifies record? | Calls notify_update? | Check | | ----------------------------------- | ----------------------------------- | -------------------- | ------------------------ | | `RefreshCover` | Yes — updates cover_url, cover_hash | Need to verify | Read worker source | | `PopulateGenres` | Yes — updates genres | Need to verify | Read worker source | | `GenerateRecordEmbedding` | Yes (indirect via Similarity) | Need to verify | Read worker + Similarity | | `RecordRefreshMusicBrainzData` | Yes — updates musicbrainz_data | Need to verify | Read worker source | | `RecordRefreshAllMusicBrainzData` | Yes (batch) | Need to verify | Read worker + Batch | | `RecordGenerateAllEmbeddings` | Yes (batch) | Need to verify | Read worker + Batch | | `ImportFromMusicbrainzRelease` | Creates new record | Need to verify | Read worker source | | `ImportFromMusicbrainzReleaseGroup` | Creates new records | Need to verify | Read worker source | | `PruneAssets` | No (assets only) | N/A | Skip | | `PruneAssetCache` | No (cache only) | N/A | Skip | | `ApplyScrobbleRules` | No (scrobble data) | N/A | Skip | | `FetchArtistInfo` | No (artist info) | N/A | Skip | | All `ArtistRefresh*` workers | No (artist info) | N/A | Skip | | `RefreshScrobbles` | No (scrobble data) | N/A | Skip | | `BackfillScrobbledTracks` | No (scrobble data) | N/A | Skip | | `SendRecordsOnThisDayEmail` | No (email) | N/A | Skip | | `RepoVacuum` / `RepoOptimize` | No (maintenance) | N/A | Skip | **Additional checks**: 1. **Double-update race**: If two workers update the same record simultaneously, does the LiveView handle both `handle_info` calls correctly? (Each re-assigns `:record` — second update is latest, which is correct since workers touch different fields) 2. **Form edit + background update race**: User editing in `RecordForm` modal while a background worker updates the record. The form holds a stale `@record` assign. When the worker broadcasts `{:update, record}`, the Show page updates `socket.assigns.record`, but the form LiveComponent has its OWN `@record` assign set during `update_many`. What happens? This is a potential functional bug. 3. **ArtistLive.Show**: Does NOT handle `{:update, record}` — artist updates use a different mechanism (artist info is updated through `ArtistLive.Form` which sends `{:saved, artist_info}` directly). Verify this is intentional and complete. **Files to check**: All `lib/music_library/worker/*.ex`, `lib/music_library/records.ex` (notify_update), `lib/music_library/records/similarity.ex`, all Show LiveViews, `RecordForm` component --- ## Phase 4: Performance Low-Hanging Fruit Audit **Goal**: Identify unnecessary work — DB queries, stream resets, server round trips — that could be eliminated or deferred. ### 4a. Unnecessary Full-Stream Reloads | LiveView | Stream name | Reset trigger | Optimizable? | Impact | | ----------------------------- | ---------------------------------- | -------------------------------------------------------------------- | -------------------------------------------------------------------------------- | ---------------------------------------- | | CollectionLive.Index | `:records` | Every param change, form save, display toggle | Display toggle (grid↔list) could be CSS class only — same data, different render | Medium: 1 FTS query per toggle | | WishlistLive.Index | `:records` | Same as above | Same | Medium | | ScrobbleRulesLive.Index | `:scrobble_rules` | Form save: does `stream_insert` then `load_and_assign_rules` (reset) | The `stream_insert` is immediately overwritten by full reload — redundant | Low: cosmetic | | OnlineStoreTemplateLive.Index | `:templates` | Form save: same redundant `stream_insert` + full reload | Same | Low | | ScrobbledTracksLive.Index | `:tracks` | Form save, PubSub scrobble update | Could incrementally `stream_insert` the edited track instead of full reload | Medium: full FTS on every scrobble batch | | RecordSetLive.Index | `:record_sets` | Form create | Could `stream_insert` the new set without full reload | Low | | StatsLive.Index | `:recent_tracks`, `:recent_albums` | PubSub scrobble update | Incremental `stream_insert` could replace full reset | Medium: resets on every scrobble batch | ### 4b. Synchronous Mount Work → Async Candidates | LiveView | Blocking queries | Recommendation | Impact | | -------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------- | ----------------------------- | | StatsLive.Index | `count_records_by_artist(20)`, `count_records_by_genre(20)`, `count_records_by_release_year(20)`, `get_records_on_this_day()`, `get_latest_record()` | Move to `assign_async` — these are below-the-fold charts | High: 5 queries blocking TTFB | | CollectionLive.Index | `Chats.count_chats(:collection, ...)` | Small, acceptable | None | ### 4c. Server Round Trips → Client-Side Candidates | Interaction | Current | Optimizable? | Impact | | ------------------------------- | --------------------------------------------------------------------------- | ---------------------------------------------------------- | -------------------------------------------- | | Display mode toggle (grid↔list) | Server `handle_event("set_display")` → full DB query + stream reset | Switch to client-side JS hook that toggles CSS class | Medium: eliminates 1 round trip + 1 DB query | | Search input | Every keystroke triggers `phx-change` → `handle_event("search")` → DB query | Check if `phx-debounce` is set. If not, add 300ms debounce | Medium: reduces query load | | Sort order change | `push_patch` → `handle_params` → DB query | Cannot be client-side (pagination depends on sort order) | N/A | ### 4d. Optimistic UI Opportunities | Operation | Current behavior | Optimistic approach | Risk | | ---------------------------- | ------------------------------------------- | ---------------------------------------------------------------------------- | ------------------------------ | | Record delete | `Repo.delete` → `stream_delete` (confirmed) | `stream_delete` immediately → `Repo.delete` in background; rollback on error | Low: delete failures are rare | | Form save in modal | `push_patch` to base index → full reload | `stream_insert` updated record into parent stream without navigation | Medium: need to handle re-sort | | Scrobble rule enabled toggle | Full list reload via `push_patch` | Toggle `stream_insert` item in-place | Low | | Online store template toggle | Full list reload via `push_patch` | Toggle `stream_insert` item in-place | Low | ### 4e. N+1 and Query Pattern Audit **Checklist**: - [ ] `CollectionLive.Show.handle_params/3` — loads record, last_listened_track, play_count, record_sets, chat_count, embedding_text, similar_records. Check for N+1. - [ ] `ArtistLive.Show.handle_params/3` — loads artist, collection_records, wishlist_records. Check for N+1. - [ ] Stream rendering — check `.record_grid`, `.record_list`, `.record_card` components for per-item DB calls - [ ] Verify preloads cover all rendered fields (artists, formats, genres, dominant_colors) **Investigation method**: 1. Use QueryReporter skill to capture SQL for: `/collection`, `/collection/:id`, `/artists/:id`, `/stats` 2. Review stream usage with `grep -rn "stream(" lib/music_library_web/live/` 3. Check for `Repo` calls inside LiveComponents (should delegate to context) 4. Check for `Repo` calls inside `.heex` templates (should be in `handle_params`) **Files to check**: All `lib/music_library_web/live/**/*.ex`, all `lib/music_library_web/components/*.ex` ## Definition of Done - [x] #1 Audit report for each phase written as a Backlog.md document with file paths, line references, severity ratings, and fix recommendations - [x] #2 At least one concrete finding identified per phase before considering the phase complete - [x] #3 Each finding includes a specific file:line reference (not just a general concern) - [x] #4 Findings are triaged by severity: HIGH (crashes/data loss), MEDIUM (wrong behavior/noticeable perf), LOW (cosmetic/redundant) - [x] #5 ML-168 is referenced where relevant but not duplicated — this audit identifies NEW issues only - [x] #6 No fixes implemented during audit — only identification and recommendation