From acc5eb77ea31aff06244afd3bf3e2209a2ae1bb6 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Tue, 26 May 2026 23:00:19 -0700 Subject: [PATCH 1/6] Fix popup: anchor artist field in MB search to stop title-collision covers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `/api/musicbrainz/search_tracks` powers the Fix popup's auto-search cascade for users on MusicBrainz as primary. When both track + artist fields were filled, `search_tracks_with_artist` always took the bare keyword path (` ` joined as one query string). MB's recording-search scorer weights title matches far above artist matches, so for "Coffee Break" + "Zeds Dead" the top results were Emapea / The Vidalias / West One Orchestra's "Coffee Break" — three unrelated cover- title collisions ahead of the canonical Zeds Dead recording. The endpoint's `rerank_tracks` pass can't fix this when the right answer is below the API's 50-result cutoff. Both-fields mode now uses a strict field-scoped Lucene query first (`recording:"" AND artist:""`) which anchors the artist and prunes title-collision covers at the source. `min_score=0` because the field-scoped query is itself precise; rerank still does final ordering. Bare query stays as the fallback when strict returns nothing — covers the diacritic / alias cases the original `strict=False` path was added for ("Bjork" query vs canonical "Björk" artist where Lucene phrase match never hits the recording). Single-field mode (track-only or artist-only) is unchanged: still bare- query directly, since there's no artist value to anchor. Also stable-sort results to prefer entries with non-zero `duration_ms`. MB has multiple recordings per song (single release, album release, remasters, compilations) and not every recording carries length data. Without the preference sort, the user sees a 0:00 row first while a sibling recording with the real 3:04 sits two rows below — matches the report where MBID-paste lookup of the canonical recording (length 3:04) contradicted the search-result's 0:00 row for the same song. Tests: - new `test_search_tracks_with_artist_strict_first_when_both_fields` pins the strict=True call when both fields present - new `test_search_tracks_with_artist_falls_back_to_bare_when_strict_empty` pins the Björk-style fall-through path - new `test_search_tracks_with_artist_prefers_results_with_known_length` pins the length-preference sort - existing `..._keeps_low_score_for_rerank` updated to side_effect so the bare-fallback path is exercised; behaviour pinned identically - existing `..._uses_bare_query_mode` renamed + repurposed for strict- first; old name's behaviour no longer accurate --- core/musicbrainz_search.py | 47 +++++++-- tests/metadata/test_musicbrainz_search.py | 123 ++++++++++++++++------ webui/static/helper.js | 1 + 3 files changed, 126 insertions(+), 45 deletions(-) diff --git a/core/musicbrainz_search.py b/core/musicbrainz_search.py index 47e95a84..ff39a827 100644 --- a/core/musicbrainz_search.py +++ b/core/musicbrainz_search.py @@ -757,19 +757,46 @@ class MusicBrainzSearchClient: `search_tracks`'s structured-query dispatch (`Artist - Track` splitting, bare-name artist-first browse). - Uses bare-query mode (`strict=False`) — diacritic-folded, hits - alias/sortname indexes, no `AND`-clause that kills recall when - either side mis-matches. Score floor lowered to 20 (vs the search - tab's 80) so MB recordings whose title doesn't literally contain - the artist name still enter the candidate pool — the endpoint's - `rerank_tracks` pass then sorts by artist-match relevance. Without - this, queries like `Army of Me` + `Bjork` only surface covers - (score 73-100) and miss Björk's canonical recording (score 28). + When both fields are present: strict-first, bare-as-fallback. The + strict pass builds a field-scoped Lucene query (`recording:"" + AND artist:""`) which anchors the artist and prunes title- + collision covers — fixes the "Coffee Break" + "Zeds Dead" case + where MB's title-text-biased scorer surfaced Emapea / Vidalias / + West One Orchestra ahead of the canonical Zeds Dead recording. + `min_score=0` on strict because the field-scoped query is itself + precise, and the endpoint's `rerank_tracks` pass does the final + ordering. Bare query runs only when strict returns nothing — + catches diacritic / alias mismatches (`Bjork` query vs canonical + `Björk` artist) where strict phrase match never hits. + + When only one field is present: bare-query mode directly — same + recall-over-precision tradeoff the old single-path took. + + Results are stable-sorted to prefer entries with non-zero + `duration_ms`. MB often has several recordings per song (single + release, album release, compilations, remasters) and not every + recording carries length data. Without this, the first match can + be a length-less duplicate while a sibling recording with the + real 3:04 sits two rows down. """ if not track and not artist: return [] - return self._search_tracks_text(track, artist or None, limit, - strict=False, min_score=20) + + if track and artist: + results = self._search_tracks_text( + track, artist, limit, strict=True, min_score=0 + ) + if not results: + results = self._search_tracks_text( + track, artist, limit, strict=False, min_score=20 + ) + else: + results = self._search_tracks_text( + track, artist or None, limit, strict=False, min_score=20 + ) + + results.sort(key=lambda t: 0 if (t.duration_ms or 0) > 0 else 1) + return results def _pick_representative_release(self, releases: List[Dict[str, Any]]) -> Optional[Dict[str, Any]]: """Pick the best release out of a release-group's editions. diff --git a/tests/metadata/test_musicbrainz_search.py b/tests/metadata/test_musicbrainz_search.py index e5859707..322cc284 100644 --- a/tests/metadata/test_musicbrainz_search.py +++ b/tests/metadata/test_musicbrainz_search.py @@ -1000,33 +1000,84 @@ def test_get_recording_flat_swallows_client_errors(): # search_tracks_with_artist — Fix-popup cascade adapter # --------------------------------------------------------------------------- -def test_search_tracks_with_artist_uses_bare_query_mode(): - """The Fix-popup cascade needs MB's bare-query mode so diacritics and - bracketed suffixes don't kill recall. The adapter must pass strict=False - through to the underlying search_recording call.""" +def test_search_tracks_with_artist_strict_first_when_both_fields(): + """Both fields present → strict field-scoped Lucene query first + (`recording:"" AND artist:""`). Fixes the "Coffee Break" + + "Zeds Dead" case where bare query lets MB's title-text-biased + scorer surface unrelated covers ahead of the canonical recording.""" client = MusicBrainzSearchClient() client._client = MagicMock() client._client.search_recording.return_value = [ - {'id': 'rec-1', 'title': 'Army of Me', 'score': 95, - 'releases': [{'id': 'rel-1', 'title': 'Post', 'date': '1995'}], - 'artist-credit': [{'name': 'Björk'}]}, + {'id': 'rec-1', 'title': 'Coffee Break', 'score': 95, + 'length': 184000, + 'releases': [{'id': 'rel-1', 'title': 'Coffee Break', 'date': '2015'}], + 'artist-credit': [{'name': 'Zeds Dead'}]}, ] - tracks = client.search_tracks_with_artist('Army of Me', 'Björk', limit=10) + tracks = client.search_tracks_with_artist('Coffee Break', 'Zeds Dead', limit=10) - # strict=False is the critical bit — fuzzy recall, not phrase precision + # strict=True is the critical bit — anchors artist via Lucene AND clause client._client.search_recording.assert_called_once_with( - 'Army of Me', artist_name='Björk', limit=10, strict=False + 'Coffee Break', artist_name='Zeds Dead', limit=10, strict=True ) assert len(tracks) == 1 - assert tracks[0].name == 'Army of Me' - assert 'Björk' in tracks[0].artists + assert tracks[0].name == 'Coffee Break' + assert 'Zeds Dead' in tracks[0].artists + + +def test_search_tracks_with_artist_falls_back_to_bare_when_strict_empty(): + """Strict phrase match misses diacritic / alias cases ("Bjork" query + vs canonical "Björk" artist). When strict returns nothing, fall + through to bare query so rerank can still surface the right answer.""" + client = MusicBrainzSearchClient() + client._client = MagicMock() + client._client.search_recording.side_effect = [ + [], # strict pass → no hits (Lucene phrase match fails on diacritic) + [ # bare pass → recall via alias index + {'id': 'rec-canonical', 'title': 'Army of Me', 'score': 28, + 'releases': [], 'artist-credit': [{'name': 'Björk'}]}, + ], + ] + + tracks = client.search_tracks_with_artist('Army of Me', 'Bjork', limit=10) + + assert client._client.search_recording.call_count == 2 + first_call = client._client.search_recording.call_args_list[0] + second_call = client._client.search_recording.call_args_list[1] + assert first_call.kwargs['strict'] is True + assert second_call.kwargs['strict'] is False + assert len(tracks) == 1 + assert tracks[0].id == 'rec-canonical' + + +def test_search_tracks_with_artist_prefers_results_with_known_length(): + """MB has multiple recordings per song (single release, album release, + compilations) and not every recording carries length data. Stable + sort moves length-known entries ahead of length-zero duplicates so + the user sees the actionable 3:04 row first, not the 0:00 sibling.""" + client = MusicBrainzSearchClient() + client._client = MagicMock() + client._client.search_recording.return_value = [ + {'id': 'rec-no-length', 'title': 'Coffee Break', 'score': 100, + 'releases': [], 'artist-credit': [{'name': 'Zeds Dead'}]}, + {'id': 'rec-with-length', 'title': 'Coffee Break', 'score': 90, + 'length': 184000, + 'releases': [], 'artist-credit': [{'name': 'Zeds Dead'}]}, + ] + + tracks = client.search_tracks_with_artist('Coffee Break', 'Zeds Dead', limit=10) + + # rec-with-length must surface first even though MB scored it lower + assert tracks[0].id == 'rec-with-length' + assert tracks[0].duration_ms == 184000 + assert tracks[1].id == 'rec-no-length' def test_search_tracks_with_artist_handles_missing_artist(): - """Track-only query (no artist) still works — empty string becomes - None, and the underlying client searches recordings without an - artist filter.""" + """Track-only query (no artist) still works — single-field path takes + bare-query mode directly (no strict-first round-trip since there's no + artist to anchor). Empty string becomes None so MB drops the AND + clause.""" client = MusicBrainzSearchClient() client._client = MagicMock() client._client.search_recording.return_value = [ @@ -1036,7 +1087,6 @@ def test_search_tracks_with_artist_handles_missing_artist(): client.search_tracks_with_artist('Some Song', '', limit=5) - # Empty artist → None passed to the client so MB drops the AND clause client._client.search_recording.assert_called_once_with( 'Some Song', artist_name=None, limit=5, strict=False ) @@ -1051,33 +1101,33 @@ def test_search_tracks_with_artist_empty_returns_empty_list(): client._client.search_recording.assert_not_called() -def test_search_tracks_with_artist_keeps_low_score_for_rerank(): - """Cascade path uses a low score floor (20) so MB recordings whose - title doesn't literally contain the artist name still enter the - candidate pool — the endpoint's rerank pass surfaces them by - artist-match relevance. Real example: "Army of Me" + "Bjork" — the - canonical Björk recording scores 28 in MB (title doesn't contain - "Bjork"), while title-collision covers like "Army of Me (Bjork)" - score 73-100. Strict 80 floor drops the right answer.""" +def test_search_tracks_with_artist_bare_fallback_keeps_low_score_for_rerank(): + """When strict returns nothing and we fall through to bare, the bare + pass uses a low score floor (20) so MB recordings whose title doesn't + literally contain the artist name still enter the candidate pool — + the endpoint's rerank pass surfaces them by artist-match relevance. + Real example: "Army of Me" + "Bjork" — strict fails on the diacritic + mismatch, bare picks up the canonical Björk recording at score 28 + while filtering true noise at score 5.""" client = MusicBrainzSearchClient() client._client = MagicMock() - client._client.search_recording.return_value = [ - {'id': 'rec-cover', 'title': 'Army of Me (Bjork)', 'score': 100, - 'releases': [], 'artist-credit': [{'name': 'HIRS Collective'}]}, - {'id': 'rec-canonical', 'title': 'Army of Me', 'score': 28, - 'releases': [], 'artist-credit': [{'name': 'Björk'}]}, - {'id': 'rec-noise', 'title': 'Bjork', 'score': 5, - 'releases': [], 'artist-credit': [{'name': 'Random'}]}, + client._client.search_recording.side_effect = [ + [], # strict pass → no hits + [ # bare pass + {'id': 'rec-cover', 'title': 'Army of Me (Bjork)', 'score': 100, + 'releases': [], 'artist-credit': [{'name': 'HIRS Collective'}]}, + {'id': 'rec-canonical', 'title': 'Army of Me', 'score': 28, + 'releases': [], 'artist-credit': [{'name': 'Björk'}]}, + {'id': 'rec-noise', 'title': 'Bjork', 'score': 5, + 'releases': [], 'artist-credit': [{'name': 'Random'}]}, + ], ] tracks = client.search_tracks_with_artist('Army of Me', 'Bjork', limit=50) ids = [t.id for t in tracks] - # Score=28 canonical Björk recording is kept — the endpoint's rerank - # will surface it by artist match. assert 'rec-canonical' in ids assert 'rec-cover' in ids - # Score=5 is below the 20 floor — true garbage still filtered out. assert 'rec-noise' not in ids @@ -1120,7 +1170,10 @@ def test_search_tracks_text_strict_param_default_true(): def test_search_tracks_with_artist_swallows_client_errors(): """MB client raising must not crash the endpoint — return [] so the - Fix-popup cascade falls through to the next source.""" + Fix-popup cascade falls through to the next source. Both strict and + bare passes swallow exceptions independently, so a strict-pass raise + still lets the bare-pass run; a bare-pass raise after empty strict + returns [].""" client = MusicBrainzSearchClient() client._client = MagicMock() client._client.search_recording.side_effect = RuntimeError('network down') diff --git a/webui/static/helper.js b/webui/static/helper.js index d4f8fd40..a34e58b6 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3415,6 +3415,7 @@ function closeHelperSearch() { const WHATS_NEW = { '2.6.3': [ { unreleased: true }, + { title: 'Fix popup: artist + track fields no longer surface unrelated covers', desc: 'separate artist / track inputs in the Fix popup used to dump both into a bare MusicBrainz keyword query — and MB\'s scorer heavily favours title matches, so searching "Coffee Break" + "Zeds Dead" surfaced random "Coffee Break" tracks by other artists ahead of the canonical Zeds Dead one. fields-mode now uses a field-scoped Lucene query that actually anchors the artist, with the old bare query kept as a fallback for diacritic mismatches like "Bjork" vs "Björk" where strict phrase match misses. results also stable-prefer entries with known track length so the canonical 3:04 sibling sits above the 0:00 duplicate.' }, { title: 'Groundwork: unified playlist source layer', desc: 'first slice of a refactor that\'ll let ListenBrainz, Last.fm radio, and SoulSync Discovery playlists live as Sync-page tabs alongside Spotify / Tidal / Qobuz / YouTube — so they can be mirrored + scheduled like the rest. this commit adds the shared adapter layer all those sources will plug into; no UI changes yet. nothing to do on your end.' }, { title: 'Auto-Sync refresh now routes through the unified source layer', desc: 'follow-up to the groundwork above. the mirrored-playlist auto-refresh handler used to have a ~190-line if/elif chain branching per source (one branch each for Spotify, Spotify public, Deezer, Tidal, YouTube). now it asks the source registry for the right adapter and calls one refresh method. behavior identical — same matched_data, same Tidal-skip-on-no-auth log, same Spotify-public-prefers-authed-API fallback. unlocks ListenBrainz / Last.fm / SoulSync Discovery as future Sync-page mirror sources without a fresh elif branch each time.' }, { title: 'Discovery folded into the unified source contract', desc: 'next slice of the groundwork. each playlist source can now answer one extra question — "match these raw tracks against Spotify / iTunes" — through the same adapter interface. Spotify / Tidal / Qobuz / YouTube / Deezer / Spotify-public / iTunes-link / SoulSync-Discovery all answer trivially (their tracks already have provider IDs); ListenBrainz + Last.fm run the matching engine. mirror-refresh now calls this automatically when a source returns MB-metadata-only tracks, so when ListenBrainz becomes a Sync-page tab next commit, its mirrors land already discovered + ready to sync — no separate Discover-page round-trip needed.' }, From 39f582a6908d0f8873a1f09e53496ba6bf6a4907 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Wed, 27 May 2026 06:59:58 -0700 Subject: [PATCH 2/6] Mirrored playlist: stop Playlist Pipeline from reverting manual Fix-popup matches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit User reported that manually mapping a mirrored-playlist track via the Fix popup (either by search or by pasting an MBID) worked end-to-end once — match saved, library track downloaded — but the next Playlist Pipeline run flipped the track back to "Provider Changed" and forced them to re-do the manual map every cycle. Three independent issues were combining to cause this: 1. Hardcoded `provider: 'spotify'` on manual-fix save `update_youtube_discovery_match` (the endpoint the Fix popup posts to, also used by mirrored playlists since the frontend routes `platform === 'mirrored'` through the YouTube endpoint) always stamped the cached match as Spotify-provided. The Fix-popup cascade actually queries the user's primary metadata source first and falls back to Spotify / Deezer / iTunes / MusicBrainz — so a user on MusicBrainz primary picking an MB result still had it saved as `provider: 'spotify'`. The next prepare-discovery call (which compares cached_provider to the active source) then immediately classified the match as drifted and pending re-discovery. Fixed by deriving `match_source` from `spotify_track.get('source')` (every *_search_tracks endpoint stamps `source` on results) with a fallback to `_get_active_discovery_source()` for the MBID-paste path (which uses the lean flat shape that doesn't carry source). `matched_data['source']` and the mirrored `extra_data['provider']` both now use the derived value. `match_source` is also recomputed in the cache-save except handler so the downstream mirrored-DB save still has it. 2. Discovery worker re-queueing manual matches as "incomplete" `run_playlist_discovery_worker` in `core/discovery/playlist.py` re-adds any track to `undiscovered_tracks` when its `matched_data` lacks `track_number` or `album.id` / `album.release_date`. The check was designed as a legacy-fix backfill for old discoveries that lost those fields to a Track-dataclass stripping bug. But manual fixes from the popup are *intentionally* lean — search- result rows don't include `track_number` (none of the search endpoints return it), and the MBID-lookup flat shape doesn't carry `album.id` / `release_date` (the recording lookup returns only `album.name`). So every manual match looked "incomplete" and got re-discovered every pipeline run, overwriting the user's pick with whatever the auto-search ranked first. Manual matches now short-circuit ahead of the incomplete-data branch. 3. `prepare_mirrored_discovery` ignored the `manual_match` flag Independent of the provider-stamping fix above, the prepare- discovery endpoint that powers the mirrored-playlist UI did its own `cached_provider != current_provider` check and didn't honour manual_match either. Defence in depth — even if a future code path stamps the wrong provider on a manual match, the flag now anchors it as cached. `has_cached` also extended so manual matches with off-provider stamps still count toward the cached tally for phase classification. Tests: - new `test_manual_match_skipped_even_when_matched_data_incomplete` in `tests/discovery/test_discovery_playlist.py` pins the worker short-circuit using a realistic MB-shape matched_data (album dict without id / release_date, no top-level track_number). 16 existing tests still green; 848 across discovery / metadata / automation suites pass. --- core/discovery/playlist.py | 13 +++++++ tests/discovery/test_discovery_playlist.py | 30 ++++++++++++++++ web_server.py | 42 +++++++++++++++++++--- webui/static/helper.js | 1 + 4 files changed, 81 insertions(+), 5 deletions(-) diff --git a/core/discovery/playlist.py b/core/discovery/playlist.py index 4bbbf1c7..f25e2999 100644 --- a/core/discovery/playlist.py +++ b/core/discovery/playlist.py @@ -125,6 +125,19 @@ def run_playlist_discovery_worker(playlists, automation_id=None, deps: PlaylistD if existing_extra.get('wing_it_fallback'): # Wing It stub — always re-attempt to find a real match undiscovered_tracks.append(track) + elif existing_extra.get('manual_match'): + # User explicitly picked this match via the Fix popup. + # Manual fixes are authoritative: they may lack + # track_number / album.id / release_date (the Fix-popup + # save shape is intentionally lean — search-result rows + # don't include track_number, and the MBID-lookup flat + # shape doesn't carry album.id), but re-running discovery + # against the active source would overwrite the user's + # deliberate pick with whatever the auto-search ranks + # first. Skip — pipeline only re-discovers when the user + # has cleared the match. + pl_skipped += 1 + total_skipped += 1 else: # Check if matched_data is complete — old discoveries may be missing # track_number/release_date due to the Track dataclass stripping them. diff --git a/tests/discovery/test_discovery_playlist.py b/tests/discovery/test_discovery_playlist.py index 8ab18063..8a8a8e9c 100644 --- a/tests/discovery/test_discovery_playlist.py +++ b/tests/discovery/test_discovery_playlist.py @@ -232,6 +232,36 @@ def test_unmatched_by_user_respected(): assert deps._db.extra_data_writes == [] +def test_manual_match_skipped_even_when_matched_data_incomplete(): + """manual_match=True must skip the incomplete-matched_data re-discovery + branch. The Fix-popup save shape is intentionally lean — search-result + rows don't carry track_number, and the MBID-lookup flat shape doesn't + carry album.id / release_date — so a manual fix always looks 'incomplete' + to the old check and used to be re-discovered every pipeline run, + overwriting the user's deliberate pick with whatever the auto-search + ranked first. Pin the fix: manual matches stay put.""" + extra = { + 'discovered': True, + 'manual_match': True, + 'provider': 'musicbrainz', + 'matched_data': { + 'id': 'mb-rec-id', + 'name': 'Coffee Break', + 'artists': ['Zeds Dead'], + 'album': {'name': 'Coffee Break'}, # no id, no release_date + 'source': 'musicbrainz', + # no track_number — Fix-popup shape never has it + }, + } + tracks = [_track(track_id=1, extra_data=extra)] + deps = _build_deps(tracks_by_playlist={'p1': tracks}) + + dp.run_playlist_discovery_worker([_playlist('p1')], deps=deps) + + # No extra_data writes — the manual match wasn't overwritten + assert deps._db.extra_data_writes == [] + + # --------------------------------------------------------------------------- # Cache hit short-circuit # --------------------------------------------------------------------------- diff --git a/web_server.py b/web_server.py index 6175cc57..0fd2ff69 100644 --- a/web_server.py +++ b/web_server.py @@ -24349,6 +24349,20 @@ def update_youtube_discovery_match(): album_obj['image_url'] = image_url album_obj['images'] = [{'url': image_url}] + # Manual fixes can come from any metadata source — the Fix-popup + # cascade queries the user's primary first, then Spotify / Deezer / + # iTunes / MusicBrainz as fallbacks. Each search endpoint stamps + # `source` on its results; the MBID-paste lookup doesn't, so fall + # back to the active discovery source. Hardcoding 'spotify' here + # used to make every non-Spotify manual match look like it had + # provider-drifted on the next prepare-discovery, triggering + # automatic re-discovery that overwrote the user's manual pick. + match_source = ( + spotify_track.get('source') + or _get_active_discovery_source() + or 'spotify' + ) + matched_data = { 'id': spotify_track['id'], 'name': spotify_track['name'], @@ -24356,7 +24370,7 @@ def update_youtube_discovery_match(): 'album': album_obj, 'duration_ms': spotify_track.get('duration_ms', 0), 'image_url': image_url, - 'source': 'spotify', + 'source': match_source, } cache_db = get_database() cache_db.save_discovery_cache_match( @@ -24366,6 +24380,13 @@ def update_youtube_discovery_match(): logger.info(f"Manual fix saved to discovery cache: {original_name} by {original_artist}") except Exception as cache_err: logger.error(f"Error saving manual fix to discovery cache: {cache_err}") + # match_source needs to exist for the mirrored-DB block below even + # if cache save failed — use the same fallback chain. + match_source = ( + spotify_track.get('source') + or _get_active_discovery_source() + or 'spotify' + ) # Persist manual fix to DB for mirrored playlists if identifier.startswith('mirrored_'): @@ -24377,7 +24398,7 @@ def update_youtube_discovery_match(): db = get_database() extra_data = { 'discovered': True, - 'provider': 'spotify', + 'provider': match_source, 'confidence': 1.0, 'matched_data': matched_data, 'manual_match': True, @@ -33671,11 +33692,17 @@ def prepare_mirrored_discovery(playlist_id): extra = track.get('extra_data') if extra and extra.get('discovered'): cached_provider = extra.get('provider', 'spotify') + is_manual = bool(extra.get('manual_match')) # If the cached result was discovered by a different provider than the # currently active one, treat it as pending so re-discovery uses the # correct source (IDs, album data, images differ between providers). - if cached_provider != _current_provider: + # Manual matches are exempt: the user explicitly picked that record, + # so re-discovery would overwrite their intentional fix every cycle. + # Without this guard, fixing a track via the Fix popup correctly + # downloads it once, but the next Playlist Pipeline run re-marks it + # "Provider Changed" and reverts the manual map. + if cached_provider != _current_provider and not is_manual: has_pending = True dur = track.get('duration_ms', 0) pre_discovered_results.append({ @@ -33757,11 +33784,16 @@ def prepare_mirrored_discovery(playlist_id): 'confidence': 0, }) - # Only treat as cached if at least one track was discovered by the current provider + # Treat as cached if at least one track was discovered by the current provider, + # OR if any track carries a manual_match (those are exempt from provider-drift + # re-discovery above, so they count as cached regardless of cached_provider). has_cached = any( t.get('extra_data') and (t['extra_data'].get('discovered') or t['extra_data'].get('discovery_attempted')) and - t['extra_data'].get('provider', 'spotify') == _current_provider + ( + t['extra_data'].get('provider', 'spotify') == _current_provider + or t['extra_data'].get('manual_match') + ) for t in tracks ) diff --git a/webui/static/helper.js b/webui/static/helper.js index a34e58b6..e3d96156 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3415,6 +3415,7 @@ function closeHelperSearch() { const WHATS_NEW = { '2.6.3': [ { unreleased: true }, + { title: 'Fix popup: manual matches survive Playlist Pipeline runs', desc: 'manually mapping a mirrored-playlist track via the Fix popup (search or MBID) saved correctly and the download went through, but the next Playlist Pipeline run kept reverting the match — the track flipped back to "Provider Changed" and you had to map it again. three things were ganging up on it: the manual-fix save always stamped `provider: \'spotify\'` even when the match came from MusicBrainz / iTunes / Deezer, so prepare-discovery saw a provider mismatch on the next run; the discovery worker also re-queued any matched_data that lacked `track_number` / `album.id` / `release_date` for re-discovery, and Fix-popup saves never carry those fields by design (search results don\'t include track_number, MBID lookups don\'t include album.id) — so manual fixes always looked "incomplete" and got overwritten; and the prepare-discovery provider check didn\'t honour the `manual_match` flag at all. all three patched — manual matches now stamp the actual source, the worker short-circuits on `manual_match` before the incomplete-data branch, and prepare-discovery treats manual fixes as cached regardless of provider drift.' }, { title: 'Fix popup: artist + track fields no longer surface unrelated covers', desc: 'separate artist / track inputs in the Fix popup used to dump both into a bare MusicBrainz keyword query — and MB\'s scorer heavily favours title matches, so searching "Coffee Break" + "Zeds Dead" surfaced random "Coffee Break" tracks by other artists ahead of the canonical Zeds Dead one. fields-mode now uses a field-scoped Lucene query that actually anchors the artist, with the old bare query kept as a fallback for diacritic mismatches like "Bjork" vs "Björk" where strict phrase match misses. results also stable-prefer entries with known track length so the canonical 3:04 sibling sits above the 0:00 duplicate.' }, { title: 'Groundwork: unified playlist source layer', desc: 'first slice of a refactor that\'ll let ListenBrainz, Last.fm radio, and SoulSync Discovery playlists live as Sync-page tabs alongside Spotify / Tidal / Qobuz / YouTube — so they can be mirrored + scheduled like the rest. this commit adds the shared adapter layer all those sources will plug into; no UI changes yet. nothing to do on your end.' }, { title: 'Auto-Sync refresh now routes through the unified source layer', desc: 'follow-up to the groundwork above. the mirrored-playlist auto-refresh handler used to have a ~190-line if/elif chain branching per source (one branch each for Spotify, Spotify public, Deezer, Tidal, YouTube). now it asks the source registry for the right adapter and calls one refresh method. behavior identical — same matched_data, same Tidal-skip-on-no-auth log, same Spotify-public-prefers-authed-API fallback. unlocks ListenBrainz / Last.fm / SoulSync Discovery as future Sync-page mirror sources without a fresh elif branch each time.' }, From b67d13164aa6e0a2089c1d1dc1cedca24f23507e Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Wed, 27 May 2026 07:08:21 -0700 Subject: [PATCH 3/6] Library: persist Enhanced / Standard view toggle in localStorage User feedback: the Enhanced view toggle on the artist detail page reset to Standard on every artist click, so admins who prefer Enhanced had to re-flip the toggle every single time. Persist the choice in localStorage and reapply on every artist navigation + page reload. - `toggleEnhancedView()` writes `soulsync-library-view-mode` to localStorage on every change. - `navigateToArtistDetail()` reads the saved value after the standard reset block runs; if `enhanced` AND `isEnhancedAdmin()` it calls `toggleEnhancedView(true)` after `loadArtistDetailData` kicks off. The brief Standard render is hidden as soon as the toggle flips. - Gated on `isEnhancedAdmin()` so non-admin profiles (which never see the toggle) can't end up with a stale Enhanced preference being applied silently. - Wrapped in try/catch since localStorage is unavailable in some private-browsing modes. No backend change; no DB migration needed. --- webui/static/helper.js | 1 + webui/static/library.js | 22 ++++++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/webui/static/helper.js b/webui/static/helper.js index e3d96156..6ec49872 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3415,6 +3415,7 @@ function closeHelperSearch() { const WHATS_NEW = { '2.6.3': [ { unreleased: true }, + { title: 'Library: Enhanced / Standard view toggle now sticks per browser', desc: 'the artist detail page used to revert to Standard view every time you clicked into a new artist — flipping to Enhanced was a per-click thing with no memory. now the choice is persisted in localStorage and reapplied automatically on every artist open. survives page reloads too. non-admins (who can\'t toggle Enhanced anyway) are unaffected; admins who never touched the Enhanced view stay on Standard since the saved value defaults to it.' }, { title: 'Fix popup: manual matches survive Playlist Pipeline runs', desc: 'manually mapping a mirrored-playlist track via the Fix popup (search or MBID) saved correctly and the download went through, but the next Playlist Pipeline run kept reverting the match — the track flipped back to "Provider Changed" and you had to map it again. three things were ganging up on it: the manual-fix save always stamped `provider: \'spotify\'` even when the match came from MusicBrainz / iTunes / Deezer, so prepare-discovery saw a provider mismatch on the next run; the discovery worker also re-queued any matched_data that lacked `track_number` / `album.id` / `release_date` for re-discovery, and Fix-popup saves never carry those fields by design (search results don\'t include track_number, MBID lookups don\'t include album.id) — so manual fixes always looked "incomplete" and got overwritten; and the prepare-discovery provider check didn\'t honour the `manual_match` flag at all. all three patched — manual matches now stamp the actual source, the worker short-circuits on `manual_match` before the incomplete-data branch, and prepare-discovery treats manual fixes as cached regardless of provider drift.' }, { title: 'Fix popup: artist + track fields no longer surface unrelated covers', desc: 'separate artist / track inputs in the Fix popup used to dump both into a bare MusicBrainz keyword query — and MB\'s scorer heavily favours title matches, so searching "Coffee Break" + "Zeds Dead" surfaced random "Coffee Break" tracks by other artists ahead of the canonical Zeds Dead one. fields-mode now uses a field-scoped Lucene query that actually anchors the artist, with the old bare query kept as a fallback for diacritic mismatches like "Bjork" vs "Björk" where strict phrase match misses. results also stable-prefer entries with known track length so the canonical 3:04 sibling sits above the 0:00 duplicate.' }, { title: 'Groundwork: unified playlist source layer', desc: 'first slice of a refactor that\'ll let ListenBrainz, Last.fm radio, and SoulSync Discovery playlists live as Sync-page tabs alongside Spotify / Tidal / Qobuz / YouTube — so they can be mirrored + scheduled like the rest. this commit adds the shared adapter layer all those sources will plug into; no UI changes yet. nothing to do on your end.' }, diff --git a/webui/static/library.js b/webui/static/library.js index 4e5467bf..dac0467a 100644 --- a/webui/static/library.js +++ b/webui/static/library.js @@ -879,6 +879,15 @@ function navigateToArtistDetail(artistId, artistName, sourceOverride = null, opt const bulkBar = document.getElementById('enhanced-bulk-bar'); if (bulkBar) bulkBar.classList.remove('visible'); + // Restore persisted view preference. Non-admins can't see / toggle the + // Enhanced control so only honour the saved choice for admins; default + // is still Standard. Wrapped in try/catch because localStorage can be + // disabled in private-browsing modes. + let _preferEnhanced = false; + try { + _preferEnhanced = localStorage.getItem('soulsync-library-view-mode') === 'enhanced'; + } catch (_) { /* localStorage unavailable */ } + // Navigate to artist detail page navigateToPage('artist-detail', { artistId, @@ -895,6 +904,13 @@ function navigateToArtistDetail(artistId, artistName, sourceOverride = null, opt // Load artist data loadArtistDetailData(artistId, artistName); + + // Apply persisted Enhanced view after the standard data load is kicked off. + // toggleEnhancedView() triggers its own loadEnhancedViewData() in parallel; + // the brief Standard render is hidden as soon as the toggle flips. + if (_preferEnhanced && isEnhancedAdmin()) { + toggleEnhancedView(true); + } } function _updateArtistDetailBackButtonLabel() { @@ -2905,6 +2921,12 @@ function toggleEnhancedView(enabled) { const bulkBar = document.getElementById('enhanced-bulk-bar'); if (bulkBar) bulkBar.classList.remove('visible'); } + + // Persist the choice so the next artist click (and the next page reload) + // honours it instead of always reverting to Standard. + try { + localStorage.setItem('soulsync-library-view-mode', enabled ? 'enhanced' : 'standard'); + } catch (_) { /* localStorage unavailable */ } } async function loadEnhancedViewData(artistId) { From 8dbbf13c6129ef59ff97f9f96b6816cfd2563d8f Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Wed, 27 May 2026 07:43:21 -0700 Subject: [PATCH 4/6] Branch cleanup: lift manual-match helpers, fix length-pref ordering, profile-scope view toggle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review pass on the prior three commits — kettui-style cleanup that should have landed first time. **Length-preference sort ordering (real bug):** The `search_tracks_with_artist` stable sort that promoted length-known recordings ran in `core/musicbrainz_search.py`, but the MB endpoint in `web_server.py:search_musicbrainz_tracks` runs `rerank_tracks` after it — which re-sorts by relevance score and dropped the length-pref ordering down to tiebreaker-only. For canonical-same-song MB duplicates that all score identically the tiebreaker survived, but the order-of-operations was wrong. Moved into `rerank_tracks` itself via a new `prefer_known_duration` flag. Sort key sits between relevance score and the stable-order tiebreaker so relevance still wins (length only decides ties, never overrides a higher-relevance match). The MB endpoint opts in via `prefer_known_duration=True`; Spotify / iTunes / Deezer callers stay on the default-off path since their search results always include length. Pinned with three new `TestRerankTracks` cases: ties-promote-length, relevance-still-wins, default-off-unchanged. **Route logic lifted to `core/discovery/manual_match.py`:** Two pieces lived as inline route logic in `web_server.py` — the `derive_manual_match_provider` fallback chain (payload.source → active source → 'spotify') used by `update_youtube_discovery_match`, and the `is_drifted_for_redo` predicate (cached provider differs from active AND not manual_match) used by `prepare_mirrored_discovery`. Per kettui's "extract logic from web_server.py, don't AST-parse it" standard, both helpers now live in `core/discovery/manual_match.py` with 12 dedicated unit tests covering fallback resolution order, non-dict payload defenses, manual_match exemption from drift, absent-provider legacy default, and edge cases. Side benefits from the lift: - `match_source` now derived once before the cache-save try block instead of being duplicated in try + except (the except block existed only because the original used `match_source` later — pre-computing killed the duplication). - `prepare_mirrored_discovery`'s `has_cached` check now reuses `is_drifted_for_redo` with inverted polarity instead of restating the field whitelist inline, so a future schema change only has to land in one place. - The mirrored-DB persist block now gates on `matched_data is not None` to avoid a pre-existing latent NameError if the cache-save block raised before matched_data construction. **Enhanced toggle localStorage key now profile-scoped:** `soulsync-library-view-mode` was global — two admin profiles would share one preference. Wrapped in `_libraryViewModeKey()` which appends `:${currentProfile.id}` when a profile is loaded, falls back to the unsuffixed key otherwise (preserves pre-multi-profile saved values). Tests: - 12 new in `tests/discovery/test_manual_match.py` pinning both helpers. - 3 new in `tests/metadata/test_relevance.py` pinning the `prefer_known_duration` semantics. - `test_search_tracks_with_artist_prefers_results_with_known_length` renamed to `_does_not_resort_by_length` since the sort moved out of this method. 664 tests pass across discovery + metadata suites. --- core/discovery/manual_match.py | 70 ++++++++++++++ core/metadata/relevance.py | 18 +++- core/musicbrainz_search.py | 21 ++--- tests/discovery/test_manual_match.py | 106 ++++++++++++++++++++++ tests/metadata/test_musicbrainz_search.py | 18 ++-- tests/metadata/test_relevance.py | 53 ++++++++++- web_server.py | 80 ++++++++-------- webui/static/library.js | 17 +++- 8 files changed, 317 insertions(+), 66 deletions(-) create mode 100644 core/discovery/manual_match.py create mode 100644 tests/discovery/test_manual_match.py diff --git a/core/discovery/manual_match.py b/core/discovery/manual_match.py new file mode 100644 index 00000000..8c56bbdf --- /dev/null +++ b/core/discovery/manual_match.py @@ -0,0 +1,70 @@ +"""Helpers for Fix-popup manual match persistence. + +When the user manually fixes a mirrored-playlist discovery via the Fix +popup, two questions land at the web_server route layer that are easier +to test in isolation: + +1. *Which metadata source did the manual match come from?* — the popup + cascade queries the user's primary source first, then Spotify / + Deezer / iTunes / MusicBrainz as fallbacks; each search endpoint + stamps `source` on its rows but the MBID-paste lookup uses a lean + flat shape that doesn't carry it. `derive_manual_match_provider` + collapses the fallback chain into a single string. + +2. *Should the discovery layer re-run for this track when the current + active provider differs from the cached one?* — re-running silently + overwrites the user's deliberate pick with whatever the auto-search + ranks first, so manual matches are exempt regardless of provider + drift. `is_drifted_for_redo` encapsulates the decision. +""" + +from __future__ import annotations + +from typing import Any, Dict, Optional + + +def derive_manual_match_provider( + payload_track: Dict[str, Any], + active_provider: Optional[str], +) -> str: + """Return the provider string to stamp on a manually-fixed match. + + Resolution order: + 1. ``payload_track['source']`` — every *_search_tracks endpoint + sets this; the MBID-paste path doesn't. + 2. ``active_provider`` — what the user has configured as their + primary discovery source. + 3. ``'spotify'`` — last-ditch default matching the historic + hardcode (so behaviour is identical when both upstream + signals are absent). + """ + if not isinstance(payload_track, dict): + payload_track = {} + source = payload_track.get('source') + if source: + return source + if active_provider: + return active_provider + return 'spotify' + + +def is_drifted_for_redo( + extra_data: Optional[Dict[str, Any]], + active_provider: Optional[str], +) -> bool: + """Return True when a cached discovery entry should be treated as + stale because the user's active provider has changed since it was + cached AND the entry isn't a manual match. + + Manual matches are *always* considered fresh: re-running discovery + against the current source would overwrite the user's deliberate + pick with whatever auto-search ranks first. The first Playlist + Pipeline run after a manual fix used to clobber it for exactly + this reason — the check lives here now so it's pinned by tests. + """ + if not isinstance(extra_data, dict): + return False + if extra_data.get('manual_match'): + return False + cached_provider = extra_data.get('provider', 'spotify') + return cached_provider != active_provider diff --git a/core/metadata/relevance.py b/core/metadata/relevance.py index 37aed5e0..d3e68935 100644 --- a/core/metadata/relevance.py +++ b/core/metadata/relevance.py @@ -295,6 +295,7 @@ def rerank_tracks( *, expected_title: str, expected_artist: str, + prefer_known_duration: bool = False, ) -> List[Track]: """Return a copy of ``tracks`` sorted by descending relevance score against the expected title + artist. @@ -304,6 +305,15 @@ def rerank_tracks( fallback when two candidates score identically — the source's popularity signal is still useful as a tiebreak). + ``prefer_known_duration``: when True, recordings with non-zero + ``duration_ms`` are ranked ahead of duplicate-score recordings + that lack length data. Used for MusicBrainz which often has + several recordings per song (single edition, album edition, + compilations, remasters) where some carry length and some don't. + Sort key sits between score and the stable-order tiebreaker so + relevance still wins — length is only a tiebreaker on equal + scores, not a global re-shuffle. + No-op when both ``expected_title`` and ``expected_artist`` are empty (no signal to rank against — return input order).""" if not expected_title and not expected_artist: @@ -312,8 +322,12 @@ def rerank_tracks( (score_track(t, expected_title=expected_title, expected_artist=expected_artist), idx, t) for idx, t in enumerate(tracks) ] - # Sort by score desc; idx asc as tiebreaker preserves stable order. - scored.sort(key=lambda x: (-x[0], x[1])) + if prefer_known_duration: + # Sort key: score desc, has-length first (0 before 1), idx asc. + scored.sort(key=lambda x: (-x[0], 0 if (x[2].duration_ms or 0) > 0 else 1, x[1])) + else: + # Sort by score desc; idx asc as tiebreaker preserves stable order. + scored.sort(key=lambda x: (-x[0], x[1])) return [t for _score, _idx, t in scored] diff --git a/core/musicbrainz_search.py b/core/musicbrainz_search.py index ff39a827..8e7bf9bd 100644 --- a/core/musicbrainz_search.py +++ b/core/musicbrainz_search.py @@ -772,12 +772,11 @@ class MusicBrainzSearchClient: When only one field is present: bare-query mode directly — same recall-over-precision tradeoff the old single-path took. - Results are stable-sorted to prefer entries with non-zero - `duration_ms`. MB often has several recordings per song (single - release, album release, compilations, remasters) and not every - recording carries length data. Without this, the first match can - be a length-less duplicate while a sibling recording with the - real 3:04 sits two rows down. + Callers wanting MB-specific length-preference ordering (multi- + edition recordings where some lack length data) should pass + ``prefer_known_duration=True`` to ``rerank_tracks`` downstream + — a stable sort here would just be re-sorted away by the rerank + pass anyway. """ if not track and not artist: return [] @@ -790,13 +789,11 @@ class MusicBrainzSearchClient: results = self._search_tracks_text( track, artist, limit, strict=False, min_score=20 ) - else: - results = self._search_tracks_text( - track, artist or None, limit, strict=False, min_score=20 - ) + return results - results.sort(key=lambda t: 0 if (t.duration_ms or 0) > 0 else 1) - return results + return self._search_tracks_text( + track, artist or None, limit, strict=False, min_score=20 + ) def _pick_representative_release(self, releases: List[Dict[str, Any]]) -> Optional[Dict[str, Any]]: """Pick the best release out of a release-group's editions. diff --git a/tests/discovery/test_manual_match.py b/tests/discovery/test_manual_match.py new file mode 100644 index 00000000..8bc34b09 --- /dev/null +++ b/tests/discovery/test_manual_match.py @@ -0,0 +1,106 @@ +"""Tests for core.discovery.manual_match helpers. + +These pin the contract for two route-layer decisions lifted out of +web_server.py so the Fix-popup → mirrored-playlist back-sync flow is +testable in isolation (per kettui's standing rule that web_server.py +behavior is reproduced in core/ modules with real unit tests, not by +AST-parsing the route file). +""" + +from core.discovery.manual_match import ( + derive_manual_match_provider, + is_drifted_for_redo, +) + + +# --------------------------------------------------------------------------- +# derive_manual_match_provider +# --------------------------------------------------------------------------- + + +def test_derive_uses_payload_source_when_present(): + """Search-endpoint payloads always stamp `source` — that's the + authoritative provider for a manual match.""" + payload = {'id': 'rec-1', 'source': 'musicbrainz', 'name': 'Track'} + assert derive_manual_match_provider(payload, 'spotify') == 'musicbrainz' + + +def test_derive_falls_back_to_active_when_payload_missing_source(): + """MBID-paste path returns a lean flat shape without `source`. Fall + back to the user's active discovery source so the cached match + matches whatever provider next compares against it.""" + payload = {'id': 'mb-mbid', 'name': 'Track'} # no `source` + assert derive_manual_match_provider(payload, 'musicbrainz') == 'musicbrainz' + + +def test_derive_falls_back_to_spotify_when_both_missing(): + """Last-ditch default matches the historic hardcode so behaviour is + identical when both upstream signals are absent (e.g. broken + config, missing active source).""" + assert derive_manual_match_provider({}, None) == 'spotify' + assert derive_manual_match_provider({}, '') == 'spotify' + + +def test_derive_handles_non_dict_payload_gracefully(): + """Defensive — caller passes whatever request.get_json() returned.""" + assert derive_manual_match_provider(None, 'spotify') == 'spotify' + assert derive_manual_match_provider('not-a-dict', 'musicbrainz') == 'musicbrainz' + + +def test_derive_payload_source_wins_even_when_active_set(): + """`source` on payload is authoritative — even if the user's active + source changed mid-flow, the match came from whatever the popup + cascade actually queried.""" + payload = {'source': 'itunes'} + assert derive_manual_match_provider(payload, 'spotify') == 'itunes' + + +# --------------------------------------------------------------------------- +# is_drifted_for_redo +# --------------------------------------------------------------------------- + + +def test_drift_redo_when_provider_changed_and_not_manual(): + """Standard provider-drift case: cached provider differs from + active, no manual flag → re-discover so active source's IDs / + artwork take effect.""" + extra = {'discovered': True, 'provider': 'spotify'} + assert is_drifted_for_redo(extra, 'musicbrainz') is True + + +def test_drift_no_redo_when_provider_matches(): + """Same provider → cached entry is fresh, no redo needed.""" + extra = {'discovered': True, 'provider': 'spotify'} + assert is_drifted_for_redo(extra, 'spotify') is False + + +def test_drift_no_redo_when_manual_match_even_if_provider_drifted(): + """The crux of the bug fix: manual matches are exempt from + provider-drift redo. Re-running would overwrite the user's pick.""" + extra = {'discovered': True, 'provider': 'musicbrainz', 'manual_match': True} + assert is_drifted_for_redo(extra, 'spotify') is False + + +def test_drift_no_redo_when_manual_match_with_matching_provider(): + """Manual + provider match: trivially fresh.""" + extra = {'discovered': True, 'provider': 'spotify', 'manual_match': True} + assert is_drifted_for_redo(extra, 'spotify') is False + + +def test_drift_no_redo_when_extra_data_missing(): + """No cached entry → nothing to drift from.""" + assert is_drifted_for_redo(None, 'spotify') is False + assert is_drifted_for_redo({}, 'spotify') is False + + +def test_drift_handles_non_dict_extra_data(): + """Defensive — extra_data deserialisation can land non-dict shapes.""" + assert is_drifted_for_redo('not-a-dict', 'spotify') is False + + +def test_drift_default_provider_is_spotify_when_absent(): + """Historic cached entries may pre-date the provider column being + populated — treat absent provider as 'spotify' (the legacy default).""" + extra = {'discovered': True} # no provider field + assert is_drifted_for_redo(extra, 'spotify') is False + assert is_drifted_for_redo(extra, 'musicbrainz') is True diff --git a/tests/metadata/test_musicbrainz_search.py b/tests/metadata/test_musicbrainz_search.py index 322cc284..96d2c220 100644 --- a/tests/metadata/test_musicbrainz_search.py +++ b/tests/metadata/test_musicbrainz_search.py @@ -1050,11 +1050,12 @@ def test_search_tracks_with_artist_falls_back_to_bare_when_strict_empty(): assert tracks[0].id == 'rec-canonical' -def test_search_tracks_with_artist_prefers_results_with_known_length(): - """MB has multiple recordings per song (single release, album release, - compilations) and not every recording carries length data. Stable - sort moves length-known entries ahead of length-zero duplicates so - the user sees the actionable 3:04 row first, not the 0:00 sibling.""" +def test_search_tracks_with_artist_does_not_resort_by_length(): + """Length-preference ordering lives downstream in + ``rerank_tracks(..., prefer_known_duration=True)`` — sorting here + would be re-sorted away by rerank anyway, so this method preserves + the order MB returned. Pin the contract: this method does not + re-shuffle by duration_ms.""" client = MusicBrainzSearchClient() client._client = MagicMock() client._client.search_recording.return_value = [ @@ -1067,10 +1068,9 @@ def test_search_tracks_with_artist_prefers_results_with_known_length(): tracks = client.search_tracks_with_artist('Coffee Break', 'Zeds Dead', limit=10) - # rec-with-length must surface first even though MB scored it lower - assert tracks[0].id == 'rec-with-length' - assert tracks[0].duration_ms == 184000 - assert tracks[1].id == 'rec-no-length' + # MB's order is preserved here — rerank applies length-pref downstream. + assert tracks[0].id == 'rec-no-length' + assert tracks[1].id == 'rec-with-length' def test_search_tracks_with_artist_handles_missing_artist(): diff --git a/tests/metadata/test_relevance.py b/tests/metadata/test_relevance.py index 38e71555..4c6f9394 100644 --- a/tests/metadata/test_relevance.py +++ b/tests/metadata/test_relevance.py @@ -50,6 +50,7 @@ def _track( album: str = 'Unknown', album_type: str = 'album', track_id: str = 't', + duration_ms: int = 200000, ) -> Track: """Tiny Track factory — keeps test bodies focused on the fields under test.""" @@ -58,7 +59,7 @@ def _track( name=name, artists=[artist], album=album, - duration_ms=200000, + duration_ms=duration_ms, album_type=album_type, ) @@ -366,6 +367,56 @@ class TestRerankTracks: ranked = rerank_tracks([a, b], expected_title='Track', expected_artist='Artist') assert [t.id for t in ranked] == ['first', 'second'] + def test_prefer_known_duration_promotes_length_known_on_ties(self): + """MB has multiple recordings per song where some lack length + data. With ``prefer_known_duration=True`` and equal relevance + scores, the recording with non-zero duration_ms must surface + ahead of the length-less sibling.""" + no_length = _track('Track', artist='Artist', track_id='no-len', duration_ms=0) + with_length = _track('Track', artist='Artist', track_id='with-len', duration_ms=184000) + ranked = rerank_tracks( + [no_length, with_length], + expected_title='Track', + expected_artist='Artist', + prefer_known_duration=True, + ) + assert [t.id for t in ranked] == ['with-len', 'no-len'] + + def test_prefer_known_duration_does_not_override_relevance(self): + """Length-preference is a TIEBREAKER, not a global resort. A + length-less track that scores higher on relevance must still + win — only equal-score pairs use length as the deciding bit.""" + # Wrong-artist length-known: scored low. Right-artist length-less: + # scored high. + length_known_cover = _track( + 'Track', artist='Karaoke Channel', album='Karaoke Hits', + album_type='compilation', track_id='cover', duration_ms=184000, + ) + length_less_real = _track( + 'Track', artist='Real Artist', track_id='real', duration_ms=0, + ) + ranked = rerank_tracks( + [length_known_cover, length_less_real], + expected_title='Track', + expected_artist='Real Artist', + prefer_known_duration=True, + ) + assert ranked[0].id == 'real', "relevance must beat length-pref" + + def test_prefer_known_duration_default_off(self): + """Default behaviour unchanged — length-preference is opt-in + for MB callers; Spotify / iTunes / Deezer don't need it + because their search results always include length.""" + no_length = _track('Track', artist='Artist', track_id='no-len', duration_ms=0) + with_length = _track('Track', artist='Artist', track_id='with-len', duration_ms=184000) + ranked = rerank_tracks( + [no_length, with_length], + expected_title='Track', + expected_artist='Artist', + ) + # Default: stable input order, no length-pref re-shuffle. + assert [t.id for t in ranked] == ['no-len', 'with-len'] + # --------------------------------------------------------------------------- # filter_and_rerank — score floor convenience diff --git a/web_server.py b/web_server.py index 0fd2ff69..7e6209ed 100644 --- a/web_server.py +++ b/web_server.py @@ -19416,12 +19416,19 @@ def search_musicbrainz_tracks(): # Local rerank — same helper Deezer / iTunes use. Penalises # cover / karaoke / tribute patterns + boosts exact-artist match. + # `prefer_known_duration=True` is MB-specific: MB has multiple + # recordings per song (single / album / compilation / remaster + # editions) and not every recording carries length data. The + # flag promotes length-known recordings ahead of length-less + # duplicates when relevance scores tie, so the user sees the + # actionable 3:04 row before the 0:00 sibling. if track_q or artist_q: from core.metadata.relevance import rerank_tracks tracks = rerank_tracks( tracks, expected_title=track_q, expected_artist=artist_q, + prefer_known_duration=True, ) tracks_dict = [{ @@ -24315,6 +24322,20 @@ def update_youtube_discovery_match(): logger.info(f"Manual match updated: youtube - {identifier} - track {track_index}") logger.info(f" → {result['spotify_artist']} - {result['spotify_track']}") + # See core.discovery.manual_match — Fix-popup matches can come from + # any metadata source (primary first, then Spotify / Deezer / iTunes + # / MusicBrainz as fallbacks). Hardcoding 'spotify' here used to + # make every non-Spotify manual match look provider-drifted on the + # next prepare-discovery, which triggered automatic re-discovery + # that overwrote the user's pick. Computed once before the try + # block so both the cache-save path AND the mirrored-DB save below + # (in the except fallback case) see the same value. + from core.discovery.manual_match import derive_manual_match_provider + match_source = derive_manual_match_provider( + spotify_track, _get_active_discovery_source() + ) + matched_data = None + # Save manual fix to discovery cache so it appears in discovery pool try: # Get original track name from the YouTube/source track data @@ -24349,20 +24370,6 @@ def update_youtube_discovery_match(): album_obj['image_url'] = image_url album_obj['images'] = [{'url': image_url}] - # Manual fixes can come from any metadata source — the Fix-popup - # cascade queries the user's primary first, then Spotify / Deezer / - # iTunes / MusicBrainz as fallbacks. Each search endpoint stamps - # `source` on its results; the MBID-paste lookup doesn't, so fall - # back to the active discovery source. Hardcoding 'spotify' here - # used to make every non-Spotify manual match look like it had - # provider-drifted on the next prepare-discovery, triggering - # automatic re-discovery that overwrote the user's manual pick. - match_source = ( - spotify_track.get('source') - or _get_active_discovery_source() - or 'spotify' - ) - matched_data = { 'id': spotify_track['id'], 'name': spotify_track['name'], @@ -24380,16 +24387,12 @@ def update_youtube_discovery_match(): logger.info(f"Manual fix saved to discovery cache: {original_name} by {original_artist}") except Exception as cache_err: logger.error(f"Error saving manual fix to discovery cache: {cache_err}") - # match_source needs to exist for the mirrored-DB block below even - # if cache save failed — use the same fallback chain. - match_source = ( - spotify_track.get('source') - or _get_active_discovery_source() - or 'spotify' - ) - # Persist manual fix to DB for mirrored playlists - if identifier.startswith('mirrored_'): + # Persist manual fix to DB for mirrored playlists. Skips when the + # cache-save block raised before matched_data was constructed — + # without the payload there's nothing to persist, and re-deriving + # it here would duplicate the construction logic above. + if matched_data is not None and identifier.startswith('mirrored_'): try: tracks = state['playlist']['tracks'] if track_index < len(tracks): @@ -33688,21 +33691,20 @@ def prepare_mirrored_discovery(playlist_id): pre_discovered_count = 0 has_pending = False + from core.discovery.manual_match import is_drifted_for_redo + for idx, track in enumerate(tracks): extra = track.get('extra_data') if extra and extra.get('discovered'): cached_provider = extra.get('provider', 'spotify') - is_manual = bool(extra.get('manual_match')) - # If the cached result was discovered by a different provider than the - # currently active one, treat it as pending so re-discovery uses the - # correct source (IDs, album data, images differ between providers). - # Manual matches are exempt: the user explicitly picked that record, - # so re-discovery would overwrite their intentional fix every cycle. - # Without this guard, fixing a track via the Fix popup correctly - # downloads it once, but the next Playlist Pipeline run re-marks it - # "Provider Changed" and reverts the manual map. - if cached_provider != _current_provider and not is_manual: + # See core.discovery.manual_match.is_drifted_for_redo — + # provider-drift triggers re-discovery so the active source's + # IDs / artwork take effect, but manual matches are exempt: + # re-running would overwrite the user's deliberate pick with + # whatever auto-search ranks first. Pre-fix, every Playlist + # Pipeline run clobbered manual fixes for exactly this reason. + if is_drifted_for_redo(extra, _current_provider): has_pending = True dur = track.get('duration_ms', 0) pre_discovered_results.append({ @@ -33784,16 +33786,14 @@ def prepare_mirrored_discovery(playlist_id): 'confidence': 0, }) - # Treat as cached if at least one track was discovered by the current provider, - # OR if any track carries a manual_match (those are exempt from provider-drift - # re-discovery above, so they count as cached regardless of cached_provider). + # Treat as cached when at least one track has a non-drifted cached + # discovery — same predicate the per-track loop above uses (inverse + # polarity), so a future field change only has to land in + # core.discovery.manual_match.is_drifted_for_redo. has_cached = any( t.get('extra_data') and (t['extra_data'].get('discovered') or t['extra_data'].get('discovery_attempted')) and - ( - t['extra_data'].get('provider', 'spotify') == _current_provider - or t['extra_data'].get('manual_match') - ) + not is_drifted_for_redo(t['extra_data'], _current_provider) for t in tracks ) diff --git a/webui/static/library.js b/webui/static/library.js index dac0467a..a979233c 100644 --- a/webui/static/library.js +++ b/webui/static/library.js @@ -885,7 +885,7 @@ function navigateToArtistDetail(artistId, artistName, sourceOverride = null, opt // disabled in private-browsing modes. let _preferEnhanced = false; try { - _preferEnhanced = localStorage.getItem('soulsync-library-view-mode') === 'enhanced'; + _preferEnhanced = localStorage.getItem(_libraryViewModeKey()) === 'enhanced'; } catch (_) { /* localStorage unavailable */ } // Navigate to artist detail page @@ -2925,10 +2925,23 @@ function toggleEnhancedView(enabled) { // Persist the choice so the next artist click (and the next page reload) // honours it instead of always reverting to Standard. try { - localStorage.setItem('soulsync-library-view-mode', enabled ? 'enhanced' : 'standard'); + localStorage.setItem(_libraryViewModeKey(), enabled ? 'enhanced' : 'standard'); } catch (_) { /* localStorage unavailable */ } } +// localStorage key for the Enhanced/Standard toggle, scoped to the active +// profile so different admin profiles can keep different defaults. Falls +// back to an unsuffixed key when no profile is loaded (matches the original +// behaviour for any pre-multi-profile saved value). +function _libraryViewModeKey() { + const pid = (typeof currentProfile === 'object' && currentProfile && currentProfile.id != null) + ? currentProfile.id + : null; + return pid != null + ? `soulsync-library-view-mode:${pid}` + : 'soulsync-library-view-mode'; +} + async function loadEnhancedViewData(artistId) { const container = document.getElementById('enhanced-view-container'); if (!container) return; From 6125ef8834c60ef80c939557bc3f7ba9a0a32b1c Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Wed, 27 May 2026 08:13:28 -0700 Subject: [PATCH 5/6] MB rerank: prefer_known_duration is now a score boost, not a tiebreaker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Live smoke against `/api/musicbrainz/search_tracks?track=Coffee+Break&artist=Zeds+Dead` exposed the edge case the tiebreaker implementation couldn't reach: The canonical Zeds Dead "Coffee Break" recording (mbid 6e2d4a70, length 184000ms) lives on the Coffee Break Single release — album_type='single', which carries a 0.85 album_type_weight in `score_track`. A sibling length-less recording (mbid 3b89bf3c) lives on an Album release — album_type='album', weight 1.0. After multiplying by EXACT_ARTIST_BOOST the canonical sat at 1.275 while the length-less sibling sat at 1.5. The previous tiebreaker only kicked in on equal scores, so the length-less album edition wins and the user sees 0:00 first instead of the actionable 3:04 row. Bug reproduced: ordering came out length-less / canonical / Omar-LinX-collab. Switched `prefer_known_duration` to a 1.25x score boost on recordings with non-zero duration_ms. The multiplier is sized above the album-vs-single weight spread (0.176) so length-known recordings can overcome an album-type penalty when scores would otherwise tie on title + artist match, but stays small enough that cover/karaoke penalty (0.05) and variant-tag penalty (0.85) still dominate — a length-known tribute still loses to a length-less canonical. Post-fix live response: 6e2d4a70 (canonical, 184000ms) sits first, 8ec2ce3f (Zeds Dead + Omar LinX collab, 153000ms) second, 3b89bf3c (length-less album edition) third. Verified Björk diacritic fallback path unaffected — `Bjork` + `Army of Me` still cascades strict-empty → bare and returns all 10 Björk recordings. 122 metadata tests pass — the three `prefer_known_duration` cases were designed to pin behaviour, not the specific multiplier value, so they all still pass under the boost implementation: ties promote length-known, relevance still beats length-pref, default-off behaviour unchanged. --- core/metadata/relevance.py | 38 ++++++++++++++++++++++++++------------ 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/core/metadata/relevance.py b/core/metadata/relevance.py index d3e68935..43ab430b 100644 --- a/core/metadata/relevance.py +++ b/core/metadata/relevance.py @@ -306,13 +306,20 @@ def rerank_tracks( popularity signal is still useful as a tiebreak). ``prefer_known_duration``: when True, recordings with non-zero - ``duration_ms`` are ranked ahead of duplicate-score recordings - that lack length data. Used for MusicBrainz which often has - several recordings per song (single edition, album edition, - compilations, remasters) where some carry length and some don't. - Sort key sits between score and the stable-order tiebreaker so - relevance still wins — length is only a tiebreaker on equal - scores, not a global re-shuffle. + ``duration_ms`` get a score boost. Used for MusicBrainz, which + often has several recordings per song (single edition, album + edition, compilations, remasters) where some carry length data + and some don't. The boost is set above the album_type weight + spread so length-known recordings can beat length-less + siblings even when the sibling sits on a higher-weighted + album-type — real case: Zeds Dead "Coffee Break" canonical + recording lives on the Single release (album_type='single', + weight 0.85) while a length-less sibling lives on an Album + release (weight 1.0). Without the boost, the length-less album + edition wins and the user sees 0:00 instead of 3:04. Cover / + karaoke penalties dominate the boost (their penalty is 0.05) + so a length-known tribute still loses to a length-less + canonical match. No-op when both ``expected_title`` and ``expected_artist`` are empty (no signal to rank against — return input order).""" @@ -323,11 +330,18 @@ def rerank_tracks( for idx, t in enumerate(tracks) ] if prefer_known_duration: - # Sort key: score desc, has-length first (0 before 1), idx asc. - scored.sort(key=lambda x: (-x[0], 0 if (x[2].duration_ms or 0) > 0 else 1, x[1])) - else: - # Sort by score desc; idx asc as tiebreaker preserves stable order. - scored.sort(key=lambda x: (-x[0], x[1])) + # Multiplier sized above the album-type weight spread (album 1.0 + # vs single 0.85 = ~18%) so length-known recordings can overcome + # the album-vs-single penalty when scores would otherwise tie on + # title + artist match. Penalty multipliers (cover/karaoke=0.05, + # variant=0.85) still dominate, so this only flips order among + # close-relevance siblings — exactly the MB-duplicate case. + scored = [ + (score * 1.25 if (t.duration_ms or 0) > 0 else score, idx, t) + for score, idx, t in scored + ] + # Sort by score desc; idx asc as tiebreaker preserves stable order. + scored.sort(key=lambda x: (-x[0], x[1])) return [t for _score, _idx, t in scored] From 65d7756da27176698a3467edc1b8c92aedb41eac Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Wed, 27 May 2026 08:33:36 -0700 Subject: [PATCH 6/6] Resolve pre-existing ruff lint errors blocking CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five pre-existing lint errors on dev baseline (all introduced May 25-26 before this branch was cut) were blocking CI on this PR. Cleared as courtesy fixes so the merge isn't gated on unrelated tech debt: - web_server.py:22613 — F811 duplicate `urlparse` import inside `_parse_itunes_link_url` (already imported at module top, line 20). Removed from the inline `from urllib.parse import parse_qs, urlparse`; kept `parse_qs` since that one is only used here. - core/listenbrainz_manager.py:746 — S110 silenced with `# noqa: S110 — best-effort lookup, delete proceeds either way`. Matches the existing project convention used in web_server.py:1693, core/watchlist/auto_scan.py:463, core/library_reorganize.py:548. - core/playlists/sources/listenbrainz.py:236 — B905 `zip()` without explicit `strict=`. Added `strict=False` — preserves existing behaviour where `matched` can legitimately be shorter than `match_indices` on partial discover failure. - core/playlists/sources/listenbrainz.py:273 — S110 silenced with `# noqa: S110 — caller falls back to last cached playlist on refresh failure`. - core/playlists/sources/soulsync_discovery.py:105 — S110 silenced with `# noqa: S110 — manager persists last_generation_error on failure; surface existing snapshot`. The existing multi-line comment that already explained the swallow was rolled into the noqa justification so the rule + reason live on one line. Ruff `python -m ruff check .` now passes; 664 discovery + metadata tests still pass. --- core/listenbrainz_manager.py | 2 +- core/playlists/sources/listenbrainz.py | 4 ++-- core/playlists/sources/soulsync_discovery.py | 4 +--- web_server.py | 2 +- 4 files changed, 5 insertions(+), 7 deletions(-) diff --git a/core/listenbrainz_manager.py b/core/listenbrainz_manager.py index ddae7ae8..efa16158 100644 --- a/core/listenbrainz_manager.py +++ b/core/listenbrainz_manager.py @@ -743,7 +743,7 @@ class ListenBrainzManager: ) row = cursor.fetchone() playlist_type = row[0] if row else '' - except Exception: + except Exception: # noqa: S110 — best-effort lookup, delete proceeds either way pass # Delete tracks first (SQLite FK CASCADE requires PRAGMA foreign_keys=ON) diff --git a/core/playlists/sources/listenbrainz.py b/core/playlists/sources/listenbrainz.py index 0c5f72ff..838cda15 100644 --- a/core/playlists/sources/listenbrainz.py +++ b/core/playlists/sources/listenbrainz.py @@ -233,7 +233,7 @@ class ListenBrainzPlaylistSource(PlaylistSource): return tracks out = list(tracks) - for slot_idx, result in zip(match_indices, matched): + for slot_idx, result in zip(match_indices, matched, strict=False): if not result: continue track = out[slot_idx] @@ -270,7 +270,7 @@ class ListenBrainzPlaylistSource(PlaylistSource): return None try: manager.update_all_playlists() - except Exception: + except Exception: # noqa: S110 — caller falls back to last cached playlist on refresh failure pass return self.get_playlist(playlist_id) diff --git a/core/playlists/sources/soulsync_discovery.py b/core/playlists/sources/soulsync_discovery.py index c648830c..d2cfec25 100644 --- a/core/playlists/sources/soulsync_discovery.py +++ b/core/playlists/sources/soulsync_discovery.py @@ -102,9 +102,7 @@ class SoulSyncDiscoveryPlaylistSource(PlaylistSource): variant=record.variant, profile_id=record.profile_id, ) - except Exception: - # Manager already persists ``last_generation_error`` on - # failure; surface the existing snapshot regardless. + except Exception: # noqa: S110 — manager persists last_generation_error on failure; surface existing snapshot pass return self.get_playlist(playlist_id) diff --git a/web_server.py b/web_server.py index 7e6209ed..268ec877 100644 --- a/web_server.py +++ b/web_server.py @@ -22610,7 +22610,7 @@ _APPLE_MUSIC_BUNDLE_SCRAPE_CAP = 8 def _parse_itunes_link_url(url): """Return {'type': 'album'|'track'|'playlist', 'id': str} for supported Apple links.""" import re - from urllib.parse import parse_qs, urlparse + from urllib.parse import parse_qs raw = (url or '').strip() if not raw: