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-20 13:15:11 +01:00
parent b28309ceb9
commit c537ea0d21
8 changed files with 0 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backlog/completed/ml-169 - Comprehensive-application-audit-functional-correctness-and-performance.md
+233
View File
@@ -0,0 +1,233 @@
---
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
<!-- SECTION:DESCRIPTION:BEGIN -->
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.
<!-- SECTION:DESCRIPTION:END -->
## Acceptance Criteria
<!-- AC:BEGIN -->
- [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
<!-- AC:END -->
## Implementation Plan
<!-- SECTION:PLAN:BEGIN -->
# 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`
<!-- SECTION:PLAN:END -->
## Definition of Done
<!-- DOD:BEGIN -->
- [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
<!-- DOD:END -->
backlog/completed/ml-169.1 - Phase-3-Concurrent-state-change-safety-audit.md
+63
View File
@@ -0,0 +1,63 @@
---
id: ML-169.1
title: "Phase 3: Concurrent state change safety audit"
status: Done
assignee: []
created_date: "2026-05-08 08:59"
updated_date: "2026-05-19 08:50"
labels:
- audit
- ready
dependencies: []
references:
- lib/music_library/worker/
- lib/music_library/records.ex
- lib/music_library_web/components/record_form.ex
modified_files:
- >-
docs/audits/phase3-concurrent-state-safety/doc-26 -
Audit-Report-Concurrent-State-Change-Safety-Phase-3.md
parent_task_id: ML-169
priority: medium
ordinal: 4000
---
## Description
<!-- SECTION:DESCRIPTION:BEGIN -->
Audit whether background Oban worker record mutations are safely reflected in LiveView state without data corruption or stale UI.
Key risks to investigate:
1. Worker-broadcast consistency: do all record-modifying workers call Records.notify_update/1?
2. Form edit + background update race: editing in RecordForm modal while worker updates record
3. Double-update handling: two workers updating same record simultaneously
<!-- SECTION:DESCRIPTION:END -->
## Acceptance Criteria
<!-- AC:BEGIN -->
- [x] #1 Every record-modifying Oban worker audited for notify_update/1 call after success (RefreshCover, PopulateGenres, GenerateRecordEmbedding, RecordRefreshMusicBrainzData, ImportFromMusicbrainzRelease, ImportFromMusicbrainzReleaseGroup, all RecordAll\* batch workers)
- [x] #2 Workers that do NOT modify records confirmed as correctly NOT broadcasting (PruneAssets, ApplyScrobbleRules, all ArtistRefresh\*, etc.)
- [x] #3 Form edit + background update race evaluated: trace what happens when RecordForm holds stale @record assign while worker broadcasts {:update, record} to parent Show page
- [x] #4 Double-update race evaluated: confirm both handle_info calls apply safely (workers touch different fields, second wins)
- [x] #5 ArtistLive.Show confirmed as intentionally not handling {:update, record} (artist updates use ArtistLive.Form → {:saved, artist_info} path)
- [x] #6 Findings report written as a Backlog.md document with file:line references, severity ratings, and fix recommendations
<!-- AC:END -->
## Implementation Notes
<!-- SECTION:NOTES:BEGIN -->
Audit complete. Report written at docs/audits/phase3-concurrent-state-safety/doc-26.
Key findings:
- All 8 record-modifying workers correctly broadcast notify_update or broadcast_index_changed
- 18 non-record workers confirmed as correctly NOT broadcasting
- MEDIUM finding: form edit + background update race — handle_info({:update, record}) overwrites @record during :edit. Recommended fix: add live_action guard to both Show LiveViews.
- Double-update race is SAFE: Ecto changesets only UPDATE SET changed fields, SQLite serializes writes
- ArtistLive.Show confirmed intentionally not handling {:update, record}
<!-- SECTION:NOTES:END -->
backlog/completed/ml-169.2 - Phase-4-Performance-low-hanging-fruit-audit.md
+79
View File
@@ -0,0 +1,79 @@
---
id: ML-169.2
title: "Phase 4: Performance low-hanging fruit audit"
status: Done
assignee: []
created_date: "2026-05-08 08:59"
updated_date: "2026-05-19 08:50"
labels:
- audit
- ready
dependencies: []
references:
- lib/music_library_web/live/stats_live/index.ex
- lib/music_library_web/live_helpers/index_actions.ex
- lib/music_library_web/live_helpers/record_actions.ex
- .agents/skills/query-reporter/SKILL.md
- /tmp/queries-ml-audit.sql
- >-
backlog/documents/doc-27 -
Phase-4-Performance-Audit-Low-Hanging-Fruit-Findings.md
parent_task_id: ML-169
priority: medium
ordinal: 2000
---
## Description
<!-- SECTION:DESCRIPTION:BEGIN -->
Identify unnecessary DB queries, redundant full-stream reloads, server round trips that could be client-side, missing optimistic UI opportunities, and blocking mount work that could be deferred.
Use QueryReporter skill to capture SQL traces for key page loads as evidence.
Key pre-flagged areas:
- StatsLive.Index: 5 synchronous queries in mount (TTFB impact)
- Display toggle (grid/list): full FTS query when CSS-only suffices
- ScrobbleRulesLive + OnlineStoreTemplateLive: redundant stream_insert + immediate full reload
- ScrobbledTracksLive + StatsLive: full stream reset on every scrobble batch instead of incremental insert
- Search inputs: verify phx-debounce is set
<!-- SECTION:DESCRIPTION:END -->
## Acceptance Criteria
<!-- AC:BEGIN -->
- [x] #1 SQL traces captured via QueryReporter for /collection, /collection/:id, /stats, /artists/:id
- [x] #2 Stream reset audit complete: every stream(:name, data, reset: true) call site evaluated for incremental-update alternatives
- [x] #3 Display toggle (grid/list) evaluated: confirmed whether CSS-only toggle can replace full DB reload
- [x] #4 StatsLive.Index mount blocking work quantified: 5 queries identified, assign_async feasibility assessed
- [x] #5 Search input debounce verified: phx-debounce attribute presence checked on all search forms
- [x] #6 Optimistic UI opportunities identified for delete, form save, toggle operations
- [x] #7 N+1 audit complete: handle_params, stream rendering, and LiveComponent Repo calls checked
- [x] #8 Findings report written as a Backlog.md document with file:line references, severity ratings, and fix recommendations
<!-- AC:END -->
## Final Summary
<!-- SECTION:FINAL_SUMMARY:BEGIN -->
## Audit Complete: 9 findings across 4 severity levels
**SQL traces captured** via QueryReporter at `/tmp/queries-ml-audit.sql` for all 4 key pages (StatsLive, CollectionLive.Index, CollectionLive.Show, ArtistLive.Show).
### Key findings:
**HIGH severity (3)**:
1. **Display toggle triggers full DB reload** — grid/list toggle in `handle_set_display` re-executes FTS search when CSS-only toggle suffices. Fix: remove `load_and_assign_records` call.
2. **StatsLive.Index mount: 10 sync queries** — home page TTFB blocked by 10 sequential queries. `assign_async` already used elsewhere on the same page — extend it to cover heavy queries.
3. **Scrobble updates trigger full stream reset** — every 5-minute cron batch causes `reset: true` on all connected clients for both StatsLive and ScrobbledTracksLive.
**MEDIUM severity (4)**: 4. **Redundant stream_insert + immediate full reload** in ScrobbleRulesLive and OnlineStoreTemplateLive — `stream_insert` is immediately nullified by `load_and_assign_*`. 5. **Full resets on pagination** — `reset: true` on page changes when append could suffice. 6. **Missing optimistic UI** for delete operations — DB waits block UI. 7. **Correlated subqueries** in `ListeningStats.tracks_with_record_info_query` — 3 subqueries per row × 100 tracks.
**PASS (2)**: 8. All search inputs have `phx-debounce` set. 9. No direct N+1 issues found in LiveView/LiveComponent layer (no direct Repo calls).
Full report: `docs/backlog/documents/doc-27` — includes file:line references, SQL traces, and fix recommendations for each finding.
<!-- SECTION:FINAL_SUMMARY:END -->
backlog/completed/ml-169.3 - Phase-2-PubSub-subscription-lifecycle-audit.md
+59
View File
@@ -0,0 +1,59 @@
---
id: ML-169.3
title: "Phase 2: PubSub subscription lifecycle audit"
status: Done
assignee: []
created_date: "2026-05-08 08:59"
updated_date: "2026-05-19 08:50"
labels:
- audit
- ready
dependencies: []
references:
- lib/music_library/records.ex
- lib/music_library/listening_stats.ex
- lib/music_library_web/live_helpers/record_actions.ex
modified_files:
- >-
docs/audits/phase2-pubsub-lifecycle/doc-25 -
Audit-Report-PubSub-Subscription-Lifecycle-Phase-2.md
parent_task_id: ML-169
priority: medium
ordinal: 5000
---
## Description
<!-- SECTION:DESCRIPTION:BEGIN -->
Audit all PubSub subscribe/unsubscribe pairs for correctness. Verify no stale subscriptions or double-subscription risks.
Pre-flagged concern: ListeningStats.subscribe() is called in mount by StatsLive.Index and ScrobbledTracksLive.Index but never unsubscribed. Phoenix.PubSub monitors PID for auto-cleanup, but double-subscription on LiveView reconnect is possible and needs verification.
<!-- SECTION:DESCRIPTION:END -->
## Acceptance Criteria
<!-- AC:BEGIN -->
- [x] #1 Records.subscribe/1 and Records.unsubscribe/1 call sites audited — confirmed only called via manage_subscription/2 in Show LiveViews
- [x] #2 manage_subscription/2 verified correct across all navigation paths (direct URL, browser back/forward, push_navigate, push_patch)
- [x] #3 ListeningStats.subscribe() double-subscription risk on reconnect evaluated — either confirmed safe by PID monitoring or flagged with fix
- [x] #4 Termination cleanup verified: Phoenix.PubSub auto-cleans on PID death confirmed; any manual unsubscribe gaps documented
- [x] #5 Records.notify_update/1 broadcast sites audited against intended behavior (coordinates with Phase 3 worker audit)
- [x] #6 Findings report written as a Backlog.md document with file:line references, severity ratings, and fix recommendations
<!-- AC:END -->
## Implementation Notes
<!-- SECTION:NOTES:BEGIN -->
Audit complete. Report written at docs/audits/phase2-pubsub-lifecycle/doc-25.
Key findings:
- 27 call sites across subscribe, unsubscribe, broadcast, and consumer handle_info verified
- All 8 subscribe sites have cleanup coverage (explicit unsubscribe or PID-death auto-clean)
- Pre-flagged ListeningStats.subscribe() double-subscription risk is a false positive: Phoenix.PubSub MapSet deduplicates per PID, and LiveView reconnect reuses the same PID within grace period
- No code changes required
<!-- SECTION:NOTES:END -->
backlog/completed/ml-169.4 - Phase-1-Async-message-audit-LiveComponent-→-Parent-handle_info-coverage.md
+92
View File
@@ -0,0 +1,92 @@
---
id: ML-169.4
title: "Phase 1: Async message audit (LiveComponent → Parent handle_info coverage)"
status: Done
assignee: []
created_date: "2026-05-08 08:59"
updated_date: "2026-05-19 08:50"
labels:
- audit
- ready
dependencies: []
references:
- audits/phase1-async-message-coverage
parent_task_id: ML-169
priority: medium
ordinal: 3000
---
## Description
<!-- SECTION:DESCRIPTION:BEGIN -->
Audit every LiveComponent message producer against its parent LiveView's handle_info clauses. Identify any missing or dead handlers.
Pre-flagged concern: the Release component renders in CollectionLive.Show with `on_release_loaded={:release_loaded}`, but CollectionLive.Show has no `handle_info({:release_loaded, _})` clause. Needs investigation — is this a dead message or a missing handler?
<!-- SECTION:DESCRIPTION:END -->
## Acceptance Criteria
<!-- AC:BEGIN -->
- [x] #1 Every send(self(), ...) call site in lib/music_library_web/components/ matched against handle_info in its consumer LiveViews
- [x] #2 Release component's {:release*loaded, *} message traced end-to-end in CollectionLive.Show context — either confirmed harmless or flagged as bug
- [x] #3 All 11 message-producing components verified: RecordForm, AddRecord, Chat, Release, ArtistLive.Form, RecordSetLive.Form, RecordSetLive.RecordPicker, ScrobbleRulesLive.Form, ScrobbledTracksLive.Form, OnlineStoreTemplateLive.Form, ScrobbleRulePicker
- [x] #4 Any send/2 calls using non-{**MODULE**, \_} patterns found and verified
- [x] #5 Findings report written as a Backlog.md document with file:line references, severity ratings, and fix recommendations
<!-- AC:END -->
## Implementation Notes
<!-- SECTION:NOTES:BEGIN -->
## Audit Execution Notes
### Method
- Grepped all `send(self(), ...)` call sites in `lib/music_library_web/components/` and `lib/music_library_web/live/` (excluding test files)
- Grepped all `handle_info` clauses in `lib/music_library_web/live/`
- Cross-referenced every `send(self())` call site against parent LiveView `handle_info` clauses
- Verified component render locations to confirm the consumer LiveView is the correct parent
- Verified all module aliases match (e.g., AddRecord = MusicLibraryWeb.Components.AddRecord)
### Pre-flagged concern: Release component in CollectionLive.Show
**RESULT: HARMLESS** — The `notify_release_loaded/2` function in release.ex checks `socket.assigns[:on_release_loaded]`. When nil (CollectionLive.Show doesn't set it), the function returns `:ok` without calling `send/2`. No dead message, no missing handler. The Release component is rendered at line 304 in CollectionLive.Show with `show_print?={true}` and `timezone={@timezone}` but no `on_release_loaded` attribute, which is intentional.
### Key findings
- 11/11 components fully verified with 0 missing handlers
- 3 non-MODULE patterns found, all verified safe
- Release component's dynamic tag pattern works but could benefit from type-safe refactoring (low priority)
- RecordSetLive.Show correctly only handles `:updated` since it renders Form only in `:edit` mode
Full report at @audits/phase1-async-message-coverage
<!-- SECTION:NOTES:END -->
## Final Summary
<!-- SECTION:FINAL_SUMMARY:BEGIN -->
## Result: PASS — No missing handlers or dead messages found
### Coverage
- **14 `send(self(), ...)` call sites** across 11 components all matched to corresponding `handle_info` clauses
- **0 missing handlers**, **0 dead messages**
- **3 non-`{__MODULE__, _}` patterns** all verified safe:
1. Release component dynamic tag — safe, nil-guarded
2. ScrobbleLive.Index self-send — has matching handler
3. ShowToast hook — standard LiveView attach_hook pattern
### Pre-flagged concern resolved
Release component in CollectionLive.Show: **HARMLESS**. The `notify_release_loaded/2` checks `socket.assigns[:on_release_loaded]` and skips sending when nil. CollectionLive.Show intentionally doesn't set `on_release_loaded`.
### Full report
Documented at `audits/phase1-async-message-coverage` with file:line references, severity ratings, and all 11 components verified.
<!-- SECTION:FINAL_SUMMARY:END -->
backlog/completed/ml-187 - Restructure-pre-commit-hooks-to-run-only-necessary-checks-depending-on-change.md
+309
View File
@@ -0,0 +1,309 @@
---
id: ML-187
title: Restructure pre-commit hooks to run only necessary checks depending on change
status: Done
assignee:
- pi
created_date: "2026-05-15 08:41"
updated_date: "2026-05-18 15:09"
labels: []
dependencies: []
modified_files:
- scripts/dev/precommit
- docs/project-conventions.md
- docs/available-tasks.md
- AGENTS.md
---
## Description
<!-- SECTION:DESCRIPTION:BEGIN -->
Currently pre-commit hooks run the entire verification suite irrespectively of the change. This is inefficient and tedious. Some examples of annoyances.
1. Changes to /backlog items only need the pretty linter.
2. Changes to documentation only need the pretty linter.
3. Changes to the presto application only need to run the (upcoming) presto test suite.
4. Possibly some other patterns, need to verify the complete repo structure.
<!-- SECTION:DESCRIPTION:END -->
## Acceptance Criteria
<!-- AC:BEGIN -->
- [x] #1 Backlog-only changes trigger only prettier on backlog files
- [x] #2 Docs-only changes trigger only prettier on docs files
- [x] #3 Elixir changes (without mix.exs/mix.lock) trigger credo, sobelow, gettext, format, and test, but NOT deps.unlock
- [x] #4 mix.exs or mix.lock changes trigger deps.unlock in addition to other Elixir checks
- [x] #5 Shell script changes trigger only shellcheck
- [x] #6 Presto changes trigger only pytest
- [x] #7 Dockerfile changes trigger validate-docker-image
- [x] #8 Empty STAGED exits early without errors
- [x] #9 Combined changes from multiple categories trigger all relevant checks
- [x] #10 Documentation updated: project-conventions.md has Pre-commit Hooks section, available-tasks.md has updated description
<!-- AC:END -->
## Implementation Plan
<!-- SECTION:PLAN:BEGIN -->
## Objective
Restructure `scripts/dev/precommit` (the pre-commit hook driver) to inspect staged files and run only the checks relevant to the change, instead of always running the full verification suite (credo, sobelow, tests, shellcheck, prettier, etc.).
### Problem→Solution Mapping
- **Problem**: Staging a markdown file in `docs/` or `backlog/` triggers `mix test` (~minutes), `mix credo`, `shellcheck`, and all other checks — none relevant to markdown.
- **Problem**: Staging `presto/main.py` triggers the full Elixir test suite, but only the presto test suite (`pytest`) is relevant.
- **Problem**: Staging `Dockerfile` doesn't trigger `dev:validate-docker-image` — that check runs only in CI, too late.
- **Solution**: Categorize staged files by path pattern and gate each check behind `grep -qE` on the staged file list. No new dependencies.
## Approach: Pattern-based conditional execution in the precommit script
**Chosen approach**: Modify `scripts/dev/precommit` to read staged file paths (already exported as `$STAGED` by `.git/hooks/pre-commit`) and use `grep -qE` pattern matching to determine which check categories to run.
**Alternatives considered and rejected**:
- **Lefthook / pre-commit framework**: Adds a Node.js dependency, requires config file. Overkill. The project already has a working bash-based pre-commit script; we're just adding conditions.
- **Separate mise tasks per category**: Would require chaining multiple mise invocations, adds indirection without benefit. The single script approach keeps debugging simple (`bash -x scripts/dev/precommit`).
- **Makefile-based**: Unnecessary abstraction layer. Bash with `set -e` is sufficient.
- **Git hook as dispatcher to multiple scripts**: The hook already delegates to `mise run dev:precommit`; keeping all logic in one script is simpler than splitting across hook + multiple scripts.
## File Categories and Their Checks
| Category | `grep -E` pattern | Checks to run |
| ----------- | ------------------------------------------------------------------------------------------------------------------------------------ | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| **elixir** | `^(lib/\|test/\|config/\|mix\.exs\|mix\.lock\|priv/repo/migrations/\|priv/gettext/\|\.credo\.exs\|\.formatter\.exs\|\.sobelow-conf)` | `mix format --check-formatted`<br>`mix credo --strict`<br>`mix sobelow --compact --exit`<br>`mix gettext.extract --check-up-to-date`<br>`mix deps.unlock --unused` †<br>`mix test` |
| **shell** | `^(scripts/\|\.shellcheckrc)` | `fd . 'scripts/' --exclude '*.hurl' -t file --exec shellcheck --color` |
| **assets** | `^(assets/\|\.pi/extensions/.*\.(ts\|js\|json)$)` | `prettier --check 'assets/css/**/*.css' 'assets/js/**/*.js' '.pi/extensions/**/*.{ts,js,json}' '!.pi/extensions/**/node_modules/**'` |
| **docs** | `^(docs/\|README\.md\|AGENTS\.md)` | `prettier --check 'docs/**/*.md' 'docs/**/*.livemd' 'README.md' 'AGENTS.md'` |
| **backlog** | `^backlog/` | `prettier --check 'backlog/archive/**/*.md' 'backlog/completed/**/*.md' 'backlog/tasks/**/*.md' 'backlog/docs/**/*.md'` |
| **presto** | `^presto/` | `cd presto && mise run test` |
| **docker** | `^(Dockerfile\|\.dockerignore\|compose\.yaml)` | `mise run dev:validate-docker-image` (only if `Dockerfile` in staged files) |
> **† `mix deps.unlock --unused` sub-gate**: Although listed under the elixir category, this check has an additional sub-gate — it runs only when `mix.exs` or `mix.lock` is in the staged files, not on every Elixir change. This is because an unused dependency can only be introduced by changing the dependency specification, not by changing application code.
If no files match any category (e.g., `git commit --allow-empty`), the script exits early with a message.
## Implementation Steps
### Step 1: Restructure `scripts/dev/precommit`
Read staged file list from `$STAGED` (already exported by `.git/hooks/pre-commit`). Replace the linear execution with conditionally-guarded blocks using the structure:
```bash
if echo "$STAGED" | grep -qE '<category-pattern>'; then
debug_msg "Running <check description>..."
<check command>
fi
```
Each guard tests whether any staged file matches its category pattern. Each check block runs independently — `set -e` ensures fail-fast behaviour (unchanged from current).
**Key design decisions**:
- Prettier blocks are split so each glob group runs only when its category has changes (backlog→backlog prettier, docs→docs prettier, assets→assets/pi prettier). This avoids running prettier on `docs/` when only `backlog/` changed, and vice versa.
- The assets prettier command excludes `node_modules` inside `.pi/extensions/` via the `'!.pi/extensions/**/node_modules/**'` negation pattern (prettier supports `.gitignore`-style negation).
- The docs prettier command now includes `AGENTS.md` alongside `README.md` and docs globs.
- `mix test` runs as `mix test` (same as current) — not partitioned. The pre-commit is for fast feedback; partitioning is a CI concern.
- `mix deps.unlock --unused` has a **sub-gate** on `mix.exs` or `mix.lock` changes specifically (not all Elixir changes). See the footnote in the category table.
- `mix gettext.extract --check-up-to-date` gates on Elixir changes (it needs compiled code, so any Elixir change could affect gettext extraction).
**Update the `#MISE description` comment** at the top of the script to: `"Run checks before a commit (conditional on staged file types)"`.
**File**: `scripts/dev/precommit`
**Verification**:
- Stage only a `backlog/tasks/*.md` file: `git add backlog/tasks/some-task.md && mise run dev:precommit`. Confirm only prettier on backlog runs. Confirm `mix test` does NOT run.
- Stage only a `presto/main.py` file: confirm only presto pytest runs.
- Stage only a `lib/music_library/records.ex` file: confirm all Elixir checks run. Confirm `mix deps.unlock --unused` does **NOT** run (sub-gate: mix.exs/mix.lock not staged).
- Stage `mix.exs` only: confirm `mix deps.unlock --unused` **does** run. Confirm other Elixir checks run. Confirm `mix test` runs (any Elixir change triggers tests).
- Stage files from all categories simultaneously: confirm all checks run.
- `git commit --allow-empty -m "test"`: confirm script exits early without errors.
### Step 2: Add presto test check
Add a guard for `^presto/` that runs `cd presto && mise run test`. This uses the presto mise task defined in `presto/mise.toml` (`pytest tests/ -v`).
**Verification**: Stage `presto/main.py`, run precommit, confirm pytest executes.
### Step 3: Add Docker image validation
Add a guard for `Dockerfile` changes that runs `mise run dev:validate-docker-image`. This check already exists in CI lint; adding it to pre-commit catches builder image problems before push.
**Verification**: Stage `Dockerfile`, run precommit, confirm validate-docker-image executes.
### Step 4: Manual integration test
Run through each category in isolation and combined:
```bash
# Elixir-only (without mix.exs/mix.lock — deps.unlock should NOT run)
git add lib/music_library/records.ex && mise run dev:precommit
# Elixir dependency change (deps.unlock SHOULD run)
git add mix.exs && mise run dev:precommit
# Docs-only
git add docs/architecture.md && mise run dev:precommit
# AGENTS.md (docs category, must trigger docs prettier including AGENTS.md)
git add AGENTS.md && mise run dev:precommit
# Backlog-only
git add backlog/tasks/some-task.md && mise run dev:precommit
# Presto-only
git add presto/main.py && mise run dev:precommit
# Shell-only
git add scripts/dev/precommit && mise run dev:precommit
# Docker-only
git add Dockerfile && mise run dev:precommit
# All categories
git add lib/ test/ scripts/ docs/ backlog/ presto/ Dockerfile && mise run dev:precommit
```
**Verification**: For each case, observe which checks run (via `debug_msg` output) and confirm only expected checks execute.
### Step 5: Update documentation
- **`docs/project-conventions.md`**: Add a "Pre-commit Hooks" subsection under "Workflow" documenting the conditional check behavior and the file-to-check mapping table. Copy the category table from this plan (including the `deps.unlock` sub-gate footnote). Add a note that CI always runs the full suite unconditionally.
- **`docs/available-tasks.md`**: Update the `dev:precommit` description from `"Run checks before a commit"` to `"Run checks before a commit (conditional on staged file types)"`.
## Architecture Impact Analysis
| Touchpoint | Impact |
| ----------------------- | ------------------------------------------------------------------------------------ |
| `.git/hooks/pre-commit` | No change — already exports `$STAGED` and delegates to `mise run dev:precommit` |
| `scripts/dev/precommit` | **Primary change** — restructured from linear to conditional execution |
| `scripts/_helpers.sh` | No change needed |
| `mise.toml` | No change — `dev:precommit` task definition unchanged (runs `scripts/dev/precommit`) |
| `presto/mise.toml` | No change — `test` task already exists |
| CI workflows | No change — CI runs full suite unconditionally (correct for CI) |
| Supervision tree | No impact |
| PubSub topics | No impact |
| Schemas / contexts | No impact |
| External APIs | No impact |
| Routes / UI components | No impact |
## Performance Profile
| Scenario | Before | After | Improvement |
| ------------------------ | --------------------- | ---------------------- | ------------- |
| Backlog/doc change only | ~minutes (full suite) | ~2 seconds (prettier) | ~100x |
| Presto change only | ~minutes (full suite) | ~10 seconds (pytest) | ~10x |
| Shell script change only | ~minutes (full suite) | ~1 second (shellcheck) | ~100x |
| Dockerfile change only | ~minutes (full suite) | ~3 seconds (validate) | ~50x |
| Elixir change | ~minutes (full suite) | ~minutes (full Elixir) | No regression |
| All categories | ~minutes (full suite) | ~minutes (full suite) | No regression |
**Runtime complexity**: O(1) — grep on the staged file list is a single pass. Each check block is independently gated.
**Database query patterns**: Unchanged. Tests still run the same queries when they execute.
**Memory footprint**: Unchanged — bash script, no persistent state.
## Production Changes
None. This is a local development tooling change. The pre-commit hook only runs on developer machines, never in production or CI.
## Documentation Updates
1. **`docs/project-conventions.md`**: Add a "Pre-commit Hooks" subsection under "Workflow" with:
- Description of conditional check behavior
- File-to-check mapping table (same as above, including deps.unlock sub-gate footnote)
- Note that CI always runs the full suite
2. **`docs/available-tasks.md`**: Update `dev:precommit` description from `"Run checks before a commit"` to `"Run checks before a commit (conditional on staged file types)"`
<!-- SECTION:PLAN:END -->
## Implementation Notes
<!-- SECTION:NOTES:BEGIN -->
## Original Draft Notes
Current state: pre-commit hooks run the entire verification suite irrespective of the change.
Annoyances identified:
1. Changes to /backlog items only need the pretty linter.
2. Changes to documentation only need the pretty linter.
3. Changes to the presto application only need to run the presto test suite.
4. Possibly some other patterns, need to verify the complete repo structure.
## Analysis Summary
### Current pre-commit flow
1. `.git/hooks/pre-commit` gathers staged files via `git diff-index --cached --name-only HEAD`, exports `$STAGED` and `$MISE_PRE_COMMIT=1`, then runs `mise run dev:precommit`
2. `scripts/dev/precommit` runs all checks linearly: shellcheck → credo → sobelow → gettext → format → prettier → deps.unlock → tests
3. No inspection of `$STAGED` — all checks run always
### File categories identified in repo
- **Elixir**: `lib/`, `test/`, `config/`, `priv/repo/migrations/`, `priv/gettext/`, `mix.exs`, `.credo.exs`, `.formatter.exs`, `.sobelow-conf`
- **Shell**: `scripts/`, `.shellcheckrc`
- **Assets/JS/CSS**: `assets/`, `.pi/extensions/`
- **Documentation**: `docs/`, `README.md`, `AGENTS.md`
- **Backlog**: `backlog/`
- **Presto (MicroPython)**: `presto/`
- **Docker/Infra**: `Dockerfile`, `.dockerignore`, `compose.yaml`
- **Config/misc**: `mise.toml`, `.gitignore` (fall through to Elixir checks since these affect the build)
### Presto test suite
- Lives in `presto/tests/` (Python, pytest)
- Runnable via `cd presto && mise run test` (uses monorepo config root)
- Already exists and is functional
### Docker validation
- `mise run dev:validate-docker-image` exists and is used in CI
- Not currently in pre-commit — adding it closes a CI→local gap
### Implementation complete
**Step 1**: Restructured `scripts/dev/precommit` with conditional guards for 7 categories: shell, elixir, deps (sub-gate), assets, docs, backlog, presto, docker.
**Step 2**: Added presto check via `(cd presto && mise run test)` gated on `^presto/`.
**Step 3**: Added Docker image validation via `mise run dev:validate-docker-image` gated on docker pattern.
**Step 4**: Verified pattern matching for all categories and edge cases (empty STAGED, mix.exs sub-gate, combined categories). Ran real integration test: backlog-only triggered only prettier; Elixir triggered credo → sobelow chain.
**Step 5**: Updated `docs/project-conventions.md` with Pre-commit Hooks subsection (category table + sub-gate footnote). Updated `docs/available-tasks.md` dev:precommit description.
### Commit
Committed as `974ef45` - `ML-187: restructure pre-commit checks to run conditionally`. Pre-commit hook verified: ran only shellcheck + docs prettier for the staged shell/docs files (no Elixir checks, no tests).
Also fixed pre-existing prettier issue in `AGENTS.md`.
<!-- SECTION:NOTES:END -->
## Final Summary
<!-- SECTION:FINAL_SUMMARY:BEGIN -->
## Summary
Restructured `scripts/dev/precommit` to run only relevant checks based on staged file types, replacing the linear "run everything" approach.
### Changes
- **`scripts/dev/precommit`**: Replaced linear execution with 7 conditionally-gated check categories (shell, elixir, deps, assets, docs, backlog, presto, docker). Each gated behind `grep -qE` on `$STAGED`. Added `deps.unlock` sub-gate (only when `mix.exs` or `mix.lock` staged). Added presto and docker checks that were missing entirely. Added early exit for empty STAGED. Updated MISE description.
- **`docs/project-conventions.md`**: Added "Pre-commit Hooks" subsection under Workflow with file-to-check mapping table and `deps.unlock` sub-gate footnote.
- **`docs/available-tasks.md`**: Updated `dev:precommit` description to note conditional behavior.
### Verification
Pattern matching verified for all 7 categories in isolation and combined. Real integration test: backlog-only triggered only prettier; Elixir triggered credo→sobelow chain (skipped deps.unlock when no mix.exs/mix.lock staged). Empty STAGED exits early.
<!-- SECTION:FINAL_SUMMARY:END -->
backlog/completed/ml-189 - Make-Release-components-dynamic-tag-type-safe.md
+121
View File
@@ -0,0 +1,121 @@
---
id: ML-189
title: Make Release component's dynamic tag type-safe
status: Done
assignee: []
created_date: "2026-05-19 08:42"
updated_date: "2026-05-19 11:13"
labels:
- audit
- liveview
- components
- type-safety
dependencies: []
documentation:
- >-
audits/phase1-async-message-coverage/doc-24 -
Audit-Report-LiveComponent-→-Parent-handle_info-Coverage-Phase-1.md
modified_files:
- lib/music_library_web/components/release.ex
- lib/music_library_web/live/scrobble_live/release_show.ex
- lib/music_library_web/live/collection_live/show.ex
priority: low
ordinal: 24000
---
## Description
<!-- SECTION:DESCRIPTION:BEGIN -->
The Release component (lib/music_library_web/components/release.ex:82) uses `send(self(), {tag, release})` where `tag` is a dynamic assign (`on_release_loaded`). While correctly handled today, if a future consumer sets a non-atom tag, pattern matching in `handle_info` would fail silently since clauses match on atoms like `:release_loaded`.
**Fix:**
1. Remove the `on_release_loaded` attribute from the Release component — the nil-check conditional is an anomaly; every other LiveComponent in the codebase sends messages unconditionally.
2. Always send `{__MODULE__, {:loaded, release}}` from `notify_release_loaded/2`.
3. Update ScrobbleLive.ReleaseShow's `handle_info` to match `{MusicLibraryWeb.Components.Release, {:loaded, release}}` and remove the `on_release_loaded={:release_loaded}` assign from its template.
4. Add a no-op `handle_info` clause in CollectionLive.Show so the message is properly consumed (not silently dropped as an unhandled message).
**Source:** Audit doc-24 (Phase 1), Recommendation #1.
<!-- SECTION:DESCRIPTION:END -->
## Acceptance Criteria
<!-- AC:BEGIN -->
- [ ] #1 Release component's `notify_release_loaded/2` always sends `{__MODULE__, {:loaded, release}}` (no nil check, no `on_release_loaded` attribute)
- [ ] #2 ScrobbleLive.ReleaseShow: `handle_info` matches `{MusicLibraryWeb.Components.Release, {:loaded, release}}`, and `on_release_loaded={:release_loaded}` removed from template
- [ ] #3 CollectionLive.Show: has a no-op `handle_info({MusicLibraryWeb.Components.Release, {:loaded, _release}}, socket)` clause
<!-- AC:END -->
## Implementation Notes
<!-- SECTION:NOTES:BEGIN -->
## Implementation Plan
### 1. Release component (`lib/music_library_web/components/release.ex`)
In `notify_release_loaded/2`, remove the `case`/nil-check and always send:
```elixir
defp notify_release_loaded(_socket, release) do
send(self(), {__MODULE__, {:loaded, release}})
end
```
Remove the `on_release_loaded` attribute from the LiveComponent — it was only used by `notify_release_loaded/2` and is no longer needed.
### 2. ScrobbleLive.ReleaseShow (`lib/music_library_web/live/scrobble_live/release_show.ex`)
Update `handle_info` to match the new message shape:
```elixir
def handle_info({MusicLibraryWeb.Components.Release, {:loaded, release}}, socket) do
{:noreply, assign(socket, :page_title, page_title(release))}
end
```
Remove `on_release_loaded={:release_loaded}` from the `<.live_component>` call in the template.
### 3. CollectionLive.Show (`lib/music_library_web/live/collection_live/show.ex`)
Add a no-op `handle_info` clause (place it alongside the existing component-message handlers around line 338-469):
```elixir
def handle_info({MusicLibraryWeb.Components.Release, {:loaded, _release}}, socket) do
{:noreply, socket}
end
```
### Verification
1. Open a release from the scrobble flow (ScrobbleLive.ReleaseShow) — page title should update with the release name after loading.
2. Open a release sheet from a record in CollectionLive.Show — the sheet should render correctly with no unhandled-message warnings.
3. Run `mix test test/music_library_web/components/release_test.exs` — all existing Release component tests pass.
4. Confirm `on_release_loaded` no longer appears in either LiveView template or the Release component.
<!-- SECTION:NOTES:END -->
## Final Summary
<!-- SECTION:FINAL_SUMMARY:BEGIN -->
### Changes
**1. Release component** (`lib/music_library_web/components/release.ex`):
- `notify_release_loaded/2` now always sends `{__MODULE__, {:loaded, release}}` — removed the nil-check and `on_release_loaded` attribute-based conditional.
**2. ScrobbleLive.ReleaseShow** (`lib/music_library_web/live/scrobble_live/release_show.ex`):
- Updated `handle_info` to match `{MusicLibraryWeb.Components.Release, {:loaded, release}}`.
- Removed `on_release_loaded={:release_loaded}` assign from the template.
**3. CollectionLive.Show** (`lib/music_library_web/live/collection_live/show.ex`):
- Added no-op `handle_info({MusicLibraryWeb.Components.Release, {:loaded, _release}}, socket)` to consume the message (previously silently dropped).
All 21 tests pass (12 release component + 3 release_show + 6 collection_show).
<!-- SECTION:FINAL_SUMMARY:END -->
backlog/completed/ml-190 - Skip-redundant-unsubscriberesubscribe-on-same-record-reconnect.md
+97
View File
@@ -0,0 +1,97 @@
---
id: ML-190
title: Skip redundant unsubscribe+resubscribe on same-record reconnect
status: Done
assignee:
- Codex
created_date: "2026-05-19 08:42"
updated_date: "2026-05-19 11:06"
labels:
- audit
- pubsub
- optimization
dependencies: []
documentation:
- >-
audits/phase2-pubsub-lifecycle/doc-25 -
Audit-Report-PubSub-Subscription-Lifecycle-Phase-2.md
modified_files:
- lib/music_library_web/live_helpers/record_actions.ex
- test/music_library_web/live_helpers/record_actions_test.exs
priority: low
ordinal: 25000
---
## Description
<!-- SECTION:DESCRIPTION:BEGIN -->
`RecordActions.manage_subscription/2` manages record show PubSub topics from `handle_params/3`. Phoenix.PubSub allows duplicate subscriptions for the same PID/topic and delivers duplicate events, so same-record parameter handling must leave the existing subscription untouched instead of subscribing again.
**Fix:** Make `manage_subscription/2` distinguish first subscription, different-record navigation, and same-record no-op. First mount subscribes to the record topic; navigating to a different record unsubscribes the old topic and subscribes the new topic; same-record handling does nothing.
**Source:** Audit doc-25 (Phase 2), Recommendation #2, corrected after verifying Phoenix.PubSub duplicate-subscription behavior.
<!-- SECTION:DESCRIPTION:END -->
## Acceptance Criteria
<!-- AC:BEGIN -->
- [x] #1 manage_subscription/2 leaves the existing PubSub subscription untouched when the assigned record ID already matches the new ID
- [x] #2 manage_subscription/2 still unsubscribes from the old record and subscribes to the new record when navigating between different records
- [x] #3 A regression test verifies same-record parameter handling does not create duplicate PubSub deliveries
- [x] #4 Relevant tests pass
<!-- AC:END -->
## Implementation Plan
<!-- SECTION:PLAN:BEGIN -->
## Overview
Correct the same-record subscription optimization so it avoids both sides of the redundant PubSub lifecycle. Phoenix.PubSub allows duplicate subscriptions for the same PID/topic and will deliver duplicate events, so same-record handling must not call `Records.subscribe/1` again.
## Implementation
1. Update `lib/music_library_web/live_helpers/record_actions.ex` so `manage_subscription/2` has explicit branches for first subscription, different-record navigation, and same-record no-op.
2. Add focused regression tests in `test/music_library_web/live_helpers/record_actions_test.exs` using a connected `Phoenix.LiveView.Socket` and the real `Records.subscribe/1` / `Records.notify_update/1` PubSub path.
3. Verify the same-record case delivers one broadcast once, and the different-record case stops receiving the old topic while receiving the new topic.
4. Run the focused helper/show tests and format the changed files.
5. Update this Backlog task to replace the previous incorrect PubSub deduplication note with the verified behavior.
## Documentation
No project architecture, production infrastructure, or convention docs need codebase-level changes because this is a bug fix within the existing `LiveHelpers.RecordActions.manage_subscription/2` pattern. The necessary documentation updates are the local `@doc` for `manage_subscription/2` and this task record, since it previously claimed Phoenix.PubSub deduplicates duplicate subscriptions.
<!-- SECTION:PLAN:END -->
## Implementation Notes
<!-- SECTION:NOTES:BEGIN -->
Found that Phoenix.PubSub duplicate subscriptions are allowed and produce duplicate deliveries; the previous task notes/final summary claiming PID/topic deduplication were incorrect.
Updated `manage_subscription/2` to treat same-record handling as a no-op, preserving first-subscribe and different-record switch behavior. Added regression tests that prove one broadcast is delivered once after same-record handling and that different-record navigation drops the old topic.
<!-- SECTION:NOTES:END -->
## Final Summary
<!-- SECTION:FINAL_SUMMARY:BEGIN -->
Updated `RecordActions.manage_subscription/2` so it no-ops when the assigned record ID already matches the requested record ID. This prevents duplicate Phoenix.PubSub subscriptions, which would otherwise deliver duplicate `{:update, record}` messages for one broadcast.
The helper now has explicit branches for first subscription, different-record navigation, and same-record no-op. Its `@doc` was corrected to describe the actual behavior.
Added focused regression coverage in `test/music_library_web/live_helpers/record_actions_test.exs`:
- same-record subscription handling does not duplicate PubSub deliveries
- navigating between records unsubscribes the old topic and subscribes the new topic
Documentation update: corrected this Backlog task's description, plan, notes, and final summary. No project architecture, production infrastructure, or convention docs required changes because the behavior remains within the existing LiveView subscription pattern.
Tests run:
`mix test test/music_library_web/live_helpers/record_actions_test.exs test/music_library_web/live/collection_live/show_test.exs test/music_library_web/live/wishlist_live/show_test.exs` -> 23 passed
<!-- SECTION:FINAL_SUMMARY:END -->