diff --git a/.github/workflows/docker-publish.yml b/.github/workflows/docker-publish.yml index d56a76d5..d11761e1 100644 --- a/.github/workflows/docker-publish.yml +++ b/.github/workflows/docker-publish.yml @@ -9,9 +9,9 @@ on: workflow_dispatch: inputs: version_tag: - description: 'Version tag (e.g. 2.5.6)' + description: 'Version tag (e.g. 2.5.7)' required: true - default: '2.5.6' + default: '2.5.7' jobs: build-and-push: diff --git a/Dockerfile b/Dockerfile index a225b8b0..fdf5a21f 100644 --- a/Dockerfile +++ b/Dockerfile @@ -73,8 +73,14 @@ COPY --chown=soulsync:soulsync --from=webui-builder /app/webui/static/dist /app/ # pre-baked directory would crash the container into a restart loop on # rootless Docker/Podman where in-container "root" can't write to /app. # Pre-baking the dir here makes the entrypoint mkdir a guaranteed no-op. -RUN mkdir -p /app/config /app/data /app/logs /app/downloads /app/Transfer /app/Staging /app/MusicVideos /app/scripts && \ - chown soulsync:soulsync /app/config /app/data /app/logs /app/downloads /app/Transfer /app/Staging /app/MusicVideos /app/scripts +# NOTE: /app/Stream is the transient single-file streaming cache used by +# the basic-search "Play" flow (cleared per use, never persistent). It's +# created lazily by `core/streaming/prepare.py` via `os.makedirs`, which +# fails silently on rootless Docker where the soulsync UID can't write +# to /app — playback then errors out with no obvious cause. Pre-baking +# at build time (when the layer is owned by root) avoids that path. +RUN mkdir -p /app/config /app/data /app/logs /app/downloads /app/Transfer /app/Staging /app/Stream /app/MusicVideos /app/scripts && \ + chown soulsync:soulsync /app/config /app/data /app/logs /app/downloads /app/Transfer /app/Staging /app/Stream /app/MusicVideos /app/scripts # Create defaults directory and copy template files # These will be used by entrypoint.sh to initialize empty volumes diff --git a/api/downloads.py b/api/downloads.py index 69a9846d..29a6989d 100644 --- a/api/downloads.py +++ b/api/downloads.py @@ -125,6 +125,9 @@ def register_routes(bp): if not soulseek: return api_error("NOT_AVAILABLE", "Soulseek client not configured.", 503) + current_app.logger.info( + f"[CancelTrigger:api.public_cancel] download_id={download_id} username={username}" + ) ok = run_async(soulseek.cancel_download(download_id, username, remove=True)) if ok: return api_success({"message": "Download cancelled."}) diff --git a/core/amazon_client.py b/core/amazon_client.py index 8d85e3cd..2414051e 100644 --- a/core/amazon_client.py +++ b/core/amazon_client.py @@ -327,7 +327,7 @@ class AmazonClient: def search_tracks(self, query: str, limit: int = 20) -> List[Track]: _rate_limit() - items = self.search_raw(query, types="track,album") + items = self.search_raw(query, types="track") track_pairs: List[tuple] = [] # (Track, album_asin) seen_album_asins: List[str] = [] for item in items: @@ -360,7 +360,7 @@ class AmazonClient: def search_artists(self, query: str, limit: int = 20) -> List[Artist]: _rate_limit() - items = self.search_raw(query, types="track,album") + items = self.search_raw(query, types="track") seen: Dict[str, Artist] = {} artist_album_asin: Dict[str, str] = {} # artist name → first album ASIN seen for item in items: @@ -384,14 +384,14 @@ class AmazonClient: def search_albums(self, query: str, limit: int = 20) -> List[Album]: _rate_limit() - items = self.search_raw(query, types="track,album") + items = self.search_raw(query, types="track") album_candidates: List[tuple] = [] # (Album, asin) seen_keys: set = set() for item in items: - if not item.is_album: + album_asin = item.album_asin + raw_name = item.album_name + if not raw_name: continue - album_asin = item.album_asin or item.asin - raw_name = item.album_name or item.title display_name = _strip_edition(raw_name) artist = _primary_artist(item.artist_name) # Collapse Explicit/Clean variants: same normalised name + artist = same album @@ -519,7 +519,7 @@ class AmazonClient: artist_name = _primary_artist(streams[0].artist) try: search_items = self.search_raw( - f"{album_name} {artist_name}", types="track,album" + f"{album_name} {artist_name}", types="track" ) for item in search_items: if item.album_asin == asin and item.duration_seconds: @@ -533,12 +533,12 @@ class AmazonClient: "name": _strip_edition(s.title), "artists": [{"name": _primary_artist(s.artist), "id": ""}], "duration_ms": duration_map.get(s.asin, 0), - "track_number": s.track_number, + "track_number": s.track_number if s.track_number is not None else idx + 1, "disc_number": s.disc_number, "release_date": s.date or "", "isrc": s.isrc, } - for s in streams + for idx, s in enumerate(streams) ] return {"items": items, "total": len(items), "limit": 50, "next": None} @@ -547,7 +547,7 @@ class AmazonClient: _rate_limit() search_name = _unslugify(artist_name) try: - items = self.search_raw(search_name, types="track,album") + items = self.search_raw(search_name, types="track") except AmazonClientError: return None name_lower = search_name.lower() @@ -577,7 +577,7 @@ class AmazonClient: _rate_limit() search_name = _unslugify(artist_name) try: - items = self.search_raw(f"{search_name} album", types="track,album") + items = self.search_raw(f"{search_name} album", types="track") except AmazonClientError: return [] album_candidates: List[tuple] = [] # (Album, asin) @@ -630,7 +630,7 @@ class AmazonClient: """Return an album cover as artist image stand-in (T2Tunes has no artist images).""" search_name = _unslugify(artist_id) try: - items = self.search_raw(search_name, types="track,album") + items = self.search_raw(search_name, types="track") except AmazonClientError: return None name_lower = search_name.lower() @@ -686,20 +686,33 @@ class AmazonClient: def _get_json(self, path: str, params: Optional[Dict[str, Any]] = None) -> Any: url = urljoin(f"{self.base_url}/", path.lstrip("/")) - try: - resp = self.session.get(url, params=params, timeout=self.timeout) - resp.raise_for_status() - except requests.HTTPError as exc: - raise AmazonClientError( - f"HTTP {exc.response.status_code} for {url}" - ) from exc - except requests.RequestException as exc: - raise AmazonClientError(f"Request failed for {url}: {exc}") from exc - try: - return resp.json() - except ValueError as exc: - preview = resp.text[:200].replace("\n", " ") - raise AmazonClientError(f"Response not JSON for {url}: {preview!r}") from exc + last_exc: Optional[Exception] = None + for attempt in range(3): + if attempt: + time.sleep(1.0 * attempt) + try: + resp = self.session.get(url, params=params, timeout=self.timeout) + resp.raise_for_status() + except requests.HTTPError as exc: + body = exc.response.text[:500].replace("\n", " ") + # T2Tunes returns 400 for transient Amazon-side failures — retry those. + if exc.response.status_code == 400 and "Failed to search" in exc.response.text: + logger.debug("T2Tunes transient 400 on attempt %d, retrying: %s", attempt + 1, url) + last_exc = AmazonClientError( + f"HTTP {exc.response.status_code} for {url} — body: {body!r}" + ) + continue + raise AmazonClientError( + f"HTTP {exc.response.status_code} for {url} — body: {body!r}" + ) from exc + except requests.RequestException as exc: + raise AmazonClientError(f"Request failed for {url}: {exc}") from exc + try: + return resp.json() + except ValueError as exc: + preview = resp.text[:200].replace("\n", " ") + raise AmazonClientError(f"Response not JSON for {url}: {preview!r}") from exc + raise last_exc # type: ignore[misc] @staticmethod def _iter_search_items(response: Any) -> Iterator[T2TunesSearchItem]: diff --git a/core/artist_source_detail.py b/core/artist_source_detail.py index d8e8b45c..5576f224 100644 --- a/core/artist_source_detail.py +++ b/core/artist_source_detail.py @@ -52,7 +52,7 @@ def build_source_only_artist_detail( ``jsonify`` or equivalent. Status is 200 on success, 404 when the source's discography lookup returned no releases. """ - resolved_name = (artist_name or artist_id or "").strip() + resolved_name = (artist_name or "").strip() # 1. Image URL via the same helper /api/artist//image uses. image_url: Optional[str] = None @@ -101,6 +101,17 @@ def build_source_only_artist_detail( source_genres = az_artist.get("genres") or [] if not image_url and az_artist.get("images"): image_url = az_artist["images"][0].get("url") + elif source == "musicbrainz": + try: + from core.musicbrainz_search import MusicBrainzSearchClient + mb = MusicBrainzSearchClient() + mb_artist = mb.get_artist(artist_id) + if mb_artist: + if not artist_name and mb_artist.get("name"): + resolved_name = mb_artist["name"] + source_genres = mb_artist.get("genres") or [] + except Exception as e: + logger.debug(f"MusicBrainz artist info lookup failed for {artist_id}: {e}") except Exception as e: logger.debug(f"Source-side artist info lookup failed for {source}:{artist_id}: {e}") diff --git a/core/downloads/cancel.py b/core/downloads/cancel.py index 9536eda4..b375f8fa 100644 --- a/core/downloads/cancel.py +++ b/core/downloads/cancel.py @@ -45,6 +45,9 @@ _TERMINAL_STATUSES = { def cancel_single_download(download_orchestrator, run_async: Callable, download_id: str, username: str) -> bool: """Cancel one specific slskd download (with `remove=True`).""" + logger.info( + f"[CancelTrigger:api.manual_cancel_single] download_id={download_id} username={username}" + ) return run_async(download_orchestrator.cancel_download(download_id, username, remove=True)) diff --git a/core/downloads/candidates.py b/core/downloads/candidates.py index b2e92fcb..52700ca9 100644 --- a/core/downloads/candidates.py +++ b/core/downloads/candidates.py @@ -343,6 +343,10 @@ def attempt_download_with_candidates(task_id, candidates, track, batch_id=None, logger.warning(f"[Modal Worker] Task {task_id} cancelled after download {download_id} started - attempting to cancel download") # Try to cancel the download immediately try: + logger.info( + f"[CancelTrigger:candidates.worker_cancelled_during_download] " + f"download_id={download_id} username={username} task_id={task_id}" + ) deps.run_async(deps.download_orchestrator.cancel_download(download_id, username, remove=True)) logger.warning(f"Successfully cancelled active download {download_id}") except Exception as cancel_error: diff --git a/core/downloads/monitor.py b/core/downloads/monitor.py index 3252f324..79a90d8d 100644 --- a/core/downloads/monitor.py +++ b/core/downloads/monitor.py @@ -197,8 +197,21 @@ class WebUIDownloadMonitor: for op in deferred_ops: try: if op[0] == 'cancel_download': - _, download_id, username = op - logger.debug(f"[Deferred] Cancelling download: {download_id} from {username}") + # Issue #648 diagnostic — `op` now carries a trigger + # label (4-tuple, was 3-tuple) so the next log dump + # tells us WHICH path in `_should_retry_task` is + # firing for users seeing "Tidal downloads failed to + # start" mass-cancels. Label format pinned in commit + # message for grep-ability. + if len(op) >= 4: + _, download_id, username, trigger = op[0], op[1], op[2], op[3] + else: + _, download_id, username = op + trigger = 'unlabeled' + logger.info( + f"[CancelTrigger:monitor.{trigger}] download_id={download_id} " + f"username={username}" + ) run_async(download_orchestrator.cancel_download(download_id, username, remove=True)) logger.debug(f"[Deferred] Successfully cancelled download {download_id}") elif op[0] == 'cleanup_orphan': @@ -353,7 +366,8 @@ class WebUIDownloadMonitor: # Defer slskd cancel to outside the lock if task_username and download_id: - deferred_ops.append(('cancel_download', download_id, task_username)) + deferred_ops.append(('cancel_download', download_id, task_username, + 'not_in_live_transfers_90s')) # Mark current source as used (full filename to match worker format) if task_username and task_filename: @@ -424,7 +438,8 @@ class WebUIDownloadMonitor: # Defer slskd cancel to outside the lock if username and download_id: - deferred_ops.append(('cancel_download', download_id, username)) + deferred_ops.append(('cancel_download', download_id, username, + 'errored_state_retry')) # Mark current source as used to prevent retry loops # CRITICAL: Use full filename (not basename) to match worker's source_key format @@ -527,7 +542,8 @@ class WebUIDownloadMonitor: # Defer slskd cancel to outside the lock if username and download_id: - deferred_ops.append(('cancel_download', download_id, username)) + deferred_ops.append(('cancel_download', download_id, username, + 'queued_state_timeout')) # UNIFIED RETRY LOGIC: Handle timeout retry exactly like error retry # Mark current source as used to prevent retry loops @@ -615,7 +631,8 @@ class WebUIDownloadMonitor: # Defer slskd cancel to outside the lock if username and download_id: - deferred_ops.append(('cancel_download', download_id, username)) + deferred_ops.append(('cancel_download', download_id, username, + 'stuck_at_0pct_timeout')) # UNIFIED RETRY LOGIC: Handle 0% timeout retry exactly like error retry # Mark current source as used to prevent retry loops @@ -704,7 +721,8 @@ class WebUIDownloadMonitor: download_id = task.get('download_id') if username and download_id: - deferred_ops.append(('cancel_download', download_id, username)) + deferred_ops.append(('cancel_download', download_id, username, + 'unknown_state_no_progress_timeout')) if username and filename: used_sources = task.get('used_sources', set()) diff --git a/core/imports/quarantine.py b/core/imports/quarantine.py index 38be1b41..43ed5191 100644 --- a/core/imports/quarantine.py +++ b/core/imports/quarantine.py @@ -93,6 +93,65 @@ def entry_id_from_quarantined_filename(quarantined_filename: str) -> str: return _entry_id_from_filename(os.path.basename(quarantined_filename)) +def get_quarantined_source_keys(quarantine_dir: str) -> set: + """Return a set of ``(username, filename)`` tuples for every Soulseek + source that has been quarantined. + + Used to gate the Soulseek candidate filter against re-picking the + exact same upload that already failed post-download verification. + Issue #652 — without this gate, the auto-wishlist processor's + candidate ranking is deterministic, so the same `(uploader, file)` + keeps winning the quality picker, downloading, quarantining, and + re-queueing in an infinite loop. Users wake up to hundreds of + duplicate `.quarantined` files for the same source URL. + + The keys come from the sidecar JSON's + ``context.original_search_result`` field which `move_to_quarantine` + persists from the originating SearchResult. Sidecars missing either + field (legacy thin sidecars written pre-Feb 2026, or orphaned + files) are skipped silently — they can't gate anything anyway. + + Returns an empty set when the directory doesn't exist or has no + parseable sidecars. Never raises; filesystem / JSON errors are + swallowed at debug level so a corrupt sidecar can't block the + download pipeline. + """ + keys: set = set() + if not quarantine_dir or not os.path.isdir(quarantine_dir): + return keys + + try: + names = os.listdir(quarantine_dir) + except OSError as exc: + logger.debug("get_quarantined_source_keys: listdir failed: %s", exc) + return keys + + for name in names: + if not name.endswith('.json'): + continue + sidecar_path = os.path.join(quarantine_dir, name) + try: + with open(sidecar_path, encoding='utf-8') as f: + sidecar = json.load(f) + except Exception as exc: + logger.debug("get_quarantined_source_keys: sidecar read failed for %s: %s", name, exc) + continue + if not isinstance(sidecar, dict): + continue + ctx = sidecar.get('context') + if not isinstance(ctx, dict): + continue + osr = ctx.get('original_search_result') + if not isinstance(osr, dict): + continue + username = osr.get('username') or '' + filename = osr.get('filename') or '' + if username and filename: + keys.add((str(username), str(filename))) + + return keys + + def list_quarantine_entries(quarantine_dir: str) -> List[Dict[str, Any]]: """Enumerate quarantined files paired with their sidecars. diff --git a/core/library/service_search.py b/core/library/service_search.py index cb8b1874..38fd450e 100644 --- a/core/library/service_search.py +++ b/core/library/service_search.py @@ -100,12 +100,14 @@ def _search_service(service, entity_type, query): if not mb_worker or not mb_worker.mb_service: raise ValueError("MusicBrainz worker not initialized") mb_client = mb_worker.mb_service.mb_client + # User-facing manual search — prefer recall (fuzzy / alias / diacritic- + # folded) over strict phrase precision. User picks correct hit from list. if entity_type == 'artist': - items = mb_client.search_artist(query, limit=8) + items = mb_client.search_artist(query, limit=8, strict=False) return [{'id': a['id'], 'name': a.get('name', ''), 'image': None, 'extra': f"Score: {a.get('score', '')} · {a.get('disambiguation', '') or a.get('country', '')}"} for a in items] elif entity_type == 'album': - items = mb_client.search_release(query, limit=8) + items = mb_client.search_release(query, limit=8, strict=False) results = [] for r in items: artists = ', '.join(ac.get('name', '') for ac in r.get('artist-credit', []) if isinstance(ac, dict)) @@ -115,7 +117,7 @@ def _search_service(service, entity_type, query): 'extra': f"{artists} · {r.get('date', '')} · Score: {r.get('score', '')}"}) return results elif entity_type == 'track': - items = mb_client.search_recording(query, limit=8) + items = mb_client.search_recording(query, limit=8, strict=False) results = [] for r in items: artists = ', '.join(ac.get('name', '') for ac in r.get('artist-credit', []) if isinstance(ac, dict)) diff --git a/core/metadata/release_type.py b/core/metadata/release_type.py new file mode 100644 index 00000000..1f0b6b9b --- /dev/null +++ b/core/metadata/release_type.py @@ -0,0 +1,93 @@ +"""Canonical mapping from raw provider release-type vocabulary to the +internal `album_type` field that drives discography binning + UI. + +Why this exists +--------------- +Three sites historically duplicated the same "best-effort primary-type +→ album_type" mapping, each with a slightly different vocabulary: + + core/musicbrainz_search.py: `_map_release_type` knew about + {album, single, ep, compilation}, defaulted unknown → 'album'. + core/metadata/types.py: inline `{single: single, ep: ep}.get(...)` — + didn't even know about 'compilation', also defaulted → 'album'. + core/metadata/cache.py: Deezer-specific record_type validator — + intentionally narrow, kept here for its provider. + +Issue #650 (S-Bryce) reported that MusicBrainz tags music videos and +some legitimate singles with primary-type=`Other`, which both mappers +silently routed to `album_type='album'`. Combined with the API-level +filter at `musicbrainz_search.search_albums` (which only requested +`type=album|ep|single` from MB and dropped 'Other' entirely), users +with MB-as-primary saw entire release-groups go missing from artist +discography views, and downloaded tracks from those release-groups +appeared as orphan "ghost" tracks bound to no album card. + +Fix shape: one shared mapper consumed by every provider's +`raw → Album dataclass` projection. Knows about 'other' and +'broadcast' (MB's two remaining primary-type vocabulary words) and +maps them to 'single' so they land in the Singles section of the +artist detail page — they're almost always single-track music +releases (music videos, broadcast singles, one-off web releases). +Falling through to 'album' was the original sin — places them in +Albums view where they look misleading and clutter the proper LP +list. +""" + +from __future__ import annotations + +from typing import List, Optional + + +# MB primary-type vocabulary as of 2026 — `Album | Single | EP | +# Broadcast | Other`. Compilation is a *secondary* type; querying MB +# with type=compilation silently breaks (returns ~10% of expected +# results) — see musicbrainz_client.browse_artist_release_groups docs. +_AUDIO_OTHER_PRIMARY_TYPES = frozenset({'other', 'broadcast'}) + + +def map_release_group_type(primary_type: Optional[str], + secondary_types: Optional[List[str]] = None) -> str: + """Project a raw provider release-group primary-type + secondary-types + into the internal `album_type` value the UI binning expects. + + Returns one of: `'album'`, `'single'`, `'ep'`, `'compilation'`. + + Mapping rules: + + - `single` / `ep` pass through unchanged. + - `compilation` (primary or secondary) becomes `'compilation'`. The + compilation secondary-type check is required because MB's + canonical pattern is `primary=Album, secondary=[Compilation]`. + - `other` / `broadcast` become `'single'`. Almost always + single-track music releases (music videos, one-off web drops, + broadcast singles). Placing them in Singles is the pragmatic + bucket — they're not LPs, but excluding them entirely (the + pre-fix behaviour) hid legitimate tracks. + - Anything else (including empty/None) → `'album'`. Matches the + pre-fix default so no existing classifications shift. + + `secondary_types` is optional because the legacy types.py call site + doesn't have access to it from the same level of raw structure. + Pass `None` (or omit) for the secondary-types-unavailable path. + """ + pt = (primary_type or '').strip().lower() + + if pt == 'single': + return 'single' + if pt == 'ep': + return 'ep' + if pt == 'compilation': + return 'compilation' + + # Secondary-type override: MB's compilation albums always carry + # `primary=Album` with `secondary=[Compilation]`, so the primary + # check above can't catch them. + if secondary_types: + normalized = {str(s).strip().lower() for s in secondary_types if s} + if 'compilation' in normalized: + return 'compilation' + + if pt in _AUDIO_OTHER_PRIMARY_TYPES: + return 'single' + + return 'album' diff --git a/core/metadata/types.py b/core/metadata/types.py index 27d5a7c5..b3d5e7f0 100644 --- a/core/metadata/types.py +++ b/core/metadata/types.py @@ -400,10 +400,16 @@ class Album: if raw.get('barcode'): external_ids['barcode'] = _str(raw['barcode']) - # MB `release-group` carries the album-level type (album/single/ep) + # MB `release-group` carries the album-level type (album/single/ep/ + # compilation/other/broadcast). Centralized mapper handles the + # full vocabulary including 'other' / 'broadcast' (issue #650 — + # music videos and one-off releases) so this projection matches + # the search-adapter projection in `core/musicbrainz_search.py`. + from core.metadata.release_type import map_release_group_type rg = raw.get('release-group') or {} - primary_type = _str(rg.get('primary-type'), default='Album').lower() - album_type = {'single': 'single', 'ep': 'ep'}.get(primary_type, 'album') + primary_type = _str(rg.get('primary-type'), default='Album') + secondary_types = rg.get('secondary-types') or [] + album_type = map_release_group_type(primary_type, secondary_types) if rg.get('id'): external_ids['musicbrainz_release_group'] = _str(rg['id']) diff --git a/core/musicbrainz_client.py b/core/musicbrainz_client.py index b152f28f..f1d59491 100644 --- a/core/musicbrainz_client.py +++ b/core/musicbrainz_client.py @@ -128,27 +128,45 @@ class MusicBrainzClient: return [] @rate_limited - def search_release(self, album_name: str, artist_name: Optional[str] = None, limit: int = 10) -> List[Dict[str, Any]]: + def search_release(self, album_name: str, artist_name: Optional[str] = None, + limit: int = 10, strict: bool = True) -> List[Dict[str, Any]]: """ - Search for releases (albums) by name - + Search for releases (albums) by name. + Args: album_name: Name of the album to search for artist_name: Optional artist name to narrow search limit: Maximum number of results to return - + strict: When True (default), builds a phrase-match Lucene query + against the `release` and `artist` fields — correct for + enrichment flows where exact name+artist are known. When + False, sends a bare query (album + artist joined) so MB + hits alias / sortname indexes and folds diacritics, + dramatically improving recall for user-facing fuzzy + lookups (e.g. the manual Fix popup). + Returns: List of release results """ try: - # Escape quotes and backslashes for Lucene query - safe_album = album_name.replace('\\', '\\\\').replace('"', '\\"') - query = f'release:"{safe_album}"' - - if artist_name: - safe_artist = artist_name.replace('\\', '\\\\').replace('"', '\\"') - query += f' AND artist:"{safe_artist}"' - + if strict: + # Escape quotes and backslashes for Lucene query + safe_album = album_name.replace('\\', '\\\\').replace('"', '\\"') + query = f'release:"{safe_album}"' + + if artist_name: + safe_artist = artist_name.replace('\\', '\\\\').replace('"', '\\"') + query += f' AND artist:"{safe_artist}"' + else: + # Bare query — MB tokenizes against title + artist credit + + # alias + sortname indexes together with diacritic folding. + # Recovers cases like "Bjork" → "Björk" that strict phrase + # queries miss. + parts = [album_name] + if artist_name: + parts.append(artist_name) + query = ' '.join(p for p in parts if p) + params = { 'query': query, 'fmt': 'json', @@ -173,27 +191,44 @@ class MusicBrainzClient: return [] @rate_limited - def search_recording(self, track_name: str, artist_name: Optional[str] = None, limit: int = 10) -> List[Dict[str, Any]]: + def search_recording(self, track_name: str, artist_name: Optional[str] = None, + limit: int = 10, strict: bool = True) -> List[Dict[str, Any]]: """ - Search for recordings (tracks) by name - + Search for recordings (tracks) by name. + Args: track_name: Name of the track to search for artist_name: Optional artist name to narrow search limit: Maximum number of results to return - + strict: When True (default), builds a phrase-match Lucene query + against the `recording` and `artist` fields — correct for + enrichment flows where exact name+artist are known. When + False, sends a bare query (track + artist joined) so MB + hits alias / sortname indexes and folds diacritics. The + bare path also avoids the AND-clause that kills recall + when either side mis-matches (e.g. "Bjork" vs canonical + "Björk", or a track title with bracketed suffix like + "(Live)" that strict phrase match rejects). + Returns: List of recording results """ try: - # Escape quotes and backslashes for Lucene query - safe_track = track_name.replace('\\', '\\\\').replace('"', '\\"') - query = f'recording:"{safe_track}"' - - if artist_name: - safe_artist = artist_name.replace('\\', '\\\\').replace('"', '\\"') - query += f' AND artist:"{safe_artist}"' - + if strict: + # Escape quotes and backslashes for Lucene query + safe_track = track_name.replace('\\', '\\\\').replace('"', '\\"') + query = f'recording:"{safe_track}"' + + if artist_name: + safe_artist = artist_name.replace('\\', '\\\\').replace('"', '\\"') + query += f' AND artist:"{safe_artist}"' + else: + # Bare query — see search_release for rationale. + parts = [track_name] + if artist_name: + parts.append(artist_name) + query = ' '.join(p for p in parts if p) + params = { 'query': query, 'fmt': 'json', diff --git a/core/musicbrainz_search.py b/core/musicbrainz_search.py index 21001118..f01292d4 100644 --- a/core/musicbrainz_search.py +++ b/core/musicbrainz_search.py @@ -114,18 +114,12 @@ def _extract_title_hint(query: str, artist_name: str) -> Optional[str]: return None -def _map_release_type(primary_type: str, secondary_types: List[str] = None) -> str: - """Map MusicBrainz release group type to standard album_type.""" - pt = (primary_type or '').lower() - if pt == 'album': - return 'album' - elif pt == 'single': - return 'single' - elif pt == 'ep': - return 'ep' - elif pt == 'compilation' or 'compilation' in (secondary_types or []): - return 'compilation' - return 'album' +# Thin module-level alias retained so callers inside this file keep +# working without touching every call site. The canonical implementation +# (including the 'other' / 'broadcast' handling that fixes issue #650) +# lives in `core/metadata/release_type.py` so every provider's `raw → +# Album` projection shares one mapper. +from core.metadata.release_type import map_release_group_type as _map_release_type class MusicBrainzSearchClient: @@ -365,7 +359,15 @@ class MusicBrainzSearchClient: # the filter silently breaks. Actual compilations # (primary-type=Album with secondary-types=[Compilation]) # are handled by the studio-preference filter below. - release_types=['album', 'ep', 'single'], + # 'other' added per issue #650 — MB tags music videos + # and one-off web/broadcast releases with primary=Other, + # and many artists (Vocaloid producers, indie acts, JP + # solo artists) have legitimate singles classified + # there. Pre-fix this filter dropped them at the API + # layer, hiding tracks the user had downloaded. + # `map_release_group_type` routes 'other' into the + # singles bucket so they appear in the right UI section. + release_types=['album', 'ep', 'single', 'other'], limit=100, ) @@ -638,13 +640,34 @@ class MusicBrainzSearchClient: logger.warning(f"MusicBrainz track search failed: {e}") return [] - def _search_tracks_text(self, track_name: str, artist_name: Optional[str], limit: int) -> List[Track]: - """Fallback text-search path for structured/fuzzy track queries.""" + def _search_tracks_text(self, track_name: str, artist_name: Optional[str], limit: int, + strict: bool = True, min_score: Optional[int] = None) -> List[Track]: + """Fallback text-search path for structured/fuzzy track queries. + + `strict=True` (default) keeps the field-scoped Lucene phrase match — + precise enough for enrichment-style flows where the inputs are + already known-clean. `strict=False` switches to a bare-query + MB lookup that hits alias/sortname indexes with diacritic folding — + needed for user-facing fuzzy surfaces (Fix popup cascade) where + recall beats precision because the user picks from the result list. + Mirrors the same toggle already on `search_recording` in + `core/musicbrainz_client.py`. + + `min_score` defaults to `self._MIN_SCORE` (80) — sized for the + enhanced search tab where unfiltered MB results are noisy. Pass + a lower value (or 0) when a downstream stage like + `core.metadata.relevance.rerank_tracks` will re-sort by artist + match — MB's free-text score heavily favours title-text matches + ("Army of Me (Bjork)" cover by HIRS Collective scores 100, + Björk's canonical "Army of Me" scores 28) so a high floor drops + the right answer. + """ try: - results = self._client.search_recording(track_name, artist_name=artist_name, limit=limit) - # Score filter matches the artist/album logic — cuts garbage - # title collisions from unrelated recordings. - results = [r for r in results if (r.get('score', 0) or 0) >= self._MIN_SCORE] + results = self._client.search_recording( + track_name, artist_name=artist_name, limit=limit, strict=strict + ) + threshold = self._MIN_SCORE if min_score is None else min_score + results = [r for r in results if (r.get('score', 0) or 0) >= threshold] tracks = [] for r in results: @@ -656,6 +679,30 @@ class MusicBrainzSearchClient: logger.warning(f"MusicBrainz track search failed: {e}") return [] + def search_tracks_with_artist(self, track: str, artist: str, + limit: int = 10) -> List[Track]: + """Search MB tracks with track + artist passed as separate fields. + + Powers the Fix-popup metadata cascade (`GET /api/musicbrainz/search_tracks`) + and any future surface where the caller already has the title/artist + split and wants the fuzzy-recall MB lookup without going through + `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). + """ + if not track and not artist: + return [] + 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. @@ -725,6 +772,57 @@ class MusicBrainzSearchClient: logger.error(f'get_track_details({track_id}) error: {e}') return None + def get_recording_flat(self, mbid: str) -> Optional[Dict[str, Any]]: + """Return a Fix-popup-compatible flat track dict by recording MBID. + + Distinct from `get_track_details` which returns a Spotify-shaped + nested dict (artists as objects, album as nested object with + images array). The Discovery Fix popup expects the flat shape that + the spotify/deezer/itunes search endpoints produce — artists as a + list of strings, album as a string, single image_url field. + + Used by `GET /api/musicbrainz/recording/` to support the + MBID-paste lookup field — power-user escape hatch when fuzzy auto- + search ranks the wrong recording among many same-title versions. + + Returns None when the MBID is missing or MB returns no recording. + Recording-without-release is valid (album = '', image_url = ''). + """ + if not mbid: + return None + try: + rec = self._client.get_recording( + mbid, includes=['releases', 'artist-credits', 'release-groups'] + ) + if not rec: + return None + + releases = rec.get('releases', []) or [] + releases.sort(key=self._release_preference_key) + first_rel = releases[0] if releases else {} + rg = first_rel.get('release-group', {}) or {} + release_id = first_rel.get('id', '') + rg_id = rg.get('id', '') + + artists = _extract_artist_credit(rec.get('artist-credit', [])) + album_name = first_rel.get('title', '') or '' + image_url = self._cached_art(release_id, rg_id) if (release_id or rg_id) else None + + return { + 'id': rec.get('id', '') or mbid, + 'name': rec.get('title', '') or '', + 'artists': artists if artists else [], + 'album': album_name, + 'duration_ms': rec.get('length') or 0, + 'image_url': image_url or '', + 'external_urls': { + 'musicbrainz': f'https://musicbrainz.org/recording/{mbid}' + }, + } + except Exception as e: + logger.error(f'get_recording_flat({mbid}) error: {e}') + return None + def get_album_tracks(self, album_mbid: str) -> Optional[Dict[str, Any]]: """Return {items: [...], total: N} track listing for a release/release-group MBID.""" album = self.get_album(album_mbid, include_tracks=True) @@ -856,8 +954,13 @@ class MusicBrainzSearchClient: shape the download modal expects. `rg_fallback` supplies release-group metadata (type, artist credits) when resolving from a release-group whose releases may be lightly populated.""" + # NOTE: `cover-art-archive` is NOT a valid `inc` param for the + # /release resource — MB returns 400 if you pass it. The CAA flags + # (`{'front': True, 'back': True, ...}`) come back on every release + # response by default, so we read them below without requesting an + # include. release = self._client.get_release( - release_mbid, includes=['recordings', 'artist-credits', 'release-groups', 'cover-art-archive'] + release_mbid, includes=['recordings', 'artist-credits', 'release-groups'] ) if not release: return None diff --git a/core/repair_jobs/unknown_artist_fixer.py b/core/repair_jobs/unknown_artist_fixer.py index bf037748..774cf32c 100644 --- a/core/repair_jobs/unknown_artist_fixer.py +++ b/core/repair_jobs/unknown_artist_fixer.py @@ -160,8 +160,18 @@ class UnknownArtistFixerJob(RepairJob): # Compute expected file path expected_rel = None if reorganize_files and corrected.get('artist') and corrected.get('album'): - from core.repair_jobs.library_reorganize import _build_path_from_template, _get_audio_quality - quality = _get_audio_quality(resolved) + # Issue #646: `core.repair_jobs.library_reorganize` was + # rewritten in commit ca5c9316 ("Rewrite Library + # Reorganize job to delegate to per-album planner") and + # the private `_build_path_from_template` / + # `_get_audio_quality` helpers moved out. They live in + # the import pipeline now — same shape, different + # module. Without this re-wire the Unknown Artist Fixer + # crashes on first scan with `ImportError: cannot + # import name '_build_path_from_template'`. + from core.imports.paths import get_file_path_from_template_raw + from core.imports.file_ops import get_audio_quality_string + quality = get_audio_quality_string(resolved) tmpl_ctx = { 'artist': corrected['artist'], 'albumartist': corrected['artist'], @@ -173,7 +183,7 @@ class UnknownArtistFixerJob(RepairJob): 'quality': quality, 'albumtype': 'Album', } - folder, fname_base = _build_path_from_template(album_template, tmpl_ctx) + folder, fname_base = get_file_path_from_template_raw(album_template, tmpl_ctx) file_ext = os.path.splitext(resolved)[1] if quality and f'[{quality}]' not in fname_base: fname_base = f"{fname_base} [{quality}]" diff --git a/core/soulseek_client.py b/core/soulseek_client.py index 6b928688..71b20345 100644 --- a/core/soulseek_client.py +++ b/core/soulseek_client.py @@ -250,6 +250,7 @@ class SoulseekClient(DownloadSourcePlugin): if response.status in [200, 201, 204]: # Accept 200 OK, 201 Created, and 204 No Content self._last_401_logged = False # Reset on success + self._last_unreachable_logged = False # Same reset for unreachable-host suppression try: if response_text.strip(): # Only parse if there's content return await response.json() @@ -291,6 +292,25 @@ class SoulseekClient(DownloadSourcePlugin): f"{method} {url} — slskd may be overloaded or unreachable" ) return None + except aiohttp.ClientConnectorError as e: + # Issue #649: slskd_url is configured but the host is unreachable + # (slskd not running, wrong port, DNS / Docker bridge issue). + # Status polling at /api/downloads/status fans out to every plugin + # including soulseek even when the user has soulseek toggled out + # of their active download sources, so each frontend poll + # produced an ERROR log line — visible spam during any + # non-soulseek download. Suppress repeats to debug; emit one + # WARNING with actionable context, then reset on any successful + # response (slskd came back up). + if not getattr(self, '_last_unreachable_logged', False): + logger.warning( + f"slskd unreachable at {self.base_url}: {e}. " + f"Either start slskd or clear `soulseek.slskd_url` in settings " + f"if you don't use Soulseek. Suppressing further connection errors." + ) + self._last_unreachable_logged = True + logger.debug(f"slskd connection failed: {method} {url}: {e}") + return None except Exception as e: logger.error(f"Error making API request: {e}") return None @@ -348,6 +368,19 @@ class SoulseekClient(DownloadSourcePlugin): f"{method} {url} — slskd may be overloaded or unreachable" ) return None + except aiohttp.ClientConnectorError as e: + # Issue #649 — same suppression as _make_request. Direct + # request is a less common path but uses the same base_url, + # so the same unreachable-host condition fires here. + if not getattr(self, '_last_unreachable_logged', False): + logger.warning( + f"slskd unreachable at {self.base_url}: {e}. " + f"Either start slskd or clear `soulseek.slskd_url` in settings " + f"if you don't use Soulseek. Suppressing further connection errors." + ) + self._last_unreachable_logged = True + logger.debug(f"slskd direct connection failed: {method} {url}: {e}") + return None except Exception as e: logger.error(f"Error making direct API request: {e}") return None @@ -1464,14 +1497,71 @@ class SoulseekClient(DownloadSourcePlugin): 'other': (0, 500), } + def _drop_quarantined_sources(self, results: List[TrackResult]) -> List[TrackResult]: + """Filter out candidates whose `(username, filename)` is on the + quarantine record. Issue #652. + + Reads quarantine sidecars fresh each call so newly-quarantined + sources are honored immediately on the next search — no client + state to invalidate. Filesystem cost is bounded (one listdir + + N small JSON reads) and dwarfed by the Soulseek search itself. + + Returns the input list unchanged when the quarantine directory + is absent, empty, or unreadable — i.e. defaults to today's + behaviour if anything goes wrong on the dedup path. + """ + try: + from core.imports.quarantine import get_quarantined_source_keys + download_path = config_manager.get('soulseek.download_path', './downloads') + quarantine_dir = os.path.join(download_path, 'ss_quarantine') + blocked = get_quarantined_source_keys(quarantine_dir) + except Exception as exc: + logger.debug("quarantine dedup: failed to load source keys, skipping filter: %s", exc) + return results + + if not blocked: + return results + + kept: List[TrackResult] = [] + skipped = 0 + for candidate in results: + key = (candidate.username or '', candidate.filename or '') + if key in blocked: + skipped += 1 + continue + kept.append(candidate) + + if skipped: + logger.info( + f"Quarantine dedup: dropped {skipped} candidate(s) matching previously-quarantined sources; " + f"{len(kept)} remain" + ) + return kept + def filter_results_by_quality_preference(self, results: List[TrackResult]) -> List[TrackResult]: """ Filter candidates based on user's quality profile with bitrate density constraints. Uses priority waterfall logic: tries highest priority quality first, falls back to lower priorities. Returns candidates matching quality profile constraints, sorted by confidence and effective bitrate. + + Issue #652: also drops candidates whose `(username, filename)` + matches a previously-quarantined download. Without this pre-filter + the auto-wishlist processor's ranking is deterministic — the same + `(uploader, file)` keeps winning the quality picker, downloading, + failing AcoustID, quarantining, and re-queueing in an infinite + loop. Users wake up to hundreds of duplicate `.quarantined` files + for the same source URL. """ from database.music_database import MusicDatabase + if not results: + return [] + + # Drop sources already quarantined — bypass the quality picker + # entirely so the same bad upload doesn't get re-selected on the + # next wishlist cycle. Filesystem read is bounded (~few hundred + # sidecars in practical use × <1ms each). + results = self._drop_quarantined_sources(results) if not results: return [] diff --git a/entrypoint.sh b/entrypoint.sh index 7d830525..8cde8e0f 100644 --- a/entrypoint.sh +++ b/entrypoint.sh @@ -40,7 +40,7 @@ if [ "$CURRENT_UID" != "$PUID" ] || [ "$CURRENT_GID" != "$PGID" ]; then DATA_OWNER=$(stat -c '%u:%g' /app/data 2>/dev/null || echo "unknown") if [ "$DATA_OWNER" != "$PUID:$PGID" ]; then echo "🔒 Fixing permissions on app directories..." - chown -R soulsync:soulsync /app/config /app/data /app/logs /app/downloads /app/Transfer /app/Staging 2>/dev/null || true + chown -R soulsync:soulsync /app/config /app/data /app/logs /app/downloads /app/Transfer /app/Staging /app/Stream 2>/dev/null || true else echo "✅ App directory permissions already correct" fi @@ -81,15 +81,15 @@ chown soulsync:soulsync /app/config/settings.py 2>/dev/null || true # Pre-mid-2026 the chown line had `|| true` but mkdir didn't — combined # with `set -e`, a permission-denied mkdir crashed the container into a # restart loop. Both lines are now best-effort. -mkdir -p /app/config /app/data /app/logs /app/downloads /app/Transfer /app/Staging 2>/dev/null || true -chown soulsync:soulsync /app/config /app/data /app/logs /app/downloads /app/Transfer /app/Staging 2>/dev/null || true +mkdir -p /app/config /app/data /app/logs /app/downloads /app/Transfer /app/Staging /app/Stream 2>/dev/null || true +chown soulsync:soulsync /app/config /app/data /app/logs /app/downloads /app/Transfer /app/Staging /app/Stream 2>/dev/null || true # Writability audit — surface a loud warning if any bind-mounted dir # isn't writable by the soulsync user. The restart-loop fix above makes # the container start regardless, but a non-writable Staging / downloads # / Transfer will fail silently inside the app (auto-import quarantine, # download writes). Better to log now than to debug missing files later. -for dir in /app/config /app/data /app/logs /app/downloads /app/Transfer /app/Staging /app/MusicVideos /app/scripts; do +for dir in /app/config /app/data /app/logs /app/downloads /app/Transfer /app/Staging /app/Stream /app/MusicVideos /app/scripts; do if [ -d "$dir" ] && ! gosu soulsync test -w "$dir" 2>/dev/null; then echo "⚠️ WARNING: $dir is not writable by soulsync (uid $(id -u soulsync))." echo " Host bind-mount perms likely mismatch the PUID/PGID env vars." diff --git a/pr_description.md b/pr_description.md deleted file mode 100644 index 510794f5..00000000 --- a/pr_description.md +++ /dev/null @@ -1,57 +0,0 @@ -## Summary - -Adds source-aware artist detail deep links so artist pages can be opened directly as `/artist-detail/:source/:id`, including metadata-source artists from Spotify, Deezer, iTunes, Discogs, Amazon, Hydrabase, and existing library artists. - -This also fixes Discover/download modal artist links that were falling back to `/artist-detail/library/` or using the wrong album/card ID as an artist ID. - -## What Changed - -- Added canonical artist detail routes in the SPA: - - `/artist-detail/library/` - - `/artist-detail/spotify/` - - `/artist-detail/deezer/` - - `/artist-detail/itunes/` - - plus other supported metadata sources. -- Preserved legacy `/artist-detail/` behavior as a library fallback. -- Updated shell routing and deep-link activation so refresh/direct navigation works for nested artist-detail URLs. -- Updated artist detail navigation to carry `artistSource` through the SPA instead of relying only on artist name/id. -- Improved source-only artist detail loading so provider-fetched artist names are used when the URL only contains the source ID. -- Prevented source-only artist pages from running library-only ownership/enhancement checks. -- Treats unknown ownership on source-only discographies as missing/clickable instead of leaving cards stuck on "still checking ownership." -- Uses release artwork as a generic artist-detail hero fallback when an artist portrait is missing or fails to load. -- Preserved source/artist IDs from Discover album modals, seasonal albums, cached discovery albums, recent releases, and download modal hero links. -- Prevented modal artist links from falling back to fake library routes when a real source artist ID is unavailable. -- Returned seasonal album `source` from cached seasonal album rows so seasonal modal links retain their provider context. - -## Behavior - -- Clicking a Spotify artist result can now land on: - - `/artist-detail/spotify/2YZyLoL8N0Wb9xBt1NhZWg` -- Clicking a Deezer artist result can now land on: - - `/artist-detail/deezer/525046` -- Existing library artist links continue to resolve through the library path. -- If a source artist resolves to an existing library artist, the page upgrades to the library-backed artist and keeps library-only tools/checks available. -- If a source artist is not in the library, the page shows source discography as missing/clickable and skips library-only endpoints. -- If a modal lacks a trustworthy source artist ID, it shows a warning instead of navigating to an invalid library artist URL. - -## Tests - -Frontend route tests: - -```bash -cd webui -npm.cmd test -- --run src/platform/shell/route-manifest.test.ts src/platform/shell/bridge.test.ts -``` - -Result: - -```text -2 test files passed -10 tests passed -``` - -Recommended backend verification: - -```bash -./.venv/bin/python -m pytest tests/test_spa_deep_linking.py tests/metadata/test_artist_source_detail.py -``` diff --git a/tests/downloads/test_soulseek_pinning.py b/tests/downloads/test_soulseek_pinning.py index 253be224..0c7a8202 100644 --- a/tests/downloads/test_soulseek_pinning.py +++ b/tests/downloads/test_soulseek_pinning.py @@ -347,3 +347,336 @@ def test_make_direct_request_returns_none_on_timeout(configured_client): result = _run_async(configured_client._make_direct_request('GET', 'health')) assert result is None + + +# --------------------------------------------------------------------------- +# Issue #649 — connection-error log spam suppression +# --------------------------------------------------------------------------- + + +def _build_unreachable_session(error_message: str = 'Cannot connect to host'): + """Stub aiohttp session whose request() raises ClientConnectorError.""" + import aiohttp + from unittest.mock import MagicMock + + class _Cm: + async def __aenter__(self_inner): + # ClientConnectorError needs a connection_key + OSError. The + # exact values don't matter for the test — we just need an + # instance of the right class so the except-branch fires. + os_err = OSError(-2, 'Name or service not known') + raise aiohttp.ClientConnectorError(MagicMock(), os_err) + async def __aexit__(self_inner, *args): + return None + + class _StubSession: + async def __aenter__(self): + return self + async def __aexit__(self, *args): + return None + def request(self, *args, **kwargs): + return _Cm() + async def close(self): + return None + + return _StubSession + + +def test_unreachable_slskd_returns_none_not_raises(configured_client): + """Pin: ClientConnectorError must not propagate. Caller treats None + as a normal failure (same as a 5xx) — every consumer that gates on + `if response is None` keeps working when slskd is unreachable.""" + StubSession = _build_unreachable_session() + with patch('aiohttp.ClientSession', return_value=StubSession()): + result = _run_async(configured_client._make_request('GET', 'transfers/downloads')) + assert result is None + + +def test_unreachable_slskd_logs_warning_once_then_debug(configured_client, caplog): + """Issue #649: status polling at /api/downloads/status fans out to + every plugin including soulseek even when the user has soulseek + toggled out, so each frontend poll produced an ERROR log line. Pin + that the FIRST unreachable response emits one WARNING with + actionable context, and subsequent repeats demote to DEBUG so the + log isn't spammed for the lifetime of every non-soulseek download.""" + import logging + configured_client._last_unreachable_logged = False + StubSession = _build_unreachable_session() + + with patch('aiohttp.ClientSession', return_value=StubSession()): + with caplog.at_level(logging.DEBUG, logger='soulseek_client'): + # Three repeated polls — first must warn, rest must stay quiet. + _run_async(configured_client._make_request('GET', 'transfers/downloads')) + _run_async(configured_client._make_request('GET', 'transfers/downloads')) + _run_async(configured_client._make_request('GET', 'transfers/downloads')) + + warning_records = [r for r in caplog.records if r.levelno == logging.WARNING + and 'slskd unreachable' in r.message] + error_records = [r for r in caplog.records if r.levelno == logging.ERROR + and 'Error making API request' in r.message] + assert len(warning_records) == 1, \ + f"Expected exactly 1 WARNING (one-time slskd-unreachable notice), got {len(warning_records)}" + assert len(error_records) == 0, \ + "Connection errors must not log at ERROR — that's the spam pattern #649 reported" + assert configured_client._last_unreachable_logged is True + + +def test_unreachable_flag_resets_on_successful_response(configured_client, caplog): + """When slskd comes back up after a stretch of being down, a fresh + WARNING should fire if it goes down again later — the suppression is + per-outage, not per-process-lifetime. The flag resets on any + successful (200/201/204) response.""" + import logging + configured_client._last_unreachable_logged = True # Simulate prior outage already warned + + # Simulate a 200 response — must reset the suppression flag. + class _OkCm: + async def __aenter__(self_inner): + class _Resp: + status = 200 + reason = 'OK' + async def text(self_resp): + return '{"ok": true}' + async def json(self_resp): + return {'ok': True} + return _Resp() + async def __aexit__(self_inner, *args): + return None + + class _OkSession: + async def __aenter__(self): + return self + async def __aexit__(self, *args): + return None + def request(self, *args, **kwargs): + return _OkCm() + async def close(self): + return None + + with patch('aiohttp.ClientSession', return_value=_OkSession()): + _run_async(configured_client._make_request('GET', 'server/state')) + + assert configured_client._last_unreachable_logged is False, \ + "Successful response must reset the suppression flag so a future outage warns again" + + +def test_make_direct_request_also_suppresses_unreachable_spam(configured_client, caplog): + """`_make_direct_request` shares the same base_url and same outage + mode, so it gets the same WARNING-once + DEBUG-after treatment.""" + import logging + configured_client._last_unreachable_logged = False + StubSession = _build_unreachable_session() + + with patch('aiohttp.ClientSession', return_value=StubSession()): + with caplog.at_level(logging.DEBUG, logger='soulseek_client'): + _run_async(configured_client._make_direct_request('GET', 'health')) + _run_async(configured_client._make_direct_request('GET', 'health')) + + warning_records = [r for r in caplog.records if r.levelno == logging.WARNING + and 'slskd unreachable' in r.message] + error_records = [r for r in caplog.records if r.levelno == logging.ERROR + and 'Error making direct API request' in r.message] + assert len(warning_records) == 1 + assert len(error_records) == 0 + + +def test_non_connection_exception_still_logs_error(configured_client, caplog): + """Guard: only ClientConnectorError gets the suppression treatment. + Any other exception (programming bug, unexpected aiohttp behaviour, + etc.) must still surface at ERROR so we don't accidentally hide + real problems behind the noise reduction.""" + import logging + + class _BoomCm: + async def __aenter__(self_inner): + raise ValueError("not a connection error — should still log ERROR") + async def __aexit__(self_inner, *args): + return None + + class _BoomSession: + async def __aenter__(self): + return self + async def __aexit__(self, *args): + return None + def request(self, *args, **kwargs): + return _BoomCm() + async def close(self): + return None + + with patch('aiohttp.ClientSession', return_value=_BoomSession()): + with caplog.at_level(logging.DEBUG, logger='soulseek_client'): + result = _run_async(configured_client._make_request('GET', 'transfers/downloads')) + + assert result is None # Still returns None — non-raising contract preserved + error_records = [r for r in caplog.records if r.levelno == logging.ERROR + and 'Error making API request' in r.message] + assert len(error_records) == 1, "Non-connection exceptions must still log ERROR" + + +# --------------------------------------------------------------------------- +# Issue #652 — quarantined-source dedup in the candidate filter +# --------------------------------------------------------------------------- + + +def _mk_track_result(username='peer', filename='song.flac', quality='flac', + bitrate=1411, size=10_000_000, duration=180_000): + """Build a minimal TrackResult for the candidate filter tests.""" + from core.download_plugins.types import TrackResult + return TrackResult( + username=username, + filename=filename, + size=size, + bitrate=bitrate, + duration=duration, + quality=quality, + free_upload_slots=1, + upload_speed=1_000_000, + queue_length=0, + ) + + +def test_drop_quarantined_sources_keeps_clean_candidates(configured_client, tmp_path, monkeypatch): + """When no candidate matches a quarantined `(username, filename)`, + every result passes through. Filter is a no-op for clean searches.""" + quarantine_dir = tmp_path / 'ss_quarantine' + quarantine_dir.mkdir() + + # Patch config_manager to point at our temp download path. + import core.soulseek_client as sc + monkeypatch.setattr(sc.config_manager, 'get', + lambda key, default=None: str(tmp_path) if key == 'soulseek.download_path' else default) + + results = [ + _mk_track_result(username='goodpeer1', filename='a.flac'), + _mk_track_result(username='goodpeer2', filename='b.flac'), + ] + + kept = configured_client._drop_quarantined_sources(results) + + assert len(kept) == 2 + assert {r.username for r in kept} == {'goodpeer1', 'goodpeer2'} + + +def test_drop_quarantined_sources_drops_known_bad(configured_client, tmp_path, monkeypatch): + """Issue #652 core contract: a candidate whose `(username, filename)` + matches a quarantined entry is dropped before the quality picker + ranks it. Stops the loop where the same source kept winning the + quality picker and re-downloading itself.""" + import json as _json + quarantine_dir = tmp_path / 'ss_quarantine' + quarantine_dir.mkdir() + + # Write a sidecar matching the bad source. + sidecar = { + "original_filename": "bad.flac", + "quarantine_reason": "AcoustID mismatch", + "context": { + "original_search_result": { + "username": "badpeer", "filename": "albums/bad.flac", + }, + }, + } + (quarantine_dir / "20260518_120000.json").write_text(_json.dumps(sidecar)) + + import core.soulseek_client as sc + monkeypatch.setattr(sc.config_manager, 'get', + lambda key, default=None: str(tmp_path) if key == 'soulseek.download_path' else default) + + results = [ + _mk_track_result(username='badpeer', filename='albums/bad.flac'), + _mk_track_result(username='goodpeer', filename='albums/good.flac'), + ] + + kept = configured_client._drop_quarantined_sources(results) + + assert len(kept) == 1 + assert kept[0].username == 'goodpeer' + + +def test_drop_quarantined_sources_returns_input_when_quarantine_missing(configured_client, tmp_path, monkeypatch): + """No quarantine directory yet (fresh install / never used) — + helper returns an empty set; filter returns the input unchanged. + Defaults to today's behaviour for users with no quarantine history.""" + import core.soulseek_client as sc + monkeypatch.setattr(sc.config_manager, 'get', + lambda key, default=None: str(tmp_path) if key == 'soulseek.download_path' else default) + + results = [_mk_track_result(username='peer', filename='song.flac')] + + kept = configured_client._drop_quarantined_sources(results) + + assert kept == results + + +def test_drop_quarantined_sources_swallows_filesystem_errors(configured_client, monkeypatch): + """If something goes wrong loading the quarantine keys (permissions, + OS quirk, etc.), the filter must NOT break the download pipeline. + Returns input unchanged so legitimate downloads keep working — + same defensive contract as the existing 401/connection handlers.""" + import core.soulseek_client as sc + + def _broken_get(key, default=None): + raise RuntimeError("config explosion") + + monkeypatch.setattr(sc.config_manager, 'get', _broken_get) + + results = [_mk_track_result(username='peer', filename='song.flac')] + + kept = configured_client._drop_quarantined_sources(results) + + assert kept == results + + +def test_filter_results_by_quality_runs_quarantine_dedup_first(configured_client, tmp_path, monkeypatch): + """Integration pin: `filter_results_by_quality_preference` calls + the quarantine dedup BEFORE the quality picker. If a candidate is + on the quarantine record, it can't win the picker by virtue of + superior bitrate — that's how the #652 loop manifested.""" + import json as _json + quarantine_dir = tmp_path / 'ss_quarantine' + quarantine_dir.mkdir() + + sidecar = { + "context": { + "original_search_result": { + "username": "badpeer", "filename": "high_bitrate_bad.flac", + }, + }, + } + (quarantine_dir / "20260518_120000.json").write_text(_json.dumps(sidecar)) + + import core.soulseek_client as sc + monkeypatch.setattr(sc.config_manager, 'get', + lambda key, default=None: str(tmp_path) if key == 'soulseek.download_path' else default) + + # Mock the DB call inside filter_results_by_quality_preference so the + # test doesn't need a real DB. Quality profile permits FLAC. + class _FakeDB: + def get_quality_profile(self): + return { + 'preset': 'flac', + 'qualities': { + 'flac': {'enabled': True, 'min_kbps': 800, 'max_kbps': 99999}, + 'mp3_320': {'enabled': False, 'min_kbps': 0, 'max_kbps': 0}, + 'mp3_256': {'enabled': False, 'min_kbps': 0, 'max_kbps': 0}, + 'mp3_192': {'enabled': False, 'min_kbps': 0, 'max_kbps': 0}, + }, + 'priority': ['flac'], + } + + import database.music_database as md + monkeypatch.setattr(md, 'MusicDatabase', lambda: _FakeDB()) + + results = [ + # The "bad" source has the BEST quality on paper — pre-fix would win the picker. + _mk_track_result(username='badpeer', filename='high_bitrate_bad.flac', + quality='flac', bitrate=1411, size=20_000_000, duration=180_000), + _mk_track_result(username='goodpeer', filename='good.flac', + quality='flac', bitrate=1411, size=20_000_000, duration=180_000), + ] + + kept = configured_client.filter_results_by_quality_preference(results) + + usernames = {r.username for r in kept} + assert 'badpeer' not in usernames, "Quarantined source must be filtered before the quality picker" + assert 'goodpeer' in usernames diff --git a/tests/imports/test_quarantine_management.py b/tests/imports/test_quarantine_management.py index 97aebe04..d2c527c0 100644 --- a/tests/imports/test_quarantine_management.py +++ b/tests/imports/test_quarantine_management.py @@ -5,6 +5,7 @@ from core.imports.quarantine import ( approve_quarantine_entry, delete_quarantine_entry, entry_id_from_quarantined_filename, + get_quarantined_source_keys, list_quarantine_entries, recover_to_staging, serialize_quarantine_context, @@ -285,3 +286,115 @@ def test_recover_removes_sidecar_after_move(tmp_path): recover_to_staging(str(quarantine), str(staging), "20260514_120000_song") assert not sidecar.exists() + + +# ────────────────────────────────────────────────────────────────────── +# get_quarantined_source_keys — issue #652 dedup primitive +# ────────────────────────────────────────────────────────────────────── + + +def _write_quarantine_sidecar_with_source(quarantine_dir, entry_id, *, + username=None, filename=None): + """Helper that writes a sidecar matching the shape `move_to_quarantine` + produces — `context.original_search_result.{username, filename}` is + the path `get_quarantined_source_keys` pulls from.""" + sidecar = { + "original_filename": "song.flac", + "quarantine_reason": "boom", + "timestamp": "2026-05-14T12:00:00", + "trigger": "acoustid", + } + if username is not None or filename is not None: + sidecar["context"] = { + "original_search_result": { + "username": username or "", + "filename": filename or "", + } + } + path = quarantine_dir / f"{entry_id}.json" + path.write_text(json.dumps(sidecar)) + return path + + +def test_source_keys_empty_for_missing_dir(tmp_path): + """Defensive: caller may pass a path that doesn't exist (config not + initialised, quarantine never used). Don't crash, just return an + empty set — Soulseek filter then keeps every candidate.""" + assert get_quarantined_source_keys(str(tmp_path / "nope")) == set() + + +def test_source_keys_empty_for_empty_dir(tmp_path): + """Empty quarantine dir → empty set.""" + assert get_quarantined_source_keys(str(tmp_path)) == set() + + +def test_source_keys_collects_username_filename_tuples(tmp_path): + """Sidecars with `context.original_search_result.username` and + `.filename` round-trip into `(username, filename)` tuples — that's + the exact shape the Soulseek candidate filter looks up against.""" + _write_quarantine_sidecar_with_source( + tmp_path, "20260514_120000_a", + username="badpeer", filename="path/to/bad.flac", + ) + _write_quarantine_sidecar_with_source( + tmp_path, "20260514_120100_b", + username="otherpeer", filename="other.mp3", + ) + + keys = get_quarantined_source_keys(str(tmp_path)) + + assert ("badpeer", "path/to/bad.flac") in keys + assert ("otherpeer", "other.mp3") in keys + assert len(keys) == 2 + + +def test_source_keys_skip_legacy_sidecars_without_context(tmp_path): + """Sidecars written pre-Feb 2026 don't have the `context` field — + can't gate against them since the originating source is unknown. + Must skip silently rather than crashing the dedup path.""" + _write_quarantine_sidecar_with_source(tmp_path, "legacy_id") # no username/filename + + assert get_quarantined_source_keys(str(tmp_path)) == set() + + +def test_source_keys_skip_sidecars_with_empty_source_fields(tmp_path): + """Defensive: a sidecar with an empty string for username OR filename + can't gate anything meaningfully — dropping every result whose + username equals '' would catch unrelated downloads. Skip those + entries entirely.""" + _write_quarantine_sidecar_with_source(tmp_path, "empty_user", username="", filename="x.flac") + _write_quarantine_sidecar_with_source(tmp_path, "empty_file", username="u", filename="") + + assert get_quarantined_source_keys(str(tmp_path)) == set() + + +def test_source_keys_skip_corrupt_sidecars(tmp_path): + """A corrupt JSON sidecar (truncated write, encoding glitch) must + not propagate up and break the dedup path. Filesystem read errors + are swallowed at debug level.""" + bad = tmp_path / "corrupt.json" + bad.write_text("{not valid json") + _write_quarantine_sidecar_with_source( + tmp_path, "good", username="good_peer", filename="good.flac", + ) + + keys = get_quarantined_source_keys(str(tmp_path)) + + assert keys == {("good_peer", "good.flac")} + + +def test_source_keys_dedup_repeated_sources(tmp_path): + """If the SAME `(username, filename)` was quarantined twice (which + is exactly the #652 bug — but until now wasn't being prevented), + the set collapses to one entry. The Soulseek filter still acts as + a single-membership check, so a single set entry is enough.""" + _write_quarantine_sidecar_with_source( + tmp_path, "first", username="peer", filename="dupe.flac", + ) + _write_quarantine_sidecar_with_source( + tmp_path, "second", username="peer", filename="dupe.flac", + ) + + keys = get_quarantined_source_keys(str(tmp_path)) + + assert keys == {("peer", "dupe.flac")} diff --git a/tests/metadata/test_musicbrainz_search.py b/tests/metadata/test_musicbrainz_search.py index 4269a5f8..cec77648 100644 --- a/tests/metadata/test_musicbrainz_search.py +++ b/tests/metadata/test_musicbrainz_search.py @@ -541,7 +541,7 @@ def test_get_album_resolves_release_group_mbid_to_release(): 'rg-damn', includes=['releases', 'artist-credits'] ) client._client.get_release.assert_called_once_with( - 'rel-official', includes=['recordings', 'artist-credits', 'release-groups', 'cover-art-archive'] + 'rel-official', includes=['recordings', 'artist-credits', 'release-groups'] ) assert album is not None assert album['id'] == 'rg-damn' # Canonical ID stays the release-group MBID. @@ -680,6 +680,61 @@ def test_search_albums_bare_artist_no_hint_no_filter(): assert 'Revolver' in titles +# --------------------------------------------------------------------------- +# Issue #650 — 'Other' primary-type release-groups must surface +# --------------------------------------------------------------------------- + + +def test_search_albums_browse_filter_requests_other_primary_type(): + """Issue #650: pre-fix the MB browse filter requested only + `album|ep|single`, dropping every primary-type=`Other` release-group + at the API layer. For artists like Vocaloid producers and JP indie + acts whose music videos / one-off web releases are tagged Other, + that hid legitimate tracks. Pin that the filter now includes + 'other' so those release-groups round-trip into the discography.""" + client = MusicBrainzSearchClient() + client._client = MagicMock() + client._client.search_artist.return_value = [_mk_artist('Inabakumori', 'mb-i', score=100)] + client._client.browse_artist_release_groups.return_value = [] + + client.search_albums('inabakumori', limit=10) + + # Inspect the actual call args — the API filter is the lever that + # decides whether MB returns Other-typed groups at all. + args, kwargs = client._client.browse_artist_release_groups.call_args + requested_types = kwargs.get('release_types') or (args[1] if len(args) > 1 else None) + assert requested_types is not None, \ + "browse_artist_release_groups must receive an explicit release_types filter" + assert 'other' in requested_types, \ + f"'other' must be in the requested types so #650 Other-typed releases surface; got {requested_types}" + + +def test_search_albums_other_type_release_groups_appear_as_singles(): + """When MB returns an Other-typed release-group (music video, + one-off web release), it must arrive in the discography as an + Album dataclass with album_type='single' — so the downstream + binner in `core/metadata/discography.py` routes it to the Singles + section rather than burying it among LPs.""" + client = MusicBrainzSearchClient() + client._client = MagicMock() + client._client.search_artist.return_value = [_mk_artist('Inabakumori', 'mb-i', score=100)] + client._client.browse_artist_release_groups.return_value = [ + {'id': 'rg-mv', 'title': 'ロストアンブレラ', 'primary-type': 'Other', + 'first-release-date': '2018-02-27', 'secondary-types': []}, + {'id': 'rg-single', 'title': 'ラグトレイン', 'primary-type': 'Single', + 'first-release-date': '2020-01-01', 'secondary-types': []}, + ] + + albums = client.search_albums('inabakumori', limit=10) + + by_id = {a.id: a for a in albums} + assert 'rg-mv' in by_id, "Other-typed release-group must survive the filter and arrive in the result" + assert by_id['rg-mv'].album_type == 'single', \ + "Other-typed release-group must map to album_type='single' so it lands in the Singles section" + # Pre-existing single behaviour unchanged. + assert by_id['rg-single'].album_type == 'single' + + def test_recording_to_track_total_tracks_matches_media_count(): """Regression: total_tracks was initialized at 1 and summed with media track-counts, producing an off-by-one. An 11-track album reported 12.""" @@ -765,3 +820,257 @@ def test_search_tracks_text_path_filters_by_score(): titles = [t.name for t in tracks] assert 'Good' in titles assert 'Bad' not in titles + + +# --------------------------------------------------------------------------- +# get_recording_flat — Fix-popup MBID paste adapter +# --------------------------------------------------------------------------- + +def test_get_recording_flat_happy_path(): + """Recording with a release returns flat shape with album + image.""" + client = MusicBrainzSearchClient() + client._client = MagicMock() + client._client.get_recording.return_value = { + 'id': 'rec-abc', + 'title': 'Army of Me', + 'length': 234000, + 'artist-credit': [{'artist': {'name': 'Björk'}}], + 'releases': [{ + 'id': 'rel-xyz', + 'title': 'Post', + 'date': '1995-06-13', + 'status': 'Official', + 'media': [{'track-count': 11}], + 'release-group': {'id': 'rg-post', 'primary-type': 'Album', 'secondary-types': []}, + }], + } + + track = client.get_recording_flat('rec-abc') + + assert track is not None + assert track['id'] == 'rec-abc' + assert track['name'] == 'Army of Me' + assert track['artists'] == ['Björk'] # flat list of strings, not Spotify-shaped objects + assert track['album'] == 'Post' # flat string, not nested dict + assert track['duration_ms'] == 234000 + assert track['image_url'] # CAA URL present + assert 'musicbrainz.org/recording/rec-abc' in track['external_urls']['musicbrainz'] + + +def test_get_recording_flat_missing_mbid_returns_none(): + """No MBID → no API call, returns None.""" + client = MusicBrainzSearchClient() + client._client = MagicMock() + + assert client.get_recording_flat('') is None + assert client.get_recording_flat(None) is None + client._client.get_recording.assert_not_called() + + +def test_get_recording_flat_mb_returns_no_recording(): + """MB returns None (404 / missing) → adapter returns None.""" + client = MusicBrainzSearchClient() + client._client = MagicMock() + client._client.get_recording.return_value = None + + assert client.get_recording_flat('rec-missing') is None + + +def test_get_recording_flat_recording_without_release(): + """Standalone recording (no releases) — album stays empty, + image_url empty, but the rest of the shape is intact.""" + client = MusicBrainzSearchClient() + client._client = MagicMock() + client._client.get_recording.return_value = { + 'id': 'rec-standalone', + 'title': 'Untitled Demo', + 'length': 120000, + 'artist-credit': [{'artist': {'name': 'Unknown'}}], + 'releases': [], + } + + track = client.get_recording_flat('rec-standalone') + + assert track is not None + assert track['name'] == 'Untitled Demo' + assert track['album'] == '' + assert track['image_url'] == '' + assert track['artists'] == ['Unknown'] + assert track['duration_ms'] == 120000 + + +def test_get_recording_flat_multi_artist_credit(): + """Recording with multiple credited artists — all flatten to list of strings.""" + client = MusicBrainzSearchClient() + client._client = MagicMock() + client._client.get_recording.return_value = { + 'id': 'rec-collab', + 'title': 'Collab Track', + 'length': 180000, + 'artist-credit': [ + {'artist': {'name': 'Artist A'}}, + {'artist': {'name': 'Artist B'}}, + ], + 'releases': [], + } + + track = client.get_recording_flat('rec-collab') + + assert track['artists'] == ['Artist A', 'Artist B'] + + +def test_get_recording_flat_includes_match_get_track_details(): + """Sanity: passes the same includes list so the API call is cacheable + against the same key as get_track_details (one network request can + serve both surfaces if MB ever adds response caching upstream).""" + client = MusicBrainzSearchClient() + client._client = MagicMock() + client._client.get_recording.return_value = None + + client.get_recording_flat('rec-x') + + client._client.get_recording.assert_called_once_with( + 'rec-x', includes=['releases', 'artist-credits', 'release-groups'] + ) + + +def test_get_recording_flat_swallows_client_errors(): + """MB client raising must not propagate to the route handler — return + None so the endpoint can render a friendly 404 instead of 500.""" + client = MusicBrainzSearchClient() + client._client = MagicMock() + client._client.get_recording.side_effect = RuntimeError('boom') + + assert client.get_recording_flat('rec-err') is None + + +# --------------------------------------------------------------------------- +# 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.""" + 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'}]}, + ] + + tracks = client.search_tracks_with_artist('Army of Me', 'Björk', limit=10) + + # strict=False is the critical bit — fuzzy recall, not phrase precision + client._client.search_recording.assert_called_once_with( + 'Army of Me', artist_name='Björk', limit=10, strict=False + ) + assert len(tracks) == 1 + assert tracks[0].name == 'Army of Me' + assert 'Björk' in tracks[0].artists + + +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.""" + client = MusicBrainzSearchClient() + client._client = MagicMock() + client._client.search_recording.return_value = [ + {'id': 'rec-1', 'title': 'Some Song', 'score': 90, + 'releases': [], 'artist-credit': [{'name': 'Unknown'}]}, + ] + + 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 + ) + + +def test_search_tracks_with_artist_empty_returns_empty_list(): + """No track and no artist → return [] without hitting the network.""" + client = MusicBrainzSearchClient() + client._client = MagicMock() + + assert client.search_tracks_with_artist('', '', limit=10) == [] + 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.""" + 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'}]}, + ] + + 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 + + +def test_search_tracks_text_keeps_min_score_default_80_for_enhanced_search(): + """The enhanced search tab path keeps the historical 80 floor because + it has no downstream rerank — unfiltered MB results would be noisy + for free-text user search.""" + client = MusicBrainzSearchClient() + client._client = MagicMock() + client._client.search_recording.return_value = [ + {'id': 'rec-good', 'title': 'Good', 'score': 95, + 'releases': [], 'artist-credit': [{'name': 'A'}]}, + {'id': 'rec-mid', 'title': 'Mid', 'score': 40, + 'releases': [], 'artist-credit': [{'name': 'A'}]}, + ] + + # No min_score → defaults to _MIN_SCORE (80) + tracks = client._search_tracks_text('Good', 'A', limit=10) + + titles = [t.name for t in tracks] + assert 'Good' in titles + assert 'Mid' not in titles + + +def test_search_tracks_text_strict_param_default_true(): + """Default strict=True preserves the historical behaviour of the + structured-query text-search fallback path — important so the + enrichment-style `search_tracks('Artist - Track')` flow stays on + field-scoped Lucene phrase matching as before.""" + client = MusicBrainzSearchClient() + client._client = MagicMock() + client._client.search_recording.return_value = [] + + client._search_tracks_text('Track', 'Artist', limit=10) + + client._client.search_recording.assert_called_once_with( + 'Track', artist_name='Artist', limit=10, strict=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.""" + client = MusicBrainzSearchClient() + client._client = MagicMock() + client._client.search_recording.side_effect = RuntimeError('network down') + + assert client.search_tracks_with_artist('Track', 'Artist', limit=10) == [] diff --git a/tests/metadata/test_release_type.py b/tests/metadata/test_release_type.py new file mode 100644 index 00000000..9b6be12a --- /dev/null +++ b/tests/metadata/test_release_type.py @@ -0,0 +1,140 @@ +"""Tests for the canonical release-type mapper. + +Covers issue #650 — MusicBrainz's `Other` and `Broadcast` primary +types previously defaulted to `album_type='album'`, hiding music +videos and one-off releases from artist discography views. The mapper +now routes them to `single` so they land in the Singles bucket of the +artist detail page. + +Also pins the existing mappings (album/ep/single/compilation) so the +refactor of three sibling type-mappers into one shared helper doesn't +drift the historical behaviour. +""" + +from __future__ import annotations + +import pytest + +from core.metadata.release_type import map_release_group_type + + +# --------------------------------------------------------------------------- +# Pin existing primary-type mappings (no regression from refactor) +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize("primary_type,expected", [ + ("album", "album"), + ("Album", "album"), # MB returns title-cased values + ("ALBUM", "album"), + ("single", "single"), + ("Single", "single"), + ("ep", "ep"), + ("EP", "ep"), + ("compilation", "compilation"), + ("Compilation", "compilation"), +]) +def test_known_primary_types_map_canonically(primary_type, expected): + """Pin: case-insensitive primary-type mapping for the four + canonical types every consumer relied on pre-refactor.""" + assert map_release_group_type(primary_type) == expected + + +# --------------------------------------------------------------------------- +# Issue #650 — 'Other' and 'Broadcast' primary types +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize("primary_type", ["other", "Other", "OTHER"]) +def test_other_primary_type_routes_to_singles(primary_type): + """Issue #650: MB tags music videos and one-off web releases with + `primary-type=Other`. They're functionally single-track releases, + so route them to `single` (lands in Singles section). Pre-fix + they fell through to the `album` default — placed in Albums view + where they cluttered the LP list AND, paired with the API filter, + were sometimes dropped from the discography entirely.""" + assert map_release_group_type(primary_type) == "single" + + +@pytest.mark.parametrize("primary_type", ["broadcast", "Broadcast"]) +def test_broadcast_primary_type_routes_to_singles(primary_type): + """Broadcasts (radio sessions, one-off live single transmissions) + are also single-track in practice. Same routing as 'Other'.""" + assert map_release_group_type(primary_type) == "single" + + +# --------------------------------------------------------------------------- +# Secondary-type compilation handling +# --------------------------------------------------------------------------- + + +def test_compilation_secondary_type_overrides_album_primary(): + """MB's canonical compilation pattern is `primary=Album, + secondary=[Compilation]`. The compilation secondary check must + fire even when the primary is Album, so 'Greatest Hits' style + releases land in the compilation bucket.""" + assert map_release_group_type("Album", ["Compilation"]) == "compilation" + + +def test_compilation_secondary_type_case_insensitive(): + """Secondary-type matching tolerates case + whitespace variations + in the provider response.""" + assert map_release_group_type("Album", ["compilation"]) == "compilation" + assert map_release_group_type("Album", [" Compilation "]) == "compilation" + + +def test_other_secondary_types_do_not_override_primary(): + """Only 'compilation' is checked as a secondary-type override. + Other MB secondary types (Live, Remix, Soundtrack, etc.) belong + to the discography filter at the search-adapter layer, not the + type mapper.""" + assert map_release_group_type("Album", ["Live"]) == "album" + assert map_release_group_type("Single", ["Remix"]) == "single" + + +def test_compilation_secondary_overrides_other_primary(): + """An 'Other' release tagged as Compilation lands in compilation, + not singles — secondary-type compilation is the strongest + classification signal.""" + assert map_release_group_type("Other", ["Compilation"]) == "compilation" + + +# --------------------------------------------------------------------------- +# Empty / unknown / defensive +# --------------------------------------------------------------------------- + + +def test_empty_primary_type_defaults_to_album(): + """Pin: empty / None primary-type still defaults to 'album' so + consumers that build records without complete provider data don't + suddenly land in a different bucket.""" + assert map_release_group_type("") == "album" + assert map_release_group_type(None) == "album" + + +def test_unknown_primary_type_defaults_to_album(): + """Pin: a primary-type value we don't know about defaults to + 'album'. Matches the pre-refactor fall-through so new MB + vocabulary doesn't accidentally cause a behaviour shift.""" + assert map_release_group_type("audiobook") == "album" + assert map_release_group_type("video") == "album" + + +def test_secondary_types_none_is_safe(): + """Pin: omitting secondary_types (legacy types.py call site) still + works — None and missing-arg both treated as no-secondary-types.""" + assert map_release_group_type("Album", None) == "album" + assert map_release_group_type("Album") == "album" + + +def test_secondary_types_with_none_entries_skipped(): + """Defensive: provider responses occasionally include None or empty + string in the secondary-types list. The mapper must not crash.""" + assert map_release_group_type("Album", [None, "", "Compilation"]) == "compilation" + assert map_release_group_type("Album", [None, ""]) == "album" + + +def test_whitespace_in_primary_type_normalized(): + """Defensive: a stray-whitespace primary-type still classifies.""" + assert map_release_group_type(" single ") == "single" + assert map_release_group_type(" Other ") == "single" diff --git a/tests/test_unknown_artist_fixer.py b/tests/test_unknown_artist_fixer.py index 4ff15f59..5da80e8d 100644 --- a/tests/test_unknown_artist_fixer.py +++ b/tests/test_unknown_artist_fixer.py @@ -239,3 +239,61 @@ def test_unknown_artist_fixer_supports_hydrabase_title_search(monkeypatch): assert result["source"] == "hydrabase_title_search" assert hydrabase_client.search_calls == [("Hydra Match", 5)] assert spotify_client.search_calls == [] + + +# --------------------------------------------------------------------------- +# Issue #646 — deferred imports inside scan() must resolve at runtime +# --------------------------------------------------------------------------- + + +def test_deferred_path_imports_resolve(): + """Issue #646 regression guard. The Unknown Artist Fixer's scan() + defers `get_file_path_from_template_raw` + `get_audio_quality_string` + imports to keep web_server's heavy boot off the test harness — but + that means a stale import target only surfaces at *runtime*, mid- + scan, with an ImportError. The fixer crashed with exactly that: + + ImportError: cannot import name '_build_path_from_template' from + 'core.repair_jobs.library_reorganize' + + after commit ca5c9316 rewrote `library_reorganize` and moved the + helpers into the import pipeline. + + This test runs the same import statements scan() runs, so the next + refactor that moves these helpers fails CI rather than reaching the + user.""" + from core.imports.paths import get_file_path_from_template_raw # noqa: F401 + from core.imports.file_ops import get_audio_quality_string # noqa: F401 + + +def test_deferred_path_helper_shape_matches_fixer_usage(): + """Pin the shape contract the fixer relies on: pass a template + string + a context dict with the same keys scan() builds, expect a + `(folder, filename_base)` tuple back. If either of those moves, the + fixer's `folder, fname_base = ...` unpack would fail loudly here + instead of producing a malformed expected_rel path.""" + from core.imports.paths import get_file_path_from_template_raw + + template = "$albumartist/$albumartist - $album/$track - $title" + tmpl_ctx = { + "artist": "Test Artist", + "albumartist": "Test Artist", + "album": "Test Album", + "title": "Test Track", + "track_number": 1, + "disc_number": 1, + "year": "2026", + "quality": "FLAC 16bit", + "albumtype": "Album", + } + + result = get_file_path_from_template_raw(template, tmpl_ctx) + + assert isinstance(result, tuple) and len(result) == 2, \ + "Must return a 2-tuple — fixer does `folder, fname_base = result`" + folder, fname_base = result + assert isinstance(folder, str) and isinstance(fname_base, str) + # Folder path must include the album-artist segment from the template. + assert "Test Artist" in folder + # Filename base must include the title from the template. + assert "Test Track" in fname_base diff --git a/tests/tools/test_amazon_client.py b/tests/tools/test_amazon_client.py index a2e21938..33fbac08 100644 --- a/tests/tools/test_amazon_client.py +++ b/tests/tools/test_amazon_client.py @@ -624,11 +624,20 @@ class TestSearchAlbums: albums = client.search_albums("GNX") assert len(albums) == 1 - def test_ignores_track_hits(self): + def test_derives_albums_from_track_hits(self): + """search_albums now intentionally queries `types=track` and derives + Album objects from the album metadata carried on each track hit — + Amazon's album-type query is broken upstream, so the t2tunes fix + switched everything to track-type and reconstructs albums from the + results. Distinct album ASINs across the track hits yield distinct + albums; duplicates collapse via the explicit/clean dedup key.""" client = _make_client({"amazon-music/search": SEARCH_RESPONSE_TRACKS}) with patch("core.amazon_client._rate_limit"): albums = client.search_albums("Kendrick") - assert albums == [] + # Two track hits → two distinct album ASINs → two derived albums. + assert {a.id for a in albums} == {"B0ABCDE123", "B0ABCDE456"} + assert {a.name for a in albums} == {"GNX", "euphoria"} + assert all(a.artists == ["Kendrick Lamar"] for a in albums) def test_strips_explicit_from_album_name(self): resp = { diff --git a/web_server.py b/web_server.py index 695b9a57..e87b4006 100644 --- a/web_server.py +++ b/web_server.py @@ -40,7 +40,7 @@ logger = setup_logging(_log_level, _log_path) # App version — single source of truth for backup metadata, system-info, update check, etc. # Semver: MAJOR.MINOR.PATCH. Bump at each dev→main release. -_SOULSYNC_BASE_VERSION = "2.5.6" +_SOULSYNC_BASE_VERSION = "2.5.7" def _build_version_string(): """Append short commit hash to version when available (e.g. 2.35+abc1234).""" @@ -6068,6 +6068,10 @@ def get_download_status(): transfer_id = file_info.get('id') if transfer_id: try: + logger.info( + f"[CancelTrigger:web.orphan_cleanup] " + f"download_id={transfer_id} username={username}" + ) run_async(download_orchestrator.cancel_download(str(transfer_id), username, remove=True)) except Exception as e: logger.debug("orphan transfer cancel failed: %s", e) @@ -15734,8 +15738,14 @@ def musicbrainz_search_api(): mb_client = mb_svc.mb_client results = [] + # Manual Fix popup is user-facing fuzzy search — recall matters more + # than precision because the user picks the right hit from the list. + # Use bare-query mode so diacritics, aliases, and bracketed suffixes + # like "(Live)" don't kill matches the way strict field-scoped + # phrase queries do. Enrichment workers stay on strict mode (the + # default) since they auto-accept the top hit and need precision. if entity_type == 'artist': - raw = mb_client.search_artist(query, limit=limit) + raw = mb_client.search_artist(query, limit=limit, strict=False) for r in raw: results.append({ 'mbid': r.get('id', ''), @@ -15746,7 +15756,7 @@ def musicbrainz_search_api(): 'country': r.get('country', ''), }) elif entity_type == 'release': - raw = mb_client.search_release(query, artist_name=artist or None, limit=limit) + raw = mb_client.search_release(query, artist_name=artist or None, limit=limit, strict=False) for r in raw: artist_credit = ', '.join(a.get('name', '') for a in r.get('artist-credit', []) if isinstance(a, dict)) results.append({ @@ -15760,7 +15770,7 @@ def musicbrainz_search_api(): 'track_count': r.get('track-count', 0), }) elif entity_type == 'recording': - raw = mb_client.search_recording(query, artist_name=artist or None, limit=limit) + raw = mb_client.search_recording(query, artist_name=artist or None, limit=limit, strict=False) for r in raw: artist_credit = ', '.join(a.get('name', '') for a in r.get('artist-credit', []) if isinstance(a, dict)) releases = r.get('releases', []) @@ -15783,6 +15793,33 @@ def musicbrainz_search_api(): return jsonify({"error": str(e)}), 500 +# Recording MBID format: standard UUID, 8-4-4-4-12 hex. +_MB_RECORDING_MBID_RE = re.compile( + r'^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$', re.IGNORECASE +) + + +@app.route('/api/musicbrainz/recording/', methods=['GET']) +def musicbrainz_recording_lookup_api(mbid): + """Look up a single MusicBrainz recording by MBID and return it in the + Fix-popup-compatible flat track shape. Powers the MBID-paste field — + user-facing escape hatch when fuzzy auto-search ranks the wrong + recording among many same-title versions.""" + mbid = (mbid or '').strip().lower() + if not _MB_RECORDING_MBID_RE.match(mbid): + return jsonify({"error": "Invalid MusicBrainz recording MBID"}), 400 + try: + from core.musicbrainz_search import MusicBrainzSearchClient + mb_search = MusicBrainzSearchClient() + track = mb_search.get_recording_flat(mbid) + if not track: + return jsonify({"error": "Recording not found on MusicBrainz"}), 404 + return jsonify(track) + except Exception as e: + logger.error(f"Error looking up MB recording {mbid}: {e}") + return jsonify({"error": str(e)}), 500 + + @app.route('/api/metadata-cache/mb-match', methods=['POST']) def metadata_cache_save_mb_match(): """Save a manual MusicBrainz match for a failed lookup.""" @@ -16947,6 +16984,10 @@ def cancel_download_task(): # Optionally try to cancel the Soulseek download (don't block worker progression) if download_id and username: try: + logger.info( + f"[CancelTrigger:web.cancel_download_task] " + f"download_id={download_id} username={username} task_id={task_id}" + ) # This is an async call, so we run it and wait run_async(download_orchestrator.cancel_download(download_id, username, remove=True)) logger.warning(f"Successfully cancelled Soulseek download {download_id} for task {task_id}") @@ -17199,6 +17240,10 @@ def cancel_task_v2(): # and silently left streaming downloads running in background. try: logger.info(f"[Atomic Cancel] Dispatching cancel to orchestrator: username={username} download_id={download_id}") + logger.info( + f"[CancelTrigger:web.atomic_cancel_v2] " + f"download_id={download_id} username={username}" + ) cancel_success = run_async( download_orchestrator.cancel_download(download_id, username, remove=True) ) @@ -19157,6 +19202,69 @@ def search_deezer_tracks(): return jsonify({"error": str(e)}), 500 +@app.route('/api/musicbrainz/search_tracks', methods=['GET']) +def search_musicbrainz_tracks(): + """Search for tracks on MusicBrainz — used by the Discovery Fix popup + cascade and any future surface that needs track-level MB search in the + Fix-popup track shape. + + Mirrors the spotify / itunes / deezer search_tracks endpoints exactly: + accepts `track` + `artist` (or legacy `query`) plus `limit`, returns + `{tracks: [{id, name, artists, album, duration_ms, image_url, source}]}`. + + Uses MB's bare-query mode for max recall (diacritic-folded, + alias/sortname indexed) — same rationale as the manual MBID-paste + endpoint shipped earlier. The Fix popup is a user-facing fuzzy search + where the user picks from the result list, so recall beats precision. + """ + try: + track_q = request.args.get('track', '').strip() + artist_q = request.args.get('artist', '').strip() + legacy_query = request.args.get('query', '').strip() + limit = int(request.args.get('limit', 20)) + + if not (track_q or artist_q or legacy_query): + return jsonify({"error": "Query parameter is required"}), 400 + + from core.musicbrainz_search import MusicBrainzSearchClient + mb_search = MusicBrainzSearchClient() + + if track_q or artist_q: + tracks = mb_search.search_tracks_with_artist( + track_q or legacy_query, artist_q, limit=limit + ) + else: + # Legacy single-string query — let MB's structured-query + # dispatch decide artist-first browse vs text search. + tracks = mb_search.search_tracks(legacy_query, limit=limit) + + # Local rerank — same helper Deezer / iTunes use. Penalises + # cover / karaoke / tribute patterns + boosts exact-artist match. + 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, + ) + + tracks_dict = [{ + 'id': t.id, + 'name': t.name, + 'artists': t.artists, + 'album': t.album, + 'duration_ms': t.duration_ms, + 'image_url': t.image_url, + 'source': 'musicbrainz', + } for t in tracks] + + return jsonify({'tracks': tracks_dict}) + + except Exception as e: + logger.error(f"Error searching MusicBrainz tracks: {e}") + return jsonify({"error": str(e)}), 500 + + @app.route('/api/itunes/album/', methods=['GET']) def get_itunes_album_tracks(album_id): """Fetches full track details for a specific iTunes album.""" diff --git a/webui/index.html b/webui/index.html index c0c86dd8..4bcfba98 100644 --- a/webui/index.html +++ b/webui/index.html @@ -2418,7 +2418,7 @@
- +
diff --git a/webui/src/app/router.test.tsx b/webui/src/app/router.test.tsx index f347a5be..63295b4b 100644 --- a/webui/src/app/router.test.tsx +++ b/webui/src/app/router.test.tsx @@ -60,6 +60,8 @@ function createShellBridge(overrides: Partial = {}): ShellBridge { resolveLegacyPath: vi.fn<(pathname: string) => ShellPageId | null>(() => 'search'), setActivePageChrome: vi.fn(), activateLegacyPath: vi.fn(), + navigateToArtistDetail: vi.fn(), + cancelSimilarArtistsLoad: vi.fn(), showReactHost: vi.fn(), ...overrides, }; diff --git a/webui/src/platform/shell/bridge.test.ts b/webui/src/platform/shell/bridge.test.ts index 00dd88d4..9682bbe7 100644 --- a/webui/src/platform/shell/bridge.test.ts +++ b/webui/src/platform/shell/bridge.test.ts @@ -17,6 +17,7 @@ describe('waitForShellContext', () => { getCurrentProfileContext: vi.fn(() => ({ profileId: 2, isAdmin: true })), resolveLegacyPath: vi.fn(() => 'issues'), setActivePageChrome: vi.fn(), + cancelSimilarArtistsLoad: vi.fn(), showReactHost: vi.fn(), } as NonNullable; @@ -38,6 +39,7 @@ describe('waitForShellContext', () => { getCurrentProfileContext, resolveLegacyPath: vi.fn(() => 'issues'), setActivePageChrome: vi.fn(), + cancelSimilarArtistsLoad: vi.fn(), showReactHost: vi.fn(), } as NonNullable; @@ -88,4 +90,13 @@ describe('bindWindowWebRouter', () => { replace: true, }); }); + + it('refuses artist detail navigation without an artist id', async () => { + const navigate = vi.fn().mockResolvedValue(undefined); + + bindWindowWebRouter({ navigate } as never); + + await expect(window.SoulSyncWebRouter?.navigateToPage('artist-detail', {} as never)).resolves.toBe(false); + expect(navigate).not.toHaveBeenCalled(); + }); }); diff --git a/webui/src/platform/shell/bridge.ts b/webui/src/platform/shell/bridge.ts index 1ca85768..1c9683f5 100644 --- a/webui/src/platform/shell/bridge.ts +++ b/webui/src/platform/shell/bridge.ts @@ -88,6 +88,9 @@ export function bindWindowWebRouter(router: AnyRouter) { async navigateToPage(pageId, options) { const route = getShellRouteByPageId(pageId); if (!route) return false; + if (pageId === 'artist-detail' && !options?.artistId) { + return false; + } let href: `/${string}` = route.path; if (pageId === 'artist-detail' && options?.artistId) { diff --git a/webui/src/platform/shell/globals.d.ts b/webui/src/platform/shell/globals.d.ts index 27de813c..65d5b471 100644 --- a/webui/src/platform/shell/globals.d.ts +++ b/webui/src/platform/shell/globals.d.ts @@ -35,6 +35,15 @@ declare global { resolveLegacyPath: (pathname: string) => ShellPageId | null; setActivePageChrome: (pageId: ShellPageId) => void; activateLegacyPath: (pathname: string) => void; + navigateToArtistDetail: ( + artistId: string | number, + artistName: string, + sourceOverride?: string | null, + options?: { + skipRouteChange?: boolean; + }, + ) => void; + cancelSimilarArtistsLoad: () => void; showReactHost: (pageId: ShellPageId) => void; }; } diff --git a/webui/src/platform/shell/route-manifest.test.ts b/webui/src/platform/shell/route-manifest.test.ts index 4430dd11..369f7b3d 100644 --- a/webui/src/platform/shell/route-manifest.test.ts +++ b/webui/src/platform/shell/route-manifest.test.ts @@ -16,7 +16,7 @@ describe('shellRouteManifest', () => { expect(resolveShellPageFromPath('/discover')).toBe('discover'); expect(resolveShellPageFromPath('/watchlist')).toBe('watchlist'); expect(resolveShellPageFromPath('/active-downloads')).toBe('active-downloads'); - expect(resolveShellPageFromPath('/artist-detail')).toBe('artist-detail'); + expect(resolveShellPageFromPath('/artist-detail')).toBeNull(); expect(resolveShellPageFromPath('/artist-detail/spotify/2YZyLoL8N0Wb9xBt1NhZWg')).toBe('artist-detail'); expect(resolveShellPageFromPath('/artists')).toBeNull(); }); @@ -50,6 +50,7 @@ describe('shellRouteManifest', () => { expect(resolveLegacyShellPageFromPath('/search')).toBe('search'); expect(resolveLegacyShellPageFromPath('/active-downloads')).toBe('active-downloads'); expect(resolveLegacyShellPageFromPath('/tools')).toBe('tools'); + expect(resolveLegacyShellPageFromPath('/artist-detail')).toBeNull(); expect(resolveLegacyShellPageFromPath('/artist-detail/deezer/12345')).toBe('artist-detail'); expect(resolveLegacyShellPageFromPath('/issues')).toBeNull(); expect(resolveLegacyShellPageFromPath('/does-not-exist')).toBeNull(); diff --git a/webui/src/platform/shell/route-manifest.ts b/webui/src/platform/shell/route-manifest.ts index 6b1a209e..469d46d1 100644 --- a/webui/src/platform/shell/route-manifest.ts +++ b/webui/src/platform/shell/route-manifest.ts @@ -72,7 +72,10 @@ export function getShellRouteByPath(pathname: string): ShellRouteDefinition | un export function resolveShellPageFromPath(pathname: string): ShellPageId | null { const normalized = normalizeShellPath(pathname); - if (normalized === '/artist-detail' || normalized.startsWith('/artist-detail/')) { + if (normalized === '/artist-detail') { + return null; + } + if (normalized.startsWith('/artist-detail/')) { return 'artist-detail'; } return getShellRouteByPath(pathname)?.pageId ?? null; @@ -80,7 +83,10 @@ export function resolveShellPageFromPath(pathname: string): ShellPageId | null { export function resolveLegacyShellPageFromPath(pathname: string): ShellPageId | null { const normalized = normalizeShellPath(pathname); - if (normalized === '/artist-detail' || normalized.startsWith('/artist-detail/')) { + if (normalized === '/artist-detail') { + return null; + } + if (normalized.startsWith('/artist-detail/')) { return 'artist-detail'; } const route = getShellRouteByPath(pathname); diff --git a/webui/src/routeTree.gen.ts b/webui/src/routeTree.gen.ts index d90bb0ce..ca94e35b 100644 --- a/webui/src/routeTree.gen.ts +++ b/webui/src/routeTree.gen.ts @@ -12,6 +12,7 @@ import { Route as rootRouteImport } from './routes/__root' import { Route as SplatRouteImport } from './routes/$' import { Route as IssuesRouteRouteImport } from './routes/issues/route' import { Route as IndexRouteImport } from './routes/index' +import { Route as ArtistDetailSourceIdRouteImport } from './routes/artist-detail/$source/$id' const SplatRoute = SplatRouteImport.update({ id: '/$', @@ -28,35 +29,44 @@ const IndexRoute = IndexRouteImport.update({ path: '/', getParentRoute: () => rootRouteImport, } as any) +const ArtistDetailSourceIdRoute = ArtistDetailSourceIdRouteImport.update({ + id: '/artist-detail/$source/$id', + path: '/artist-detail/$source/$id', + getParentRoute: () => rootRouteImport, +} as any) export interface FileRoutesByFullPath { '/': typeof IndexRoute '/issues': typeof IssuesRouteRoute '/$': typeof SplatRoute + '/artist-detail/$source/$id': typeof ArtistDetailSourceIdRoute } export interface FileRoutesByTo { '/': typeof IndexRoute '/issues': typeof IssuesRouteRoute '/$': typeof SplatRoute + '/artist-detail/$source/$id': typeof ArtistDetailSourceIdRoute } export interface FileRoutesById { __root__: typeof rootRouteImport '/': typeof IndexRoute '/issues': typeof IssuesRouteRoute '/$': typeof SplatRoute + '/artist-detail/$source/$id': typeof ArtistDetailSourceIdRoute } export interface FileRouteTypes { fileRoutesByFullPath: FileRoutesByFullPath - fullPaths: '/' | '/issues' | '/$' + fullPaths: '/' | '/issues' | '/$' | '/artist-detail/$source/$id' fileRoutesByTo: FileRoutesByTo - to: '/' | '/issues' | '/$' - id: '__root__' | '/' | '/issues' | '/$' + to: '/' | '/issues' | '/$' | '/artist-detail/$source/$id' + id: '__root__' | '/' | '/issues' | '/$' | '/artist-detail/$source/$id' fileRoutesById: FileRoutesById } export interface RootRouteChildren { IndexRoute: typeof IndexRoute IssuesRouteRoute: typeof IssuesRouteRoute SplatRoute: typeof SplatRoute + ArtistDetailSourceIdRoute: typeof ArtistDetailSourceIdRoute } declare module '@tanstack/react-router' { @@ -82,6 +92,13 @@ declare module '@tanstack/react-router' { preLoaderRoute: typeof IndexRouteImport parentRoute: typeof rootRouteImport } + '/artist-detail/$source/$id': { + id: '/artist-detail/$source/$id' + path: '/artist-detail/$source/$id' + fullPath: '/artist-detail/$source/$id' + preLoaderRoute: typeof ArtistDetailSourceIdRouteImport + parentRoute: typeof rootRouteImport + } } } @@ -89,6 +106,7 @@ const rootRouteChildren: RootRouteChildren = { IndexRoute: IndexRoute, IssuesRouteRoute: IssuesRouteRoute, SplatRoute: SplatRoute, + ArtistDetailSourceIdRoute: ArtistDetailSourceIdRoute, } export const routeTree = rootRouteImport ._addFileChildren(rootRouteChildren) diff --git a/webui/src/routes/artist-detail/$source/$id.tsx b/webui/src/routes/artist-detail/$source/$id.tsx new file mode 100644 index 00000000..6987c00e --- /dev/null +++ b/webui/src/routes/artist-detail/$source/$id.tsx @@ -0,0 +1,31 @@ +import { createFileRoute } from '@tanstack/react-router'; +import { useLayoutEffect } from 'react'; + +import { useShellBridge } from '@/platform/shell/route-controllers'; + +export const Route = createFileRoute('/artist-detail/$source/$id')({ + component: ArtistDetailPage, +}); + +// Thin legacy handoff: TanStack owns the URL shape here, but the vanilla JS +// artist-detail page still renders the actual experience for now. The route +// owns cancellation so similar-artist loading stops when this page changes. +function ArtistDetailPage() { + const bridge = useShellBridge(); + const { source, id } = Route.useParams(); + + useLayoutEffect(() => { + if (!bridge) return; + + const normalizedSource = source.toLowerCase() === 'library' ? null : source.toLowerCase(); + bridge.navigateToArtistDetail(id, '', normalizedSource, { + skipRouteChange: true, + }); + + return () => { + bridge.cancelSimilarArtistsLoad(); + }; + }, [bridge, id, source]); + + return null; +} diff --git a/webui/src/routes/artist-detail/-route.test.tsx b/webui/src/routes/artist-detail/-route.test.tsx new file mode 100644 index 00000000..232ff944 --- /dev/null +++ b/webui/src/routes/artist-detail/-route.test.tsx @@ -0,0 +1,99 @@ +import { createMemoryHistory } from '@tanstack/react-router'; +import { render, waitFor } from '@testing-library/react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import type { ShellBridge, ShellPageId } from '@/platform/shell/bridge'; + +import { createAppQueryClient } from '@/app/query-client'; +import { AppRouterProvider, createAppRouter } from '@/app/router'; + +function createShellBridge(overrides: Partial = {}): ShellBridge { + return { + getCurrentProfileContext: vi.fn(() => ({ profileId: 2, isAdmin: false })), + isPageAllowed: vi.fn(() => true), + getProfileHomePage: vi.fn<() => ShellPageId>(() => 'discover'), + resolveLegacyPath: vi.fn<(pathname: string) => ShellPageId | null>(() => 'artist-detail'), + setActivePageChrome: vi.fn(), + activateLegacyPath: vi.fn(), + navigateToArtistDetail: vi.fn(), + cancelSimilarArtistsLoad: vi.fn(), + showReactHost: vi.fn(), + ...overrides, + }; +} + +function renderArtistDetailRoute(initialEntries = ['/artist-detail/library/42']) { + const queryClient = createAppQueryClient(); + const history = createMemoryHistory({ initialEntries }); + const router = createAppRouter({ history, queryClient }); + + return { + history, + router, + ...render(), + }; +} + +describe('artist-detail route', () => { + beforeEach(() => { + window.SoulSyncWebShellBridge = createShellBridge(); + }); + + afterEach(() => { + window.SoulSyncWebShellBridge = undefined; + }); + + it('hands off canonical artist-detail URLs to the legacy shell', async () => { + renderArtistDetailRoute(['/artist-detail/spotify/2YZyLoL8N0Wb9xBt1NhZWg']); + + await waitFor(() => { + expect(window.SoulSyncWebShellBridge?.navigateToArtistDetail).toHaveBeenCalledWith( + '2YZyLoL8N0Wb9xBt1NhZWg', + '', + 'spotify', + { + skipRouteChange: true, + }, + ); + }); + }); + + it('normalizes library sources before handing off', async () => { + renderArtistDetailRoute(['/artist-detail/library/42']); + + await waitFor(() => { + expect(window.SoulSyncWebShellBridge?.navigateToArtistDetail).toHaveBeenCalledWith( + '42', + '', + null, + { + skipRouteChange: true, + }, + ); + }); + }); + + it('cancels the similar artists stream when the route unmounts', async () => { + const { unmount } = renderArtistDetailRoute(['/artist-detail/spotify/2YZyLoL8N0Wb9xBt1NhZWg']); + + await waitFor(() => { + expect(window.SoulSyncWebShellBridge?.navigateToArtistDetail).toHaveBeenCalledWith( + '2YZyLoL8N0Wb9xBt1NhZWg', + '', + 'spotify', + { + skipRouteChange: true, + }, + ); + }); + + const cancelSimilarArtistsLoad = window.SoulSyncWebShellBridge?.cancelSimilarArtistsLoad as ReturnType; + cancelSimilarArtistsLoad.mockClear(); + + unmount(); + + await waitFor(() => { + expect(cancelSimilarArtistsLoad).toHaveBeenCalledTimes(1); + }); + }); +}); diff --git a/webui/src/routes/issues/-route.test.tsx b/webui/src/routes/issues/-route.test.tsx index 402966e1..7bbc0353 100644 --- a/webui/src/routes/issues/-route.test.tsx +++ b/webui/src/routes/issues/-route.test.tsx @@ -22,6 +22,8 @@ function createShellBridge(overrides: Partial = {}): ShellBridge { resolveLegacyPath: vi.fn<(pathname: string) => ShellPageId | null>(() => 'search'), setActivePageChrome: vi.fn(), activateLegacyPath: vi.fn(), + navigateToArtistDetail: vi.fn(), + cancelSimilarArtistsLoad: vi.fn(), showReactHost: vi.fn(), ...overrides, }; diff --git a/webui/static/api-monitor.js b/webui/static/api-monitor.js index 4b0fc398..d53d0667 100644 --- a/webui/static/api-monitor.js +++ b/webui/static/api-monitor.js @@ -2316,6 +2316,25 @@ async function openWatchlistArtistDetailView(artistId, artistName) { if (artist.mood) metaTags.push(`${escapeHtml(artist.mood)}`); if (artist.label) metaTags.push(`${escapeHtml(artist.label)}`); + let discogId = null; + let discogSource = null; + const activeSrc = (currentMusicSourceName || '').toLowerCase(); + if (activeSrc.includes('spotify') && spotify_artist_id) { + discogId = spotify_artist_id; discogSource = 'spotify'; + } else if (activeSrc.includes('discogs') && discogs_artist_id) { + discogId = discogs_artist_id; discogSource = 'discogs'; + } else if (activeSrc.includes('deezer') && deezer_artist_id) { + discogId = deezer_artist_id; discogSource = 'deezer'; + } else if (activeSrc.includes('musicbrainz') && musicbrainz_artist_id) { + discogId = musicbrainz_artist_id; discogSource = 'musicbrainz'; + } else if (itunes_artist_id) { + discogId = itunes_artist_id; discogSource = 'itunes'; + } else { + discogId = spotify_artist_id || discogs_artist_id || deezer_artist_id || musicbrainz_artist_id || itunes_artist_id; + discogSource = spotify_artist_id ? 'spotify' : discogs_artist_id ? 'discogs' : deezer_artist_id ? 'deezer' : musicbrainz_artist_id ? 'musicbrainz' : 'itunes'; + } + const discogHref = discogId ? buildArtistDetailPath(discogId, discogSource) : '#'; + overlay.innerHTML = ` ${artist.banner_url ? `
@@ -2398,7 +2417,7 @@ async function openWatchlistArtistDetailView(artistId, artistName) {
- + View Discography
@@ -2410,30 +2429,6 @@ async function openWatchlistArtistDetailView(artistId, artistName) { closeWatchlistArtistDetailView(); }); - overlay.querySelector('.watchlist-detail-discog-action').addEventListener('click', () => { - // Use the ID matching the active metadata source - let discogId, source; - const activeSrc = (currentMusicSourceName || '').toLowerCase(); - if (activeSrc.includes('spotify') && spotify_artist_id) { - discogId = spotify_artist_id; source = 'spotify'; - } else if (activeSrc.includes('discogs') && discogs_artist_id) { - discogId = discogs_artist_id; source = 'discogs'; - } else if (activeSrc.includes('deezer') && deezer_artist_id) { - discogId = deezer_artist_id; source = 'deezer'; - } else if (activeSrc.includes('musicbrainz') && musicbrainz_artist_id) { - discogId = musicbrainz_artist_id; source = 'musicbrainz'; - } else if (itunes_artist_id) { - discogId = itunes_artist_id; source = 'itunes'; - } else { - discogId = spotify_artist_id || discogs_artist_id || deezer_artist_id || musicbrainz_artist_id || itunes_artist_id; - source = spotify_artist_id ? 'spotify' : discogs_artist_id ? 'discogs' : deezer_artist_id ? 'deezer' : musicbrainz_artist_id ? 'musicbrainz' : 'itunes'; - } - if (discogId) { - closeWatchlistArtistDetailView(); - navigateToArtistDetail(discogId, artistName, source); - } - }); - overlay.querySelector('.watchlist-detail-settings-action').addEventListener('click', () => { // Remove overlay immediately so it doesn't block the config modal const detailOverlay = document.querySelector('.watchlist-artist-detail-overlay'); diff --git a/webui/static/core.js b/webui/static/core.js index 426db439..f1b047f6 100644 --- a/webui/static/core.js +++ b/webui/static/core.js @@ -201,6 +201,13 @@ let artistsSearchController = null; let artistCompletionController = null; // Track ongoing completion check to cancel when navigating away let similarArtistsController = null; // Track ongoing similar artists stream to cancel when navigating away +function cancelSimilarArtistsLoad() { + if (similarArtistsController) { + similarArtistsController.abort(); + similarArtistsController = null; + } +} + // --- Lazy Background Image Observer --- // Watches elements with data-bg-src, applies background-image when visible, unobserves after. const lazyBgObserver = new IntersectionObserver((entries) => { diff --git a/webui/static/discover.js b/webui/static/discover.js index 99e7124c..e1d05c12 100644 --- a/webui/static/discover.js +++ b/webui/static/discover.js @@ -269,10 +269,9 @@ function displayDiscoverHeroArtist(artist) { if (discographyBtn && artistId) { discographyBtn.setAttribute('data-artist-id', artistId); discographyBtn.setAttribute('data-artist-name', artist.artist_name); - // Source the click handler will pass to navigateToArtistDetail. Without - // this, source-only hero artists (which is the typical case — they - // come from discover similar-artists, not the library) get looked up - // as library IDs and 404. Backend always includes artist.source. + discographyBtn.href = buildArtistDetailPath(artistId, artist.source || null); + // Keep the source on the link so source-only hero artists resolve to + // the correct artist-detail URL instead of being treated as library IDs. if (artist.source) discographyBtn.setAttribute('data-source', artist.source); else discographyBtn.removeAttribute('data-source'); // Also store both IDs for cross-source operations @@ -557,7 +556,7 @@ function renderRecommendedArtistsModal(modal, artists, source = null) { const similarText = artist.occurrence_count > 1 ? `Similar to ${artist.occurrence_count} in your watchlist` : 'Similar to an artist in your watchlist'; - const artistSource = artist.source || source || _recommendedArtistsSource || ''; + const artistSource = artist.source || source || _recommendedArtistsSource || ''; return `
Add to Watchlist -
- ${artist.image_url ? ` - ${escapeHtml(artist.artist_name)} - ` : ` -
🎤
- `} -
-
- ${escapeHtml(artist.artist_name)} - ${similarText} -
${genreTags}
-
+ +
+ ${artist.image_url ? ` + ${escapeHtml(artist.artist_name)} + ` : ` +
🎤
+ `} +
+
+ ${escapeHtml(artist.artist_name)} + ${similarText} +
${genreTags}
+
+
`; }).join('')} @@ -599,16 +602,6 @@ function renderRecommendedArtistsModal(modal, artists, source = null) { if (watchlistBtn) { e.stopPropagation(); toggleRecommendedWatchlist(watchlistBtn); - return; - } - - const card = e.target.closest('.recommended-artist-card'); - if (card) { - const artistId = card.getAttribute('data-artist-id'); - const artistSource = card.getAttribute('data-artist-source') || _recommendedArtistsSource || null; - const nameEl = card.querySelector('.recommended-card-name'); - const artistName = nameEl ? nameEl.textContent : ''; - viewRecommendedArtistDiscography(artistId, artistName, artistSource); } }); } @@ -739,11 +732,6 @@ async function checkRecommendedWatchlistStatuses(artists) { } } -async function viewRecommendedArtistDiscography(artistId, artistName, source = null) { - closeRecommendedArtistsModal(); - navigateToArtistDetail(artistId, artistName, source || null); -} - async function checkAllHeroWatchlistStatus() { const btn = document.getElementById('discover-hero-watch-all'); if (!btn || !discoverHeroArtists || discoverHeroArtists.length === 0) return; @@ -811,26 +799,6 @@ function jumpToDiscoverHeroSlide(index) { updateDiscoverHeroIndicators(); } -async function viewDiscoverHeroDiscography() { - const button = document.getElementById('discover-hero-discography'); - if (!button) return; - - const artistId = button.getAttribute('data-artist-id'); - const artistName = button.getAttribute('data-artist-name'); - // Pass the source so /api/artist-detail knows to synthesize from that - // metadata provider instead of doing a local DB lookup. Hero similar - // artists are almost always source-only (not in the library). - const source = button.getAttribute('data-source') || null; - - if (!artistId || !artistName) { - console.error('No artist data found for discography view'); - return; - } - - console.log(`🎵 Navigating to artist detail for: ${artistName} (source: ${source || 'library'})`); - navigateToArtistDetail(artistId, artistName, source); -} - function showDiscoverHeroEmpty() { const titleEl = document.getElementById('discover-hero-title'); const subtitleEl = document.getElementById('discover-hero-subtitle'); @@ -4514,10 +4482,7 @@ function _renderYourArtistCard(artist) { const detailSource = _pickArtistDetailSource(artist); const hasId = detailSource.id && detailSource.id !== ''; - // Navigate to Artists page (name click) — source artist id, needs inline view - const navAction = hasId - ? `event.stopPropagation(); navigateToArtistDetail('${escapeForInlineJs(detailSource.id)}', '${escapeForInlineJs(artist.artist_name)}', '${escapeForInlineJs(detailSource.source)}' || null)` - : ''; + const detailHref = hasId ? buildArtistDetailPath(detailSource.id, detailSource.source) : ''; // Open info modal (card body click) — pass pool ID so we can look up all data const infoAction = hasId @@ -4545,7 +4510,9 @@ function _renderYourArtistCard(artist) {
${originDots}
-
${_esc(artist.artist_name)}
+ ${hasId + ? `${_esc(artist.artist_name)}` + : `
${_esc(artist.artist_name)}
`} `; @@ -4695,10 +4662,10 @@ async function openYourArtistInfoModal(poolId) { Explore - + `; } } catch (err) { @@ -6595,9 +6562,9 @@ function _artMapSetupInteraction(canvas) {
ⓘ Artist Info
-
+ 💿 View Discography -
+
👁 ${node.type === 'watchlist' ? 'On Watchlist' : 'Add to Watchlist'}
@@ -7275,9 +7242,9 @@ async function openGenreDeepDive(genre) { // Always open on Artists page with discography — pass source for correct routing const imgUrl = _esc(a.image_url || ''); const artSource = _esc(a.source || ''); - const clickAction = `onclick="document.getElementById('genre-deep-dive-modal').remove();navigateToArtistDetail('${_esc(a.entity_id)}','${_esc(a.name)}','${artSource}' || null)"`; + const detailHref = a.entity_id ? buildArtistDetailPath(a.entity_id, a.source || null) : '#'; const srcClass = (a.source || '').toLowerCase(); - return `
+ return `
${!a.image_url ? '🎤' : ''}
@@ -7285,7 +7252,7 @@ async function openGenreDeepDive(genre) {
${_esc(a.name)}
${a.followers ? `
${_fmtNum(a.followers)} followers
` : ''} ${a.library_id ? '
In Library
' : ''} -
`; + `; }).join('')} `; @@ -8768,4 +8735,3 @@ if (document.readyState === 'loading') { } // ============================================================================ - diff --git a/webui/static/downloads.js b/webui/static/downloads.js index 7374753d..678c1796 100644 --- a/webui/static/downloads.js +++ b/webui/static/downloads.js @@ -633,48 +633,6 @@ async function openDownloadMissingModalForYouTube(virtualPlaylistId, playlistNam hideLoadingOverlay(); } -function _navigateToArtistFromModal(artistId, artistName, imageUrl, source, playlistId) { - if (!artistName) return; - // Close the download modal - const process = playlistId ? activeDownloadProcesses[playlistId] : null; - const artistContext = process?.artist || {}; - const inferredSource = artistContext.spotify_artist_id ? 'spotify' - : artistContext.itunes_artist_id ? 'itunes' - : (artistContext.deezer_artist_id || artistContext.deezer_id) ? 'deezer' - : (artistContext.discogs_artist_id || artistContext.discogs_id) ? 'discogs' - : (artistContext.amazon_artist_id || artistContext.amazon_id) ? 'amazon' - : (artistContext.soul_id || artistContext.hydrabase_artist_id) ? 'hydrabase' - : null; - const resolvedSource = source || process?.artist?.source || process?.album?.source || process?.source || inferredSource; - const sourceKey = (resolvedSource || '').toString().toLowerCase(); - const sourceIdFields = { - spotify: ['spotify_artist_id', 'id', 'artist_id'], - itunes: ['itunes_artist_id', 'artist_id', 'id'], - deezer: ['deezer_artist_id', 'deezer_id', 'artist_id', 'id'], - discogs: ['discogs_artist_id', 'discogs_id', 'artist_id', 'id'], - amazon: ['amazon_artist_id', 'amazon_id', 'artist_id', 'id'], - hydrabase: ['soul_id', 'hydrabase_artist_id', 'artist_id', 'id'], - musicbrainz: ['musicbrainz_id', 'artist_id', 'id'], - }; - let resolvedArtistId = artistId; - for (const field of (sourceIdFields[sourceKey] || ['artist_id', 'id'])) { - const candidate = artistContext?.[field]; - if (candidate) { - resolvedArtistId = candidate; - break; - } - } - if (resolvedArtistId && String(resolvedArtistId).toLowerCase() === String(artistName).toLowerCase()) { - resolvedArtistId = null; - } - if (playlistId) closeDownloadMissingModal(playlistId); - if (!resolvedArtistId || !resolvedSource) { - showToast(`Artist details are not available for ${artistName}`, 'warning'); - return; - } - navigateToArtistDetail(resolvedArtistId, artistName, resolvedSource); -} - async function closeDownloadMissingModal(playlistId) { const process = activeDownloadProcesses[playlistId]; if (!process) { @@ -5698,13 +5656,13 @@ function _gsRenderFromState(state) { if (dbArtists.length) { h += '
📚 In Your Library
'; - h += dbArtists.map(a => `
${a.image_url ? `` : '🎤'}
${_escToast(a.name)}
Library
`).join(''); + h += dbArtists.map(a => `${a.image_url ? `
` : '
🎤
'}
${_escToast(a.name)}
Library
`).join(''); h += '
'; } if (artists.length) { h += `
🎤 Artists ${srcLabel}
`; - h += artists.map(a => `
${a.image_url ? `` : '🎤'}
${_escToast(a.name)}
`).join(''); + h += artists.map(a => `${a.image_url ? `
` : '
🎤
'}
${_escToast(a.name)}
`).join(''); h += '
'; } @@ -5778,13 +5736,6 @@ async function _gsLazyLoadArtistImages() { } } -function _gsClickArtist(id, name, isLibrary) { - _gsDeactivate(); - const activeSource = _gsController && _gsController.state.activeSource; - const source = isLibrary ? null : (activeSource || null); - navigateToArtistDetail(id, name, source); -} - async function _gsClickAlbum(albumId, albumName, artistName, imageUrl, source) { _gsDeactivate(); // Same flow as handleEnhancedSearchAlbumClick — fetch album, open download modal @@ -6400,4 +6351,3 @@ const additionalStyles = ` document.head.insertAdjacentHTML('beforeend', additionalStyles); // ============================================================================ - diff --git a/webui/static/helper.js b/webui/static/helper.js index fc8036d9..61df89d5 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3413,9 +3413,24 @@ function closeHelperSearch() { // projects that span multiple commits before shipping. Strip the flag at // release time and add a real `date:` line at the top of the version block. const WHATS_NEW = { + '2.5.7': [ + { date: 'May 19, 2026 — 2.5.7 release' }, + { title: 'Fix: MusicBrainz manual search missing results', desc: 'the Fix popup and manual library service search were using strict Lucene phrase-match queries against the `recording` / `release` / `artist` fields — diacritics ("Bjork" vs canonical "Björk"), bracketed suffixes like "(Live)", and any AND-clause mismatch all killed recall. switched user-facing manual lookups to bare queries that hit MB\'s alias / sortname indexes with diacritic folding. enrichment workers stay strict for precision.' }, + { title: 'Fix: MusicBrainz album clicks 404ing in enhanced search', desc: 'every click on a MusicBrainz album result was silently 404-ing — the /release fetch was passing `cover-art-archive` as an `inc` param, which MB rejects with 400 (that field is returned on every release response by default, no include needed). dropped the bad include; album detail now loads correctly.' }, + { title: 'Fix popup: paste a MusicBrainz URL or MBID to match directly', desc: 'new escape hatch on the Fix Track Match modal (the 🔧 Fix button on mirrored / YouTube / Tidal / Deezer / Beatport / ListenBrainz / Spotify-public discovery rows). when fuzzy search keeps ranking the wrong recording among many same-title versions, paste the MusicBrainz recording URL like `https://musicbrainz.org/recording/` or the bare UUID into the new field and hit "Look up". skips all fuzzy logic, resolves straight to that record, and runs it through the same confirm + match pipeline.' }, + { title: 'Fix popup: MusicBrainz added to the auto-search cascade', desc: 'the Fix Track Match modal used to query only Spotify → Deezer → iTunes for the auto-search, leaving MusicBrainz out of the loop entirely — even for users with MusicBrainz set as their primary metadata source. now MB is part of the cascade. when MB is your primary, it gets queried first; otherwise it sits as the last fallback. catches niche / non-mainstream / canonical-with-diacritics recordings that the commercial sources miss. Discogs is intentionally absent — Discogs has no track-level search API.' }, + { title: 'Fix: Docker basic-search streaming silently failed under rootless Docker', desc: 'the streaming "Play" flow on the basic search page tried to create `/app/Stream` lazily at runtime, which fails silently when the container runs under rootless Docker / Podman (in-container root can\'t write to `/app`). pre-baked the directory at image build time, matching the same pattern that fixed `/app/Staging` earlier in the cycle. non-persistent — no volume needed.' }, + { title: 'Fix: slskd-unreachable log spam during non-Soulseek downloads', desc: 'when slskd was configured but not actually running (or unreachable on its configured port), the `/api/downloads/status` polling loop fanned out to every download plugin including Soulseek, producing one `ERROR - Cannot connect to host ... [Name or service not known]` log line per poll for the entire duration of any download — visible spam even when the user wasn\'t using Soulseek at all. Connection failures now emit one WARNING with actionable context (start slskd, or clear the slskd_url if you don\'t use Soulseek) and demote subsequent failures to debug. The flag resets on the next successful slskd response so a later outage warns again.' }, + { title: 'Fix: MusicBrainz "Other" release-groups now visible in discography', desc: 'MB tags music videos, one-off web releases, and broadcast singles with `primary-type=Other` — common pattern for Vocaloid producers, JP indie artists, and some Western indie acts. The release-group browse filter only requested `album|ep|single`, dropping every Other-typed group at the API layer. Combined with the inline type mapper defaulting unknown primary types to "album", this hid legitimate tracks from the artist detail page and left downloaded tracks orphaned (visible in track counts but not bound to any visible album / single card). Centralised the type-mapper into one shared helper (`core/metadata/release_type.py`) used by every provider\'s raw→Album projection, added `Other` and `Broadcast` handling that routes those release-groups into the Singles section, and added `other` to the MB API filter. For inabakumori, this surfaces 5 previously-invisible releases. 23 unit tests pin the mapper contract; existing 65 search-adapter tests still green.' }, + { title: 'Fix: quarantined files no longer re-downloaded on auto-wishlist cycles', desc: 'when a file failed AcoustID verification and got quarantined, the next auto-wishlist run would search for the same track again, and the candidate picker\'s deterministic quality ranking kept selecting the same `(uploader, filename)` source — re-downloading and re-quarantining the exact same file every cycle. Users woke up to hundreds of duplicate `.quarantined` entries from a single bad upload. The Soulseek candidate filter now reads quarantine sidecars and drops any candidate whose `(username, filename)` matches a previously-quarantined source before the quality picker ranks it. Filesystem read on every search (sub-ms in practice). Approving or deleting a quarantine entry removes its source from the dedup set automatically — the user can give the source a second chance by explicitly approving / deleting the quarantine record.' }, + { title: 'Fix: Unknown Artist Fixer tool crashed on every run', desc: 'the "Fix Unknown Artists" repair job crashed instantly with `ImportError: cannot import name \'_build_path_from_template\'` — totally unrunnable. Commit ca5c9316 ("Rewrite Library Reorganize job to delegate to per-album planner") moved the private path-builder + quality-string helpers out of `core.repair_jobs.library_reorganize` and into the import pipeline (`core.imports.paths` / `core.imports.file_ops`), but the Unknown Artist Fixer\'s deferred-import path was left pointing at the old module. Re-wired to the new locations. Added two regression tests that exercise the deferred imports directly — the next refactor that moves these helpers fails CI rather than reaching the user.' }, + ], '2.5.6': [ { date: 'May 18, 2026 — 2.5.6 release' }, { title: 'MusicBrainz as Primary Metadata Source', desc: 'MusicBrainz is now a full primary metadata source on equal footing with Deezer, iTunes, Spotify, and Discogs. switch to it in Settings → Metadata Source — always available, no account or API key needed, rate-limited to 1 req/sec. covers all primary source flows: search, album/track/artist lookup, watchlist scans, discover hero, similar artist backfill, artist map.', page: 'settings' }, + { title: 'Fix: MusicBrainz artist detail showing MBID as name', desc: 'clicking a MusicBrainz artist from search results was showing the raw MBID as the artist name on the detail page. URL-driven routing (PR #644) no longer passes the display name to the backend, so the source detail endpoint now looks it up directly from MusicBrainz by MBID.' }, + { title: 'Fix: artist detail back button always showing "← Back"', desc: 'PR #644 removed the back-button label logic along with the origin stack. restored: a label stack (separate from browser history, which handles actual navigation) tracks where you came from across the full similar-artist chain — "← Back to Search", "← Back to Artist A", "← Back to Artist B", etc. API response backfills the current artist name so the stack has real names when clicking similar artists.' }, + { title: 'Fix: Amazon search albums/artists missing, album downloads all track 01', desc: 't2tunes proxies Amazon Music and uses 400 to signal transient failures — first API call in a session hit this consistently, so album/artist searches always failed while track search (called 0.5s later) scraped through. added up to 3 retries with backoff on t2tunes-specific 400s. also: all search methods were using types=track,album but t2tunes album-type queries are broken — switched everything to types=track and derive albums/artists from track metadata instead. track numbers from album downloads were also always 1 — added index-based fallback when t2tunes tags omit trackNumber.' }, ], '2.5.5': [ { date: 'May 17, 2026 — 2.5.5 release' }, @@ -3780,7 +3795,7 @@ function _getLatestWhatsNewVersion() { const versions = Object.keys(WHATS_NEW) .filter(v => _compareVersions(v, buildVer) <= 0) .sort((a, b) => _compareVersions(b, a)); - return versions[0] || '2.5.6'; + return versions[0] || '2.5.7'; } function openWhatsNew() { diff --git a/webui/static/init.js b/webui/static/init.js index c874f6fc..f00e5852 100644 --- a/webui/static/init.js +++ b/webui/static/init.js @@ -2154,10 +2154,6 @@ function _getPageFromPath() { const basePage = segs[0]; if (!_DEEPLINK_VALID_PAGES.has(basePage)) return 'dashboard'; // Context-dependent pages fall back to a sensible parent - if (basePage === 'artist-detail') { - // /artist-detail/:id deep-link — keep on artist-detail; bare /artist-detail falls back to library - return (segs.length >= 2 && segs[1]) ? 'artist-detail' : 'library'; - } if (basePage === 'playlist-explorer') return 'library'; return basePage; } @@ -2168,35 +2164,13 @@ function _normalizeArtistDetailSource(source) { } function buildArtistDetailPath(artistId, source = null) { - if (!artistId) return '/artist-detail'; + if (!artistId) { + throw new Error('artistId is required for artist-detail navigation'); + } const normalizedSource = _normalizeArtistDetailSource(source); return '/artist-detail/' + encodeURIComponent(normalizedSource) + '/' + encodeURIComponent(String(artistId)); } -/** Extract artist source + ID from /artist-detail/:source/:id or legacy /artist-detail/:id URLs. */ -function _getDeepLinkArtistDetail(pathname = window.location.pathname) { - const path = (pathname || '').replace(/^\/+|\/+$/g, ''); - const segs = path.split('/'); - if (segs[0] !== 'artist-detail' || !segs[1]) return null; - - if (segs[2]) { - return { - source: _normalizeArtistDetailSource(decodeURIComponent(segs[1])), - artistId: decodeURIComponent(segs.slice(2).join('/')), - }; - } - - return { - source: 'library', - artistId: decodeURIComponent(segs[1]), - }; -} - -/** Legacy convenience wrapper for callers that only need the artist ID. */ -function _getDeepLinkArtistId() { - return _getDeepLinkArtistDetail()?.artistId || null; -} - // =============================== // MOBILE NAVIGATION // =============================== @@ -2310,6 +2284,10 @@ function navigateToPage(pageId, options = {}) { return; } + if (pageId === 'artist-detail' && !options.artistId) { + return false; + } + const router = getWebRouter(); if (router && !options.skipRouteChange) { notifyPageWillChange(pageId); @@ -2386,7 +2364,10 @@ async function loadPageData(pageId) { case 'library': // Check if we should return to artist detail view instead of list if (artistDetailPageState.currentArtistId && artistDetailPageState.currentArtistName) { - navigateToPage('artist-detail'); + navigateToPage('artist-detail', { + artistId: artistDetailPageState.currentArtistId, + artistSource: artistDetailPageState.currentArtistSource, + }); if (!artistDetailPageState.isInitialized) { initializeArtistDetailPage(); loadArtistDetailData(artistDetailPageState.currentArtistId, artistDetailPageState.currentArtistName); @@ -2400,7 +2381,7 @@ async function loadPageData(pageId) { } break; case 'artist-detail': - // Artist detail page is handled separately by navigateToArtistDetail() + // Artist detail page is entered through the route handoff and legacy navigator. break; case 'discover': if (!discoverPageInitialized) { diff --git a/webui/static/library.js b/webui/static/library.js index 19818b16..f8ed0960 100644 --- a/webui/static/library.js +++ b/webui/static/library.js @@ -218,6 +218,7 @@ function displayLibraryArtists(artists) { // Ignore clicks on badge icons (they open external links / toggle watchlist) const badge = e.target.closest('.source-card-icon'); if (badge) { + e.preventDefault(); e.stopPropagation(); const url = badge.dataset.url; if (url) { window.open(url, '_blank'); return; } @@ -233,10 +234,6 @@ function displayLibraryArtists(artists) { } return; } - const card = e.target.closest('.library-artist-card'); - if (card) { - navigateToArtistDetail(card.dataset.artistId, card.dataset.artistName); - } }; } @@ -299,14 +296,14 @@ function buildLibraryArtistCardHTML(artist, index) { // Track stats const trackStat = artist.track_count > 0 ? `${artist.track_count} track${artist.track_count !== 1 ? 's' : ''}` : ''; - return `
+ return ` ${badgeContainerHTML} ${imageHTML}

${_esc(artist.name)}

${trackStat}
-
`; + `; } function updateLibraryPagination(pagination) { @@ -683,16 +680,31 @@ async function toggleLibraryCardWatchlist(btn, artist) { // =============================================== // Artist detail page state +const _ARTIST_DETAIL_BACK_LABELS = { + library: 'Back to Library', + search: 'Back to Search', + discover: 'Back to Discover', + watchlist: 'Back to Watchlist', + wishlist: 'Back to Wishlist', + stats: 'Back to Stats', + 'playlist-explorer': 'Back to Explorer', + automations: 'Back to Automations', + dashboard: 'Back to Dashboard', + sync: 'Back to Sync', + 'active-downloads': 'Back to Downloads', +}; + +// Stack of origins for the back-button label. Each entry: {type:'page', pageId} +// or {type:'artist', name}. Pushed on forward navigation, popped on back. +// Separate from browser history — only used for the label display. +let _artistDetailLabelStack = []; +let _artistDetailGoingBack = false; + let artistDetailPageState = { isInitialized: false, currentArtistId: null, currentArtistName: null, currentArtistSource: null, - // Stack of origins captured by navigateToArtistDetail for the back button. - // Each entry is either {type:'page', pageId} or {type:'artist', id, name, source} - // so chained navigation (Search → A → similar B → similar C) walks back one - // step at a time instead of jumping straight to Search. - originStack: [], enhancedView: false, enhancedData: null, expandedAlbums: new Set(), @@ -710,7 +722,6 @@ function clearArtistDetailPageState() { artistDetailPageState.currentArtistId = null; artistDetailPageState.currentArtistName = null; artistDetailPageState.currentArtistSource = null; - artistDetailPageState.originStack = []; } if (typeof window !== 'undefined') { @@ -786,22 +797,6 @@ if (typeof window !== 'undefined') { } -// Friendly labels for the dynamic "← Back to X" button on the artist-detail page. -// Page id (the value of currentPage) -> button label. -const _ARTIST_DETAIL_BACK_LABELS = { - library: 'Back to Library', - search: 'Back to Search', - discover: 'Back to Discover', - watchlist: 'Back to Watchlist', - wishlist: 'Back to Wishlist', - stats: 'Back to Stats', - 'playlist-explorer': 'Back to Explorer', - automations: 'Back to Automations', - dashboard: 'Back to Dashboard', - sync: 'Back to Sync', - 'active-downloads': 'Back to Downloads', -}; - function navigateToArtistDetail(artistId, artistName, sourceOverride = null, options = {}) { const normalizedSource = sourceOverride || null; @@ -815,48 +810,30 @@ function navigateToArtistDetail(artistId, artistName, sourceOverride = null, opt navigateToPage('artist-detail', { artistId, artistSource: normalizedSource, - skipRouteChange: true + skipRouteChange: options.skipRouteChange === true }); - _updateArtistDetailBackButtonLabel(); } return; } console.log(`🎵 Navigating to artist detail: ${artistName} (ID: ${artistId}${sourceOverride ? `, source: ${sourceOverride}` : ''})`); - // Capture the current location on the origin stack BEFORE navigateToPage - // flips currentPage. The back button walks this stack one step at a time, - // so a chain like Search → A → similar B → similar C steps back through - // C → B → A → Search instead of jumping straight home. `skipOriginPush` - // lets the back button re-enter a prior artist without re-pushing. - if (!options.skipOriginPush) { - // Fresh entry (from a non-artist page) starts a new chain; any stale - // entries from a prior artist-detail visit are dropped. + // Maintain the label stack. Back navigations pop; forward navigations push. + // Only treat the flag as a back-nav signal when we're still on artist-detail — + // if history.back() landed on a non-artist page first, the flag is stale. + if (_artistDetailGoingBack && currentPage === 'artist-detail') { + _artistDetailLabelStack.pop(); + _artistDetailGoingBack = false; + } else { + _artistDetailGoingBack = false; // clear any stale flag if (currentPage !== 'artist-detail') { - artistDetailPageState.originStack = []; + _artistDetailLabelStack = []; // fresh chain from a non-artist page } - - let entry; - if (currentPage === 'artist-detail' && artistDetailPageState.currentArtistId) { - entry = { - type: 'artist', - id: artistDetailPageState.currentArtistId, - name: artistDetailPageState.currentArtistName, - source: artistDetailPageState.currentArtistSource, - }; + if (currentPage === 'artist-detail' && artistDetailPageState.currentArtistName) { + _artistDetailLabelStack.push({ type: 'artist', name: artistDetailPageState.currentArtistName }); } else { const pageId = (typeof currentPage === 'string' && currentPage && currentPage !== 'artist-detail') ? currentPage : 'library'; - entry = { type: 'page', pageId }; - } - - // Avoid pushing a duplicate top entry on repeated clicks of the same target. - const top = artistDetailPageState.originStack[artistDetailPageState.originStack.length - 1]; - const isDuplicate = top && top.type === entry.type && ( - (entry.type === 'page' && top.pageId === entry.pageId) || - (entry.type === 'artist' && String(top.id) === String(entry.id)) - ); - if (!isDuplicate) { - artistDetailPageState.originStack.push(entry); + _artistDetailLabelStack.push({ type: 'page', pageId }); } } @@ -909,14 +886,13 @@ function navigateToArtistDetail(artistId, artistName, sourceOverride = null, opt skipRouteChange: options.skipRouteChange === true }); - // Update back-button label to reflect where the next pop will land. - _updateArtistDetailBackButtonLabel(); - // Initialize if needed and load data if (!artistDetailPageState.isInitialized) { initializeArtistDetailPage(); } + _updateArtistDetailBackButtonLabel(); + // Load artist data loadArtistDetailData(artistId, artistName); } @@ -924,11 +900,12 @@ function navigateToArtistDetail(artistId, artistName, sourceOverride = null, opt function _updateArtistDetailBackButtonLabel() { const backBtnLabel = document.querySelector('#artist-detail-back-btn span'); if (!backBtnLabel) return; - const stack = artistDetailPageState.originStack || []; - const top = stack[stack.length - 1]; + const top = _artistDetailLabelStack[_artistDetailLabelStack.length - 1]; if (!top) { - backBtnLabel.textContent = `← ${_ARTIST_DETAIL_BACK_LABELS.library}`; - } else if (top.type === 'artist') { + backBtnLabel.textContent = '← Back'; + return; + } + if (top.type === 'artist') { backBtnLabel.textContent = `← Back to ${top.name}`; } else { const friendly = _ARTIST_DETAIL_BACK_LABELS[top.pageId] || _ARTIST_DETAIL_BACK_LABELS.library; @@ -939,9 +916,8 @@ function _updateArtistDetailBackButtonLabel() { function initializeArtistDetailPage() { console.log("🔧 Initializing Artist Detail page..."); - // Initialize back button — pops the origin stack one step at a time so a - // chain like Search → A → B → C walks back through C → B → A → Search - // instead of jumping straight to the original entry page. + // Initialize back button — use browser history when possible, with a + // simple library fallback if the user lands here without in-app history. const backBtn = document.getElementById("artist-detail-back-btn"); if (backBtn) { backBtn.addEventListener("click", () => { @@ -951,28 +927,16 @@ function initializeArtistDetailPage() { artistDetailPageState.completionController = null; } - const stack = artistDetailPageState.originStack || []; - if (stack.length > 0) { - const target = stack.pop(); - if (target.type === 'artist') { - // Re-enter a prior artist in the chain without re-pushing, - // so the stack keeps shrinking as the user steps back. - navigateToArtistDetail(target.id, target.name, target.source, { skipOriginPush: true }); - return; - } - // target.type === 'page' — fully exit the artist-detail chain - artistDetailPageState.currentArtistId = null; - artistDetailPageState.currentArtistName = null; - artistDetailPageState.originStack = []; - navigateToPage(target.pageId); + if (window.history.length > 1) { + _artistDetailGoingBack = true; + window.history.back(); return; } - // No history — default to library - artistDetailPageState.currentArtistId = null; - artistDetailPageState.currentArtistName = null; - artistDetailPageState.originStack = []; - navigateToPage('library'); + // No history — fall back to recorded origin page or library + const top = _artistDetailLabelStack.pop(); + _updateArtistDetailBackButtonLabel(); + navigateToPage(top?.type === 'page' ? (top.pageId || 'library') : 'library'); }); } @@ -1078,6 +1042,15 @@ async function loadArtistDetailData(artistId, artistName) { artistDetailPageState.currentArtistId = data.artist.id; } + // Backfill name from API response — URL-driven navigation passes '' for the + // name so the label stack has real names when the user clicks a similar artist. + if (data.artist?.name && !artistDetailPageState.currentArtistName) { + artistDetailPageState.currentArtistName = data.artist.name; + if (typeof _updateSidebarLibraryBreadcrumb === 'function') { + _updateSidebarLibraryBreadcrumb(); + } + } + // Keep the resolved metadata source for album-track lookups. artistDetailPageState.currentArtistSource = data.discography?.source || data.artist?.source || null; @@ -1265,6 +1238,9 @@ function populateArtistDetailPage(data) { // MusicMap name lookup). Fire-and-forget — the function handles its own // loading state and errors. if (artist && artist.name && typeof loadSimilarArtists === 'function') { + if (typeof cancelSimilarArtistsLoad === 'function') { + cancelSimilarArtistsLoad(); + } loadSimilarArtists(artist.name); } } diff --git a/webui/static/media-player.js b/webui/static/media-player.js index 85f25796..b300de5a 100644 --- a/webui/static/media-player.js +++ b/webui/static/media-player.js @@ -107,6 +107,21 @@ function setTrackInfo(track) { document.getElementById('no-track-message').classList.add('hidden'); document.getElementById('media-player').classList.remove('idle'); + const gotoArtistBtn = document.getElementById('np-goto-artist'); + if (gotoArtistBtn) { + if (track.artist_id) { + gotoArtistBtn.href = buildArtistDetailPath(track.artist_id, track.artist_source || null); + gotoArtistBtn.style.pointerEvents = ''; + gotoArtistBtn.setAttribute('aria-disabled', 'false'); + gotoArtistBtn.tabIndex = 0; + } else { + gotoArtistBtn.href = '#'; + gotoArtistBtn.style.pointerEvents = 'none'; + gotoArtistBtn.setAttribute('aria-disabled', 'true'); + gotoArtistBtn.tabIndex = -1; + } + } + // Sync expanded player and media session updateNpTrackInfo(); updateMediaSessionMetadata(); @@ -184,6 +199,14 @@ function clearTrack() { document.getElementById('no-track-message').classList.remove('hidden'); document.getElementById('media-player').classList.add('idle'); + const gotoArtistBtn = document.getElementById('np-goto-artist'); + if (gotoArtistBtn) { + gotoArtistBtn.href = '#'; + gotoArtistBtn.style.pointerEvents = 'none'; + gotoArtistBtn.setAttribute('aria-disabled', 'true'); + gotoArtistBtn.tabIndex = -1; + } + // Reset queue state npQueue = []; npQueueIndex = -1; @@ -1231,15 +1254,11 @@ function initExpandedPlayer() { }); } - // Action button (Go to Artist) + // Action link (Go to Artist) const gotoArtistBtn = document.getElementById('np-goto-artist'); if (gotoArtistBtn) { - gotoArtistBtn.addEventListener('click', () => { - if (currentTrack && currentTrack.artist_id) { - closeNowPlayingModal(); - navigateToArtistDetail(currentTrack.artist_id, currentTrack.artist || ''); - } - }); + gotoArtistBtn.style.textDecoration = 'none'; + gotoArtistBtn.style.color = 'inherit'; } // Buffering state listeners on audioPlayer if (audioPlayer) { @@ -2396,4 +2415,3 @@ function updateMediaSessionPlaybackState() { } // =============================== - diff --git a/webui/static/search.js b/webui/static/search.js index fd6d2604..4974fad9 100644 --- a/webui/static/search.js +++ b/webui/static/search.js @@ -317,11 +317,7 @@ function initializeSearchModeToggle() { name: artist.name, meta: 'In Your Library', badge: { text: 'Library', class: 'enh-badge-library' }, - onClick: () => { - console.log(`🎵 Opening library artist detail: ${artist.name} (ID: ${artist.id})`); - hideDropdown(); - navigateToArtistDetail(artist.id, artist.name); - } + href: buildArtistDetailPath(artist.id), }) ); @@ -337,12 +333,7 @@ function initializeSearchModeToggle() { name: artist.name, meta: 'Artist', badge: sourceBadge, - onClick: () => { - const sourceOverride = searchController.state.activeSource; - console.log(`🎵 Opening artist detail: ${artist.name} (ID: ${artist.id}, source: ${sourceOverride})`); - hideDropdown(); - navigateToArtistDetail(artist.id, artist.name, sourceOverride || null); - } + href: buildArtistDetailPath(artist.id, searchController.state.activeSource || null), }) ); @@ -1197,16 +1188,7 @@ async function loadInitialData() { return; } - if (targetPage === 'artist-detail') { - const deepArtist = _getDeepLinkArtistDetail(); - if (deepArtist?.artistId && typeof navigateToArtistDetail === 'function') { - navigateToArtistDetail(deepArtist.artistId, '', deepArtist.source === 'library' ? null : deepArtist.source); - } else { - navigateToPage('library', { skipRouteChange: true, forceReload: true }); - } - } else { - navigateToPage(targetPage, { skipRouteChange: true, forceReload: true }); - } + navigateToPage(targetPage, { forceReload: true }); } catch (error) { console.error('Error loading initial data:', error); } diff --git a/webui/static/shared-helpers.js b/webui/static/shared-helpers.js index 7a509f6c..9840fea3 100644 --- a/webui/static/shared-helpers.js +++ b/webui/static/shared-helpers.js @@ -545,7 +545,8 @@ function renderCompactSection(sectionId, listId, countId, items, mapItem) { items.forEach(item => { const config = mapItem(item); - const elem = document.createElement('div'); + const isLink = isArtist && !!config.href; + const elem = document.createElement(isLink ? 'a' : 'div'); // Add appropriate card class if (isArtist) { @@ -566,6 +567,13 @@ function renderCompactSection(sectionId, listId, countId, items, mapItem) { elem.className = 'enh-compact-item track-item'; } + if (isLink) { + elem.href = config.href; + elem.style.color = 'inherit'; + elem.style.textDecoration = 'none'; + elem.setAttribute('aria-label', config.name || 'Artist'); + } + // Build image HTML with type-specific classes let imageClass = 'enh-item-image'; let placeholderClass = 'enh-item-image-placeholder'; @@ -612,7 +620,9 @@ function renderCompactSection(sectionId, listId, countId, items, mapItem) { ${badgeHtml} `; - elem.addEventListener('click', config.onClick); + if (config.onClick) { + elem.addEventListener('click', config.onClick); + } // Add play button handler for tracks if (isTrack && config.onPlay) { @@ -695,7 +705,7 @@ async function checkDiscographyCompletion(artistId, discography) { } catch (error) { // Don't show error if it was aborted (user navigated away) if (error.name === 'AbortError') { - console.log('⏹️ Completion check aborted (user navigated to new artist)'); + console.log('⏹️ Completion check aborted (user navigated away)'); return; } @@ -3574,16 +3584,21 @@ async function loadSimilarArtists(artistName) { container.innerHTML = ''; section.style.display = 'block'; + let controller = null; + let signal = null; + try { // Create new abort controller for this similar artists stream - similarArtistsController = new AbortController(); + controller = new AbortController(); + similarArtistsController = controller; + signal = controller.signal; // Use streaming endpoint for real-time bubble creation const url = `/api/artist/similar/${encodeURIComponent(artistName)}/stream`; console.log(`📡 Streaming from: ${url}`); const response = await fetch(url, { - signal: similarArtistsController.signal + signal }); if (!response.ok) { @@ -3612,6 +3627,7 @@ async function loadSimilarArtists(artistName) { buffer = messages.pop() || ''; // Keep incomplete message in buffer for (const message of messages) { + if (signal?.aborted) return; if (!message.trim() || !message.startsWith('data: ')) continue; try { @@ -3622,6 +3638,7 @@ async function loadSimilarArtists(artistName) { } if (jsonData.artist) { + if (signal?.aborted) return; // Hide loading on first artist if (artistCount === 0) { loadingEl.classList.add('hidden'); @@ -3636,6 +3653,7 @@ async function loadSimilarArtists(artistName) { } if (jsonData.complete) { + if (signal?.aborted) return; console.log(`🎉 Streaming complete: ${jsonData.total} artists`); if (artistCount === 0) { @@ -3648,7 +3666,7 @@ async function loadSimilarArtists(artistName) { `; } else { // Lazy load images for similar artists that don't have them - lazyLoadSimilarArtistImages(container); + await lazyLoadSimilarArtistImages(container, signal); } } } catch (parseError) { @@ -3658,13 +3676,14 @@ async function loadSimilarArtists(artistName) { } // Clear the controller when done - similarArtistsController = null; + if (similarArtistsController === controller) { + similarArtistsController = null; + } } catch (error) { // Don't show error if it was aborted (user navigated away) - if (error.name === 'AbortError') { - console.log('⏹️ Similar artists stream aborted (user navigated to new artist)'); - loadingEl.classList.add('hidden'); + if (error.name === 'AbortError' || signal?.aborted) { + console.log('⏹️ Similar artists stream aborted (user navigated away)'); return; } @@ -3683,15 +3702,18 @@ async function loadSimilarArtists(artistName) { `; } finally { // Always clear the controller - similarArtistsController = null; + if (similarArtistsController === controller) { + similarArtistsController = null; + } } } /** * Lazy load images for similar artist bubbles that don't have images */ -async function lazyLoadSimilarArtistImages(container) { +async function lazyLoadSimilarArtistImages(container, signal) { if (!container) return; + if (signal?.aborted) return; const bubblesNeedingImages = container.querySelectorAll('.similar-artist-bubble[data-needs-image="true"]'); @@ -3707,9 +3729,11 @@ async function lazyLoadSimilarArtistImages(container) { const bubbles = Array.from(bubblesNeedingImages); for (let i = 0; i < bubbles.length; i += batchSize) { + if (signal?.aborted) return; const batch = bubbles.slice(i, i + batchSize); await Promise.all(batch.map(async (bubble) => { + if (signal?.aborted) return; const artistId = bubble.getAttribute('data-artist-id'); const artistSource = bubble.getAttribute('data-artist-source') || ''; const artistPlugin = bubble.getAttribute('data-artist-plugin') || ''; @@ -3724,9 +3748,11 @@ async function lazyLoadSimilarArtistImages(container) { ? `/api/artist/${encodeURIComponent(artistId)}/image?${params.toString()}` : `/api/artist/${encodeURIComponent(artistId)}/image`; - const response = await fetch(imageUrl); + const response = await fetch(imageUrl, { signal }); const data = await response.json(); + if (signal?.aborted) return; + if (data.success && data.image_url) { const imageContainer = bubble.querySelector('.similar-artist-bubble-image'); if (imageContainer) { @@ -3737,6 +3763,9 @@ async function lazyLoadSimilarArtistImages(container) { } } } catch (error) { + if (error?.name === 'AbortError' || signal?.aborted) { + return; + } console.warn(`⚠️ Failed to load image for similar artist ${artistId}:`, error); } })); @@ -3803,10 +3832,23 @@ function displaySimilarArtists(artists) { * Create a similar artist bubble card element */ function createSimilarArtistBubble(artist) { + const artistId = artist.id ? String(artist.id).trim() : ''; + const hasArtistDetail = !!artistId; + // Create bubble container - const bubble = document.createElement('div'); + const bubble = document.createElement(hasArtistDetail ? 'a' : 'div'); bubble.className = 'similar-artist-bubble'; - bubble.setAttribute('data-artist-id', artist.id); + if (hasArtistDetail) { + bubble.href = buildArtistDetailPath(artistId, artist.source || null); + } else { + bubble.setAttribute('aria-disabled', 'true'); + bubble.style.cursor = 'default'; + bubble.style.pointerEvents = 'none'; + } + bubble.style.color = 'inherit'; + bubble.style.textDecoration = 'none'; + bubble.setAttribute('aria-label', artist.name); + bubble.setAttribute('data-artist-id', artistId); bubble.setAttribute('data-artist-source', artist.source || ''); if (artist.plugin) { bubble.setAttribute('data-artist-plugin', artist.plugin); @@ -3858,13 +3900,6 @@ function createSimilarArtistBubble(artist) { bubble.appendChild(genres); } - // Click → navigate to the standalone artist-detail page. Works for both - // library and source artists thanks to the source-aware backend endpoint. - bubble.addEventListener('click', () => { - console.log(`🎵 Clicked similar artist: ${artist.name} (ID: ${artist.id})`); - navigateToArtistDetail(artist.id, artist.name, artist.source || null); - }); - return bubble; } @@ -3933,3 +3968,31 @@ function showCompletionError() { overlay.title = 'Failed to check completion status'; }); } + + +// ---------------------------------------------------------------------------- +// MusicBrainz MBID parsing — accept full URLs or bare UUIDs. +// Used by the Discovery Fix popup's MBID-paste lookup; reusable anywhere the +// user can paste a MusicBrainz identifier (failed-MB cache, manual match, +// future surfaces). Returns the bare lowercase UUID when valid, or null. +// ---------------------------------------------------------------------------- + +const _MB_UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; + +function parseMusicBrainzMbid(input) { + if (!input || typeof input !== 'string') return null; + let candidate = input.trim(); + if (!candidate) return null; + + // Strip a musicbrainz.org URL down to the final UUID segment. Accepts + // /recording/, /release/, /release-group/, /artist/ — the caller decides + // which entity type is appropriate for its endpoint. + const urlMatch = candidate.match(/musicbrainz\.org\/[a-z-]+\/([0-9a-f-]+)/i); + if (urlMatch) candidate = urlMatch[1]; + + // Strip trailing query / fragment that survived a copy-paste from a URL + // (e.g. "?utm=...", "#tab"). + candidate = candidate.split(/[?#]/)[0].trim().toLowerCase(); + + return _MB_UUID_RE.test(candidate) ? candidate : null; +} diff --git a/webui/static/shell-bridge.js b/webui/static/shell-bridge.js index 8d2ac58a..66127869 100644 --- a/webui/static/shell-bridge.js +++ b/webui/static/shell-bridge.js @@ -80,16 +80,6 @@ function activateLegacyPath(pathname) { return; } - if (targetPage === 'artist-detail') { - const deepArtist = _getDeepLinkArtistDetail(pathname); - if (deepArtist?.artistId && typeof navigateToArtistDetail === 'function') { - navigateToArtistDetail(deepArtist.artistId, '', deepArtist.source === 'library' ? null : deepArtist.source, { skipOriginPush: true, skipRouteChange: true }); - return; - } - navigateToPage('library', { replace: true }); - return; - } - notifyPageWillChange(targetPage); activatePage(targetPage, { forceReload: true }); } @@ -107,16 +97,6 @@ function syncActivePageFromLocation() { return; } - if (targetPage === 'artist-detail') { - const deepArtist = _getDeepLinkArtistDetail(); - if (deepArtist?.artistId && typeof navigateToArtistDetail === 'function') { - navigateToArtistDetail(deepArtist.artistId, '', deepArtist.source === 'library' ? null : deepArtist.source, { skipOriginPush: true, skipRouteChange: true }); - return; - } - navigateToPage('library', { replace: true }); - return; - } - notifyPageWillChange(targetPage); const route = router?.routeManifest?.find((entry) => entry.pageId === targetPage); if (route?.kind === 'react') { @@ -185,10 +165,58 @@ window.SoulSyncWebShellBridge = { activateLegacyPath(pathname) { activateLegacyPath(pathname); }, + navigateToArtistDetail, + cancelSimilarArtistsLoad() { + if (typeof cancelSimilarArtistsLoad === 'function') { + cancelSimilarArtistsLoad(); + } + }, showReactHost(pageId) { showReactHost(pageId); }, }; +function _handleShellLinkClick(event) { + if (event.defaultPrevented || event.button !== 0 || _isModifiedLinkClick(event)) return; + + const anchor = event.target?.closest?.('a[href]'); + if (!anchor || (anchor.target && anchor.target !== '_self')) return; + + const href = anchor.getAttribute('href'); + if (!href || href === '#' || href.startsWith('javascript:')) return; + + const router = getWebRouter(); + if (!router?.navigateToPage) return; + + const pathname = anchor.pathname || new URL(anchor.href, window.location.href).pathname; + + if (pathname.startsWith('/artist-detail/')) { + _handleArtistDetailLinkClick(event, pathname, router); + return; + } +} + +function _handleArtistDetailLinkClick(event, pathname, router) { + const parts = pathname.split('/').filter(Boolean); + if (parts.length < 3) return; + + // Keep the semantic link, but hand the click back to TanStack so artist + // detail navigations stay in the SPA when the router is available. + const source = decodeURIComponent(parts[1] || ''); + const artistId = decodeURIComponent(parts.slice(2).join('/')); + if (!source || !artistId) return; + + event.preventDefault(); + void router.navigateToPage('artist-detail', { + artistId, + artistSource: source, + }); +} + +function _isModifiedLinkClick(event) { + return event.metaKey || event.ctrlKey || event.shiftKey || event.altKey; +} + window.addEventListener('popstate', syncActivePageFromLocation); +document.addEventListener('click', _handleShellLinkClick, true); window.dispatchEvent(new CustomEvent(SHELL_BRIDGE_READY_EVENT)); diff --git a/webui/static/stats-automations.js b/webui/static/stats-automations.js index 265d71eb..100f4a55 100644 --- a/webui/static/stats-automations.js +++ b/webui/static/stats-automations.js @@ -162,7 +162,7 @@ async function loadStatsData() { ${i + 1} ${item.image_url ? `` : ''}
-
${item.id ? `${_esc(item.name)}` : _esc(item.name)}${item.soul_id && !String(item.soul_id).startsWith('soul_unnamed_') ? ' ' : ''}
+
${item.id ? `${_esc(item.name)}` : _esc(item.name)}${item.soul_id && !String(item.soul_id).startsWith('soul_unnamed_') ? ' ' : ''}
${item.global_listeners ? _fmt(item.global_listeners) + ' global listeners' : ''}
${_fmt(item.play_count)} plays @@ -176,7 +176,7 @@ async function loadStatsData() { ${item.image_url ? `` : ''}
${_esc(item.name)}
-
${item.artist_id ? `${_esc(item.artist || '')}` : _esc(item.artist || '')}
+
${item.artist_id ? `${_esc(item.artist || '')}` : _esc(item.artist || '')}
${_fmt(item.play_count)} plays @@ -189,7 +189,7 @@ async function loadStatsData() { ${item.image_url ? `` : ''}
${_esc(item.name)}
-
${item.artist_id ? `${_esc(item.artist || '')}` : _esc(item.artist || '')}${item.album ? ' · ' + _esc(item.album) : ''}
+
${item.artist_id ? `${_esc(item.artist || '')}` : _esc(item.artist || '')}${item.album ? ' · ' + _esc(item.album) : ''}
${_fmt(item.play_count)} plays @@ -231,7 +231,7 @@ function _renderTopArtistsVisual(artists) { ${top5.map((a, i) => { const pct = Math.round((a.play_count / maxPlays) * 100); const size = 44 + (4 - i) * 6; // Largest first: 68, 62, 56, 50, 44 - return `
+ return `
${!a.image_url ? `${(a.name || '?')[0]}` : ''}
@@ -240,7 +240,7 @@ function _renderTopArtistsVisual(artists) {
${_esc(a.name)}
${_fmt(a.play_count)}
- `; + `; }).join('')} `; } diff --git a/webui/static/style.css b/webui/static/style.css index a17d2abd..a6f7619c 100644 --- a/webui/static/style.css +++ b/webui/static/style.css @@ -16876,7 +16876,7 @@ body.helper-mode-active #dashboard-activity-feed:hover { margin-top: 32px; } -.watchlist-detail-actions button { +.watchlist-detail-actions > * { flex: 1; padding: 12px 20px; border-radius: 8px; @@ -16885,9 +16885,14 @@ body.helper-mode-active #dashboard-activity-feed:hover { font-weight: 600; cursor: pointer; transition: background 0.15s ease, transform 0.1s ease; + display: inline-flex; + align-items: center; + justify-content: center; + text-decoration: none; + box-sizing: border-box; } -.watchlist-detail-actions button:active { +.watchlist-detail-actions > *:active { transform: scale(0.98); } diff --git a/webui/static/sync-services.js b/webui/static/sync-services.js index 8eeececb..11b50933 100644 --- a/webui/static/sync-services.js +++ b/webui/static/sync-services.js @@ -7414,6 +7414,19 @@ function openYouTubeDiscoveryModal(urlHash) { 🔍 Search + + +
+ + +
diff --git a/webui/static/sync-spotify.js b/webui/static/sync-spotify.js index e86fe83b..ad8ba041 100644 --- a/webui/static/sync-spotify.js +++ b/webui/static/sync-spotify.js @@ -1993,6 +1993,28 @@ function generateDownloadModalHeroSection(context) { const artistImage = artist?.image_url || artist?.images?.[0]?.url; const albumImage = album?.image_url || album?.images?.[0]?.url; const artistSource = artist?.source || album?.source || context.source || ''; + const sourceKey = (artistSource || '').toString().toLowerCase(); + const sourceIdFields = { + spotify: ['spotify_artist_id', 'id', 'artist_id'], + itunes: ['itunes_artist_id', 'artist_id', 'id'], + deezer: ['deezer_artist_id', 'deezer_id', 'artist_id', 'id'], + discogs: ['discogs_artist_id', 'discogs_id', 'artist_id', 'id'], + amazon: ['amazon_artist_id', 'amazon_id', 'artist_id', 'id'], + hydrabase: ['soul_id', 'hydrabase_artist_id', 'artist_id', 'id'], + musicbrainz: ['musicbrainz_id', 'artist_id', 'id'], + }; + let detailArtistId = artist?.id || artist?.artist_id || ''; + for (const field of (sourceIdFields[sourceKey] || ['artist_id', 'id'])) { + const candidate = artist?.[field]; + if (candidate) { + detailArtistId = candidate; + break; + } + } + if (detailArtistId && String(detailArtistId).toLowerCase() === String(artist?.name || '').toLowerCase()) { + detailArtistId = ''; + } + const artistHref = detailArtistId ? buildArtistDetailPath(detailArtistId, artistSource || null) : '#'; // Use album image as background if available if (albumImage) { @@ -2007,7 +2029,7 @@ function generateDownloadModalHeroSection(context) {

${escapeHtml(album.name || 'Unknown Album')}

-
by ${escapeHtml(artist.name || 'Unknown Artist')}
+
by ${escapeHtml(artist.name || 'Unknown Artist')}
${album.album_type || 'Album'} ${trackCount} tracks diff --git a/webui/static/wishlist-tools.js b/webui/static/wishlist-tools.js index ee11148f..1a50bf19 100644 --- a/webui/static/wishlist-tools.js +++ b/webui/static/wishlist-tools.js @@ -12,6 +12,9 @@ let currentDiscoveryFix = { // Store event handler reference to allow proper removal let discoveryFixEnterHandler = null; +// Separate handler for the MBID-paste input — targets lookup-by-MBID +// instead of fuzzy search so Enter does the obvious right thing per field. +let discoveryFixMbidEnterHandler = null; /** * Open discovery fix modal for a specific track @@ -113,11 +116,20 @@ function openDiscoveryFixModal(platform, identifier, trackIndex) { artist: artistInput.value }); + // MBID input — separate ref because its Enter binding targets a + // different function (direct lookup vs fuzzy search). Optional: older + // modal markup that doesn't have the MBID row will get null here. + const mbidInput = fixModalOverlay.querySelector('#fix-modal-mbid-input'); + if (mbidInput) mbidInput.value = ''; + // Remove old enter key handler if exists if (discoveryFixEnterHandler) { trackInput.removeEventListener('keypress', discoveryFixEnterHandler); artistInput.removeEventListener('keypress', discoveryFixEnterHandler); } + if (discoveryFixMbidEnterHandler && mbidInput) { + mbidInput.removeEventListener('keypress', discoveryFixMbidEnterHandler); + } // Add new enter key handler discoveryFixEnterHandler = function (e) { @@ -126,6 +138,13 @@ function openDiscoveryFixModal(platform, identifier, trackIndex) { trackInput.addEventListener('keypress', discoveryFixEnterHandler); artistInput.addEventListener('keypress', discoveryFixEnterHandler); + if (mbidInput) { + discoveryFixMbidEnterHandler = function (e) { + if (e.key === 'Enter') lookupDiscoveryFixByMbid(); + }; + mbidInput.addEventListener('keypress', discoveryFixMbidEnterHandler); + } + // Show modal BEFORE auto-search so elements are visible fixModalOverlay.classList.remove('hidden'); console.log('✅ Fix modal opened, starting auto-search...'); @@ -195,12 +214,20 @@ async function searchDiscoveryFix() { } params.set('limit', '50'); - // Use the user's active metadata source first, then fall back to others + // Use the user's active metadata source first, then fall back to others. + // MusicBrainz is included so users on MB-as-primary get MB queried first, + // and so MB is available as a fallback for fuzzy / niche / non-mainstream + // recordings that Spotify / Deezer / iTunes miss (different catalogues, + // different cover coverage). MB sits last by default because it's + // rate-limited to 1 req/sec — when it's the active primary the activeIdx + // reorder below moves it to the front. Discogs is intentionally absent — + // Discogs has no track-level search API (releases only). const activeSource = (currentMusicSourceName || 'Spotify').toLowerCase(); const allSources = [ { key: 'spotify', endpoint: '/api/spotify/search_tracks', label: 'Spotify' }, { key: 'deezer', endpoint: '/api/deezer/search_tracks', label: 'Deezer' }, { key: 'itunes', endpoint: '/api/itunes/search_tracks', label: 'iTunes' }, + { key: 'musicbrainz', endpoint: '/api/musicbrainz/search_tracks', label: 'MusicBrainz' }, ]; // Put the active source first, keep others as fallbacks const activeIdx = allSources.findIndex(s => activeSource.includes(s.key)); @@ -238,6 +265,71 @@ async function searchDiscoveryFix() { } } +/** + * Look up a track directly by MusicBrainz recording MBID — bypasses fuzzy + * search entirely. Escape hatch for cases where the user knows the exact + * record (e.g. there are 10 same-title recordings from different sessions + * and auto-search keeps ranking the wrong one). Accepts full URLs + * (`https://musicbrainz.org/recording/`) or bare UUIDs. + */ +async function lookupDiscoveryFixByMbid() { + if (!currentDiscoveryFix.identifier) { + console.error('No active fix modal context'); + return; + } + + const discoveryModal = document.getElementById(`youtube-discovery-modal-${currentDiscoveryFix.identifier}`); + if (!discoveryModal) return; + const fixModalOverlay = discoveryModal.querySelector('.discovery-fix-modal-overlay'); + if (!fixModalOverlay) return; + + const mbidInput = fixModalOverlay.querySelector('#fix-modal-mbid-input'); + if (!mbidInput) return; + + const mbid = parseMusicBrainzMbid(mbidInput.value); + const resultsContainer = fixModalOverlay.querySelector('#fix-modal-results'); + + if (!mbid) { + if (resultsContainer) { + resultsContainer.innerHTML = '
❌ Not a valid MusicBrainz recording URL or MBID. Paste a URL like https://musicbrainz.org/recording/<uuid> or the bare UUID.
'; + } + showToast('Invalid MusicBrainz MBID', 'error'); + return; + } + + if (resultsContainer) { + resultsContainer.innerHTML = '
🔗 Looking up MusicBrainz recording...
'; + } + + try { + const response = await fetch(`/api/musicbrainz/recording/${encodeURIComponent(mbid)}`); + if (response.status === 404) { + if (resultsContainer) { + resultsContainer.innerHTML = '
Recording not found on MusicBrainz. Double-check the MBID.
'; + } + return; + } + if (!response.ok) { + throw new Error(`HTTP ${response.status}`); + } + const track = await response.json(); + if (!track || !track.id) { + if (resultsContainer) { + resultsContainer.innerHTML = '
Recording not found on MusicBrainz.
'; + } + return; + } + // Render as a single-result list — user still clicks to confirm, + // matching the existing search-result flow exactly. + renderDiscoveryFixResults([track], fixModalOverlay); + } catch (error) { + console.error('MBID lookup error:', error); + if (resultsContainer) { + resultsContainer.innerHTML = '
❌ MBID lookup failed. Try again.
'; + } + } +} + /** * Render search results as clickable cards */