Merge pull request #708 from Nezreka/fix/community-feedback-mbrainz-mirrored-enhanced

Fix/community feedback mbrainz mirrored enhanced

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

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.

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)

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,22 @@ 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`` 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)."""
     if not expected_title and not expected_artist:
@@ -312,6 +329,17 @@ def rerank_tracks(
         (score_track(t, expected_title=expected_title, expected_artist=expected_artist), idx, t)
         for idx, t in enumerate(tracks)
     ]
+    if prefer_known_duration:
+        # 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]

core/musicbrainz_search.py

@@ -757,19 +757,43 @@ 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:"<t>"
+        AND artist:"<a>"`) 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.
+
+        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 []
-        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
+                )
+            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.

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)
 

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)
 

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
 # ---------------------------------------------------------------------------

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

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:"<t>" AND artist:"<a>"`). 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_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 = [
+        {'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)
+
+    # 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():
-    """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')

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

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 = [{
@@ -22603,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:
@@ -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
@@ -24356,7 +24377,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(
@@ -24367,8 +24388,11 @@ def update_youtube_discovery_match():
         except Exception as cache_err:
             logger.error(f"Error saving manual fix to discovery cache: {cache_err}")
 
-        # 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):
@@ -24377,7 +24401,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,
@@ -33667,15 +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')
 
-                # 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:
+                # 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({
@@ -33757,11 +33786,14 @@ 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 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
+            not is_drifted_for_redo(t['extra_data'], _current_provider)
             for t in tracks
         )
 

webui/static/helper.js

@@ -3415,6 +3415,9 @@ 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.' },
         { 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.' },

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(_libraryViewModeKey()) === '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,25 @@ 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(_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) {