diff --git a/.github/workflows/docker-publish.yml b/.github/workflows/docker-publish.yml index 70344823..34e38beb 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.6.8)' + description: 'Version tag (e.g. 2.6.9)' required: true - default: '2.6.8' + default: '2.6.9' jobs: build-and-push: diff --git a/config/settings.py b/config/settings.py index 8c2ab6db..c32c5b75 100644 --- a/config/settings.py +++ b/config/settings.py @@ -66,6 +66,12 @@ class ConfigManager: self._load_config() + # Placeholder shipped to the browser in place of a configured secret + # (#832 follow-up). The settings UI shows it as masked dots; if it's + # round-tripped back on save, ``set()`` treats it as "keep existing" so the + # real value is never overwritten by the mask. + REDACTED_SENTINEL = '__redacted_unchanged__' + # Dot-notation paths to sensitive config values that must be encrypted at rest. # Paths pointing to dicts encrypt the entire dict as a JSON blob. _SENSITIVE_PATHS = frozenset({ @@ -792,7 +798,40 @@ class ConfigManager: return value + def redacted_config(self) -> Dict[str, Any]: + """Deep copy of the live config with every sensitive value masked. + + Used for ``GET /api/settings`` so decrypted secrets never reach the + browser (#832 follow-up). A *set* secret becomes ``REDACTED_SENTINEL`` + (the UI renders it as masked dots); an unset one stays empty so the UI + can show "not configured". Dict-valued secrets (OAuth sessions) collapse + to the sentinel too — the UI has no field for them anyway. The matching + guard in ``set()`` turns a round-tripped sentinel back into a no-op. + """ + import copy + data = copy.deepcopy(self.config_data) + for path in self._SENSITIVE_PATHS: + keys = path.split('.') + parent = data + for k in keys[:-1]: + if isinstance(parent, dict) and k in parent: + parent = parent[k] + else: + parent = None + break + if not isinstance(parent, dict): + continue + leaf = keys[-1] + if leaf in parent and parent[leaf] not in (None, '', {}, [], 0, False): + parent[leaf] = self.REDACTED_SENTINEL + return data + def set(self, key: str, value: Any): + # The UI round-trips REDACTED_SENTINEL for any secret the user didn't + # touch — never let the mask overwrite the real value (#832 follow-up). + if value == self.REDACTED_SENTINEL and key in self._SENSITIVE_PATHS: + return + keys = key.split('.') config = self.config_data diff --git a/core/downloads/master.py b/core/downloads/master.py index 56c99d90..ce30b8d5 100644 --- a/core/downloads/master.py +++ b/core/downloads/master.py @@ -315,7 +315,52 @@ class _BatchStateAccessImpl: row['album_bundle_state'] = 'failed' -def run_full_missing_tracks_process(batch_id, playlist_id, tracks_json, deps: MasterDeps): +# Task states that mean a batch still has work in flight. While ANY of a batch's +# tasks is in one of these, a serialized album-pool worker keeps its slot. +_NON_TERMINAL_TASK_STATUSES = ('pending', 'queued', 'searching', 'downloading', 'post_processing') + + +def _wait_for_batch_drain(batch_id: str, poll_seconds: float = 1.5, + max_wait_seconds: float = 3600.0) -> None: + """Block until every task in ``batch_id`` reaches a terminal state (the batch + is fully drained), the batch is removed, shutdown is requested, or a safety + cap elapses. + + Used to make the dedicated album-bundle pool actually SERIALIZE albums: the + worker holds its pool slot for the album's whole lifetime instead of + returning the instant downloads are started. That stops every album from + dumping its tracks into the shared download pool at once (Sokhi: "searching + for way too many tracks at once"). It's a PASSIVE wait — the downloads are + driven by the monitor + completion callbacks on other threads, so this never + drives the work and can't deadlock; worst case the cap releases the slot and + the downloads simply finish in the background.""" + from core.downloads import monitor as _monitor + start = time.time() + while True: + if getattr(_monitor, 'IS_SHUTTING_DOWN', False): + return + with tasks_lock: + batch = download_batches.get(batch_id) + if not batch: + return + queue = list(batch.get('queue', ()) or ()) + still_working = any( + download_tasks.get(t, {}).get('status') in _NON_TERMINAL_TASK_STATUSES + for t in queue + ) + if not still_working: + return + if time.time() - start > max_wait_seconds: + logger.warning( + "[Album Serialize] batch %s not drained after %.0fs — releasing the " + "album-pool slot (its downloads continue in the background)", + batch_id, max_wait_seconds) + return + time.sleep(poll_seconds) + + +def run_full_missing_tracks_process(batch_id, playlist_id, tracks_json, deps: MasterDeps, + serialize: bool = False): """ A master worker that handles the entire missing tracks process: 1. Runs the analysis. @@ -1150,6 +1195,16 @@ def run_full_missing_tracks_process(batch_id, playlist_id, tracks_json, deps: Ma deps.download_monitor.start_monitoring(batch_id) deps.start_next_batch_of_downloads(batch_id) + # Album-bundle batches run on the dedicated album pool and pass + # serialize=True: hold this pool slot until the album finishes so only a + # few albums are ever in flight at once, instead of every album batch + # immediately starting and flooding the shared download pool with + # 'searching' tracks (#740 / Sokhi). The residual + playlist + manual + # paths run on the shared download pool and DON'T serialize (blocking + # there would steal an actual download worker). + if serialize: + _wait_for_batch_drain(batch_id) + except Exception as e: logger.error(f"Master worker for batch {batch_id} failed: {e}") import traceback diff --git a/core/downloads/track_link.py b/core/downloads/track_link.py new file mode 100644 index 00000000..e6dec8bd --- /dev/null +++ b/core/downloads/track_link.py @@ -0,0 +1,97 @@ +"""Recognize a pasted streaming-source track link in the manual download +search (#813). + +A user pastes e.g. ``https://tidal.com/track/434945950/u`` instead of typing a +query, to grab the exact version. We only recognize sources that download by +track ID (Tidal, Qobuz) — the manual search then resolves the link to that +track and runs the source's own search so the result is a normal, downloadable +candidate (no hand-built download encoding). + +Pure + import-safe: parsing only, no network. +""" + +from __future__ import annotations + +import re +from typing import Any, Optional, Tuple +from urllib.parse import urlparse + +# host substring → download source id. Only ID-downloadable streaming sources. +_HOSTS = ( + ('tidal.com', 'tidal'), + ('qobuz.com', 'qobuz'), +) + + +def parse_download_track_link(raw: str) -> Optional[Tuple[str, str]]: + """Parse a pasted Tidal/Qobuz track URL into ``(source, track_id)``. + + Returns None when the input isn't a recognized track link (so the caller + falls back to a normal text search). Handles the common URL shapes: + ``tidal.com/track/[/u]``, ``listen.tidal.com/track/``, + ``tidal.com/browse/track/``, ``open.qobuz.com/track/``, + ``play.qobuz.com/track/`` — with or without the scheme. + """ + raw = (raw or '').strip() + if not raw: + return None + + lowered = raw.lower() + if '://' not in raw and not any(h in lowered for h, _ in _HOSTS): + return None # not even a URL we care about + + url = raw if '://' in raw else f'https://{raw}' + parsed = urlparse(url) + host = (parsed.netloc or '').lower() + + source = next((sid for h, sid in _HOSTS if h in host), None) + if not source: + return None + + segs = [s for s in (parsed.path or '').split('/') if s] + for i, seg in enumerate(segs): + if seg.lower() == 'track' and i + 1 < len(segs): + m = re.match(r'(\d+)', segs[i + 1]) # id may carry a slug/suffix + if m: + return (source, m.group(1)) + return None + + +def _first_artist_name(value: Any) -> str: + """First artist name from a list of {'name': ...}/strings, or a single + {'name': ...}/string.""" + if isinstance(value, list): + value = value[0] if value else None + if isinstance(value, dict): + return str(value.get('name') or '') + return str(value or '') + + +def query_from_track_payload(source: str, raw: Any) -> Optional[str]: + """Build a clean ``"artist title"`` search query from a source ``get_track`` + payload — pure, so the per-source shape parsing is unit-testable without a + live client. + + - Tidal: attributes dict (``title`` + optional ``version`` + maybe + ``artists``/``artist``). The version is appended so a remix link searches + for the remix. + - Qobuz: track dict (``title`` + ``performer``/``album.artist``). + """ + if not isinstance(raw, dict): + return None + title = (raw.get('title') or '').strip() + artist = '' + + if source == 'tidal': + version = (raw.get('version') or '').strip() + if version and version.lower() not in title.lower(): + title = f"{title} ({version})" if title else version + artist = _first_artist_name(raw.get('artists') or raw.get('artist')) + elif source == 'qobuz': + artist = _first_artist_name(raw.get('performer')) + if not artist: + album = raw.get('album') if isinstance(raw.get('album'), dict) else {} + artist = _first_artist_name(album.get('artist')) + + query = f"{artist} {title}".strip() + return query or (title or None) diff --git a/core/imports/context.py b/core/imports/context.py index 73459949..f718f014 100644 --- a/core/imports/context.py +++ b/core/imports/context.py @@ -129,30 +129,36 @@ def get_import_search_result(context: Optional[Dict[str, Any]]) -> Dict[str, Any def get_import_source(context: Optional[Dict[str, Any]]) -> str: + # Several track payloads carry the metadata source under "_source" rather + # than "source" (the discography/wishlist dicts, frontend search results). + # Only the context-level "_source" was honored (normalize_import_context); + # the nested dicts were checked for "source" alone, so a Deezer-sourced + # Download Now resolved to '' and source-specific metadata logic (the + # Deezer contributors upgrade for multi-artist tags) never ran (Netti93). if not isinstance(context, dict): return "" - source = context.get("source") + source = context.get("source") or context.get("_source") if source: return str(source) track_info = get_import_track_info(context) - source = _first_value(track_info, "source", default="") + source = _first_value(track_info, "source", "_source", default="") if source: return str(source) original_search = get_import_original_search(context) - source = _first_value(original_search, "source", default="") + source = _first_value(original_search, "source", "_source", default="") if source: return str(source) album = get_import_context_album(context) - source = _first_value(album, "source", default="") + source = _first_value(album, "source", "_source", default="") if source: return str(source) artist = get_import_context_artist(context) - source = _first_value(artist, "source", default="") + source = _first_value(artist, "source", "_source", default="") return str(source) if source else "" diff --git a/core/imports/paths.py b/core/imports/paths.py index 5744744e..6546509f 100644 --- a/core/imports/paths.py +++ b/core/imports/paths.py @@ -587,6 +587,45 @@ def build_final_path_for_track(context, artist_context, album_info, file_ext, cr disc_label = _get_config_manager().get("file_organization.disc_label", "Disc") folder_path, filename_base = get_file_path_from_template(template_context, "album_path") + + # #829: if this album already lives in a single folder on disk, drop the + # new track there instead of a freshly-templated folder — this is what + # keeps an album from splitting when $albumtype/$year drift between + # batches (wishlist, Album Completeness, a missed track later). Strict + # match + transfer-dir-only + single-folder-only inside the resolver; + # any miss falls through to the template path below. Best-effort. + reuse_folder = None + if filename_base: + try: + from core.library.existing_album_folder import resolve_existing_album_folder + from database.music_database import get_database + try: + _active_server = _get_config_manager().get_active_media_server() + except Exception: + _active_server = None + _spotify_album_id = (album_context.get("id") + if album_context and str(source).startswith("spotify") else None) + _expected_tracks = None + if album_context and album_context.get("total_tracks"): + _expected_tracks = _coerce_int(album_context.get("total_tracks"), 0) or None + reuse_folder = resolve_existing_album_folder( + db=get_database(), + transfer_dir=transfer_dir, + album_name=album_info.get("album_name"), + album_artist=template_context.get("albumartist"), + spotify_album_id=_spotify_album_id, + active_server=_active_server, + expected_track_count=_expected_tracks, + config_manager=_get_config_manager(), + ) + except Exception as _reuse_err: + logger.debug("[Existing Album Folder] lookup failed: %s", _reuse_err) + reuse_folder = None + if reuse_folder and filename_base: + final_path = os.path.join(reuse_folder, filename_base + file_ext) + _ensure_dir(reuse_folder, exist_ok=True) + return final_path, True + if folder_path and filename_base: if total_discs > 1 and not user_controls_disc: disc_folder = f"{disc_label} {disc_number}" diff --git a/core/itunes_worker.py b/core/itunes_worker.py index 0d0e10a7..7e3e8a5d 100644 --- a/core/itunes_worker.py +++ b/core/itunes_worker.py @@ -734,6 +734,14 @@ class iTunesWorker: UPDATE albums SET year = ? WHERE id = ? AND (year IS NULL OR year = '' OR year = '0') """, (year, album_id)) + # #824: also store the FULL release date when iTunes has one + # (YYYY-MM or YYYY-MM-DD). Only when empty — never clobber a + # manually-set release_date. + if len(album_obj.release_date) > 4: + cursor.execute(""" + UPDATE albums SET release_date = ? + WHERE id = ? AND (release_date IS NULL OR release_date = '') + """, (album_obj.release_date, album_id)) # Cache the authoritative expected track count for the Album # Completeness repair job (see set_album_api_track_count docstring). diff --git a/core/jellyfin_client.py b/core/jellyfin_client.py index 4937c2c0..f24020d5 100644 --- a/core/jellyfin_client.py +++ b/core/jellyfin_client.py @@ -65,7 +65,19 @@ class JellyfinAlbum: self.title = jellyfin_data.get('Name', 'Unknown Album') self.addedAt = self._parse_date(jellyfin_data.get('DateCreated')) self._artist_id = jellyfin_data.get('AlbumArtists', [{}])[0].get('Id', '') if jellyfin_data.get('AlbumArtists') else '' - + # Album cover image, mirroring JellyfinArtist.thumb — so the library + # scan stores albums.thumb_url instead of leaving it empty. Without + # this, EVERY album reads back with no thumb, the web UI shows blank + # art, and the Cover Art Filler flags the entire library as "missing + # cover art" (it became the only thing populating the column). + self.thumb = self._get_album_image_url() + + def _get_album_image_url(self) -> Optional[str]: + """Jellyfin/Emby album primary image URL (same shape as the artist one).""" + if not self.ratingKey: + return None + return f"/Items/{self.ratingKey}/Images/Primary" + def _parse_date(self, date_str: Optional[str]) -> Optional[datetime]: if not date_str: return None @@ -73,7 +85,7 @@ class JellyfinAlbum: return datetime.fromisoformat(date_str.replace('Z', '+00:00')) except: return None - + def artist(self) -> Optional[JellyfinArtist]: """Get the album artist""" if self._artist_id: @@ -1554,20 +1566,40 @@ class JellyfinClient(MediaServerClient): return self.create_playlist(playlist_name, tracks) playlist_id = existing_playlist.id - existing_tracks = self.get_playlist_tracks(playlist_id) - existing_ids = { - str(t.id) for t in existing_tracks if hasattr(t, 'id') and t.id - } + # #823 round 2: the old dedupe read `t.id` off get_playlist_tracks() + # results — but JellyfinTrack only defines `ratingKey`, so the + # existing-ids set was ALWAYS empty and every sync re-appended the + # whole matched list ("added 22 ... skipped 0" on a playlist that + # already had them, every track N times). Fetch the playlist's items + # from the canonical /Playlists/{id}/Items endpoint (the same one + # reconcile uses — works on Jellyfin GUIDs and Emby numeric ids) and + # dedupe on the raw item Id; fall back to ratingKey if that fails. + existing_ids = set() + items_resp = self._make_request( + f'/Playlists/{playlist_id}/Items', {'UserId': self.user_id}) + if items_resp: + for item in items_resp.get('Items', []): + iid = str(item.get('Id') or '') + if iid: + existing_ids.add(iid) + else: + existing_ids = { + str(getattr(t, 'ratingKey', '') or '') + for t in self.get_playlist_tracks(playlist_id) + } - {''} - new_track_ids = [] + desired_ids = [] for t in tracks: tid = None if hasattr(t, 'id'): tid = str(t.id) if t.id else None elif isinstance(t, dict): tid = str(t.get('Id') or t.get('id') or '') - if tid and tid not in existing_ids and self._is_valid_guid(tid): - new_track_ids.append(tid) + if tid and self._is_valid_guid(tid): + desired_ids.append(tid) + + from core.sync.playlist_edit import plan_playlist_append + new_track_ids = plan_playlist_append(existing_ids, desired_ids) if not new_track_ids: logger.info( diff --git a/core/library/existing_album_folder.py b/core/library/existing_album_folder.py new file mode 100644 index 00000000..926ab173 --- /dev/null +++ b/core/library/existing_album_folder.py @@ -0,0 +1,129 @@ +"""Reuse an album's existing on-disk folder for new downloads (#829). + +When tracks are added to an album across multiple batches (a wishlist run, the +Album Completeness job, a missed track re-downloaded later), the destination +folder is normally rebuilt from API metadata each time. If ``$albumtype`` or +``$year`` come back blank/different on a later batch, the folder *name* changes +and the album splits across folders — forcing a Reorganize afterwards. + +This resolves the folder the album *already* lives in so the new track joins its +existing files instead. Matching is deliberately conservative: the exact stored +Spotify album id first (definitive), then a STRICT (>= 0.85) name+artist match — +higher than the 0.7 used elsewhere, because a wrong match here misplaces a file. + +Safety rails: + * Only ever returns a folder UNDER the transfer dir (the managed download + tree) — never a read-only library/NAS mount the resolver happens to find. + * Only reuses when the album lives in EXACTLY ONE folder on disk. Multiple + folders means disc subfolders (DatabaseTrack carries no disc number, so we + can't safely pick the right one) — those defer to the template path. + * Any failure returns None — the caller falls back to the normal template. +""" + +from __future__ import annotations + +import os +from typing import Any, Optional + +from core.library.path_resolver import resolve_library_file_path +from utils.logging_config import get_logger + +logger = get_logger("library.existing_album_folder") + +# Strict — a wrong album match drops the file in the wrong folder. +_STRICT_ALBUM_CONFIDENCE = 0.85 + + +def _is_under(child: str, parent: str) -> bool: + """True if ``child`` is the same as or inside ``parent`` (normalized).""" + try: + child_n = os.path.normcase(os.path.normpath(os.path.abspath(child))) + parent_n = os.path.normcase(os.path.normpath(os.path.abspath(parent))) + return child_n == parent_n or child_n.startswith(parent_n + os.sep) + except Exception: + return False + + +def _find_album(db: Any, spotify_album_id: Optional[str], album_name: Optional[str], + album_artist: Optional[str], active_server: Optional[str], + expected_track_count: Optional[int]): + """Stored Spotify id first, then a strict name+artist match. None on no match.""" + if spotify_album_id: + try: + album = db.get_album_by_spotify_album_id(spotify_album_id) + if album: + return album + except Exception as e: + logger.debug("album-by-spotify-id lookup failed: %s", e) + if album_name and album_artist: + try: + match, confidence = db.check_album_exists_with_editions( + title=album_name, artist=album_artist, + confidence_threshold=_STRICT_ALBUM_CONFIDENCE, + expected_track_count=expected_track_count, + server_source=active_server, + ) + if match and confidence >= _STRICT_ALBUM_CONFIDENCE: + return match + except Exception as e: + logger.debug("strict album name+artist match failed: %s", e) + return None + + +def resolve_existing_album_folder( + *, + db: Any, + transfer_dir: Optional[str], + album_name: Optional[str] = None, + album_artist: Optional[str] = None, + spotify_album_id: Optional[str] = None, + active_server: Optional[str] = None, + expected_track_count: Optional[int] = None, + config_manager: Any = None, + resolver=resolve_library_file_path, +) -> Optional[str]: + """Return the on-disk folder an existing album lives in (so a new track joins + it) or None to fall back to the templated path. See module docstring.""" + if not transfer_dir or not os.path.isdir(transfer_dir): + return None + if not db: + return None + + album = _find_album(db, spotify_album_id, album_name, album_artist, + active_server, expected_track_count) + if not album: + return None + + try: + tracks = db.get_tracks_by_album(album.id) + except Exception as e: + logger.debug("get_tracks_by_album(%s) failed: %s", getattr(album, 'id', '?'), e) + return None + + folders = set() + for t in tracks: + file_path = getattr(t, 'file_path', None) + if not file_path: + continue + try: + resolved = resolver(file_path, transfer_folder=transfer_dir, + config_manager=config_manager) + except Exception: + resolved = None + if not resolved: + continue + folder = os.path.dirname(resolved) + if _is_under(folder, transfer_dir): + folders.add(os.path.normpath(folder)) + + # Single folder under the transfer dir → reuse it. Zero (nothing on disk yet) + # or many (disc subfolders) → let the template decide. + if len(folders) == 1: + reuse = next(iter(folders)) + logger.info("[Existing Album Folder] Reusing '%s' for album '%s'", + reuse, getattr(album, 'title', album_name)) + return reuse + return None + + +__all__ = ["resolve_existing_album_folder"] diff --git a/core/library/path_resolve.py b/core/library/path_resolve.py new file mode 100644 index 00000000..341692e3 --- /dev/null +++ b/core/library/path_resolve.py @@ -0,0 +1,86 @@ +"""Confusable-tolerant filesystem path resolution (#833). + +the-hang-man: a track titled "I'm Upset" was written to disk with an ASCII +apostrophe (U+0027) but the library DB stored the title with a typographic one +(U+2019, the form Spotify/Apple metadata uses). Deleting rebuilt the unlink +target from the DB path, so ``os.path.exists`` compared U+2019-bytes against a +U+0027 filename — always a miss — and the file survived ("could not be deleted"). + +The same byte-exact mismatch hits any on-disk operation that starts from stored +metadata (delete, sidecar cleanup, dead-file checks). The fix is to resolve the +*real* on-disk name: descend the path component by component, taking an exact +match when present and otherwise folding a small set of typographic confusables +(curly vs straight quotes, en/em dash, ellipsis, nbsp) for the comparison ONLY. +We never rename — we just find the file that's actually there. + +Case is deliberately preserved: on a case-sensitive dataset (ext4/ZFS) two +tracks can differ only by case, so folding case could delete the wrong file. +The reported failure is purely typographic, so that's all we fold. +""" + +from __future__ import annotations + +import os +import unicodedata + +# Typographic characters that routinely differ between DB metadata (Unicode, +# from streaming-service catalogs) and the ASCII filename on disk. Folded to a +# common form for COMPARISON only. +_CONFUSABLES = { + '‘': "'", '’': "'", 'ʼ': "'", '′': "'", # ‘ ’ ʼ ′ → ' + '“': '"', '”': '"', '″': '"', # “ ” ″ → " + '–': '-', '—': '-', '‒': '-', '―': '-', # – — ‒ ― → - + '…': '...', # … → ... + ' ': ' ', # nbsp → space +} + + +def fold_confusables(name: str) -> str: + """Fold typographic confusables + NFC-normalize so a DB name and the real + on-disk name compare equal despite curly-vs-straight quotes, dashes, etc. + Case and everything else are left untouched.""" + if not name: + return '' + name = unicodedata.normalize('NFC', name) + for bad, good in _CONFUSABLES.items(): + if bad in name: + name = name.replace(bad, good) + return name + + +def find_on_disk(base_dir: str, suffix_parts): + """Descend ``base_dir`` following ``suffix_parts`` (the path components of a + stored file path). Each component is matched exactly when it exists, else by + confusable-folded comparison against the directory's real entries. Returns + the real absolute path, or None if any component can't be resolved. + + Exact matches always win — the folded scan only runs for a component that + isn't present byte-for-byte, so this never changes behaviour for paths that + already resolve. + """ + if not base_dir or not os.path.isdir(base_dir): + return None + current = base_dir + for part in suffix_parts: + if not part: + continue + exact = os.path.join(current, part) + if os.path.exists(exact): + current = exact + continue + target = fold_confusables(part) + match = None + try: + for entry in os.listdir(current): + if fold_confusables(entry) == target: + match = os.path.join(current, entry) + break + except OSError: + return None + if match is None: + return None + current = match + return current if current != base_dir else None + + +__all__ = ['fold_confusables', 'find_on_disk'] diff --git a/core/matching_engine.py b/core/matching_engine.py index 32113bcc..15f91f02 100644 --- a/core/matching_engine.py +++ b/core/matching_engine.py @@ -214,6 +214,22 @@ class MusicMatchingEngine: # Standard similarity standard_ratio = SequenceMatcher(None, str1, str2).ratio() + # Version vocabulary, shared by the prefix check and the divergent + # check below. + remaster_keywords = ['remaster', 'remastered'] + different_version_keywords = [ + 'remix', 'mix', 'rmx', # Remixes (different song) + 'live', 'live at', 'live from', # Live versions (different recording) + 'acoustic', 'unplugged', # Acoustic versions (different arrangement) + 'slowed', 'reverb', 'sped up', 'speed up', # TikTok edits (different) + 'radio edit', 'radio version', # Radio edits (different cut) + 'single edit', # Single edits (different cut) + 'album edit', # Album edits (different cut) + 'instrumental', 'karaoke', # Instrumental (different) + 'extended', 'extended version', # Extended (different length) + 'demo', 'rough cut', # Demos (different recording) + ] + # STRICT VERSION CHECKING: Different versions should score LOW # This prevents "Song Title" from matching "Song Title (Remix)" during sync shorter, longer = (str1, str2) if len(str1) <= len(str2) else (str2, str1) @@ -223,23 +239,6 @@ class MusicMatchingEngine: # Extract the extra content extra_content = longer[len(shorter):].strip() - # Check if the extra content looks like version info - # Separate remasters from other versions - they should be treated differently - remaster_keywords = ['remaster', 'remastered'] - - different_version_keywords = [ - 'remix', 'mix', 'rmx', # Remixes (different song) - 'live', 'live at', 'live from', # Live versions (different recording) - 'acoustic', 'unplugged', # Acoustic versions (different arrangement) - 'slowed', 'reverb', 'sped up', 'speed up', # TikTok edits (different) - 'radio edit', 'radio version', # Radio edits (different cut) - 'single edit', # Single edits (different cut) - 'album edit', # Album edits (different cut) - 'instrumental', 'karaoke', # Instrumental (different) - 'extended', 'extended version', # Extended (different length) - 'demo', 'rough cut', # Demos (different recording) - ] - # Normalize extra content for comparison extra_normalized = extra_content.lower().strip(' -()[]') @@ -261,6 +260,38 @@ class MusicMatchingEngine: logger.debug(f"Version mismatch detected: '{str1}' vs '{str2}' (keyword: '{keyword}') - applying heavy penalty") return 0.30 + # STRICT VERSION CHECKING (divergent case): two DIFFERENT versions of + # the same base — e.g. "Song (Shazam Remix)" vs "Song (southstar + # Remix)", or "...live at pukkelpop" vs "...live at wembley". Both + # carry a version descriptor, so neither is a prefix of the other and + # the prefix check above misses them; the raw ratio then stays high off + # the shared base. Without this, when the requested version is absent a + # different cut of the same song can outscore the threshold and get + # downloaded. A correct same-version match is identical after + # normalisation and already returned 1.0 above, so a both-versioned + # pair that survives to here with high base overlap is a genuinely + # different cut. (Remasters are intentionally excluded — the prefix + # branch gives them the lenient 0.75 so re-mastered cuts still match.) + def _versions_in(s: str) -> frozenset: + return frozenset( + kw for kw in different_version_keywords + if re.search(r'\b' + re.escape(kw) + r'\b', s)) + + v1, v2 = _versions_in(str1), _versions_in(str2) + if v1 and v2 and standard_ratio >= 0.5: + # Strip the version words; what remains is base + distinguishing + # descriptor (remixer / performance / year). + def _strip_versions(s: str) -> str: + for kw in different_version_keywords: + s = re.sub(r'\b' + re.escape(kw) + r'\b', ' ', s) + return re.sub(r'\s+', ' ', s).strip() + + if v1 != v2 or _strip_versions(str1) != _strip_versions(str2): + logger.debug( + f"Divergent version detected: '{str1}' vs '{str2}' " + f"- applying heavy penalty") + return 0.30 + return standard_ratio def duration_similarity(self, duration1: int, duration2: int) -> float: diff --git a/core/metadata/art_apply.py b/core/metadata/art_apply.py index 448ff0f2..d1ff6639 100644 --- a/core/metadata/art_apply.py +++ b/core/metadata/art_apply.py @@ -17,7 +17,7 @@ from __future__ import annotations import contextlib import errno import os -from typing import Iterable +from typing import Iterable, Optional from core.metadata.artwork import download_cover_art, embed_album_art_metadata from core.metadata.common import get_mutagen_symbols @@ -81,6 +81,38 @@ def _audio_has_art(audio, symbols) -> bool: return False +def extract_embedded_art(file_path: str) -> Optional[bytes]: + """Return the first embedded cover-art image bytes from an audio file, or + None. Used to write a cover.jpg sidecar from the album's OWN art — no API + call, and the sidecar matches what's embedded (#813/Sokhi).""" + if not file_path or not os.path.isfile(file_path): + return None + symbols = get_mutagen_symbols() + if not symbols: + return None + try: + audio = symbols.File(file_path) + if audio is None: + return None + pics = getattr(audio, "pictures", None) # FLAC / Ogg + if pics: + return bytes(pics[0].data) + if isinstance(audio, symbols.MP4): + covr = audio.get("covr") + if covr: + return bytes(covr[0]) + tags = getattr(audio, "tags", None) + if tags is not None: + with contextlib.suppress(Exception): + if isinstance(tags, symbols.ID3): + apics = tags.getall("APIC") + if apics: + return bytes(apics[0].data) + except Exception as exc: + logger.debug("embedded-art extract failed for %s: %s", file_path, exc) + return None + + def album_has_art_on_disk(rep_file_path: str) -> bool: """Does this album have art on disk? @@ -166,13 +198,49 @@ def apply_art_to_album_files( logger.warning("Could not embed art into %s: %s", fp, exc) result["failed"] += 1 - target_dir = folder or (os.path.dirname(paths[0]) if paths else None) - if target_dir and os.path.isdir(target_dir): - try: - download_cover_art(album_info, target_dir, context) - result["cover_written"] = folder_has_cover_sidecar(target_dir) - except Exception as exc: - if getattr(exc, "errno", None) == errno.EROFS: + # Prefer the caller's folder, but if it doesn't actually exist (e.g. a raw + # DB path that isn't mounted in this container), fall back to the real + # directory of the files we just wrote to — never silently skip the sidecar + # because a passed-in folder was wrong (Sokhi: cover.jpg never written). + target_dir = folder if (folder and os.path.isdir(folder)) else None + if not target_dir and paths: + cand = os.path.dirname(paths[0]) + target_dir = cand if os.path.isdir(cand) else None + if target_dir and not folder_has_cover_sidecar(target_dir): + # Prefer the album's OWN embedded art for the cover.jpg sidecar: it's + # always present once the files are arted (we may have just embedded it), + # needs no API call, and the sidecar matches the files exactly + # (#813/Sokhi: files have art, just no cover.jpg). Fall back to a fresh + # download only when there's nothing embedded to extract. + cover_path = os.path.join(target_dir, "cover.jpg") + art_bytes = None + for fp in paths: + art_bytes = extract_embedded_art(fp) + if art_bytes: + break + if art_bytes: + try: + with open(cover_path, "wb") as handle: + handle.write(art_bytes) + result["cover_written"] = True + except OSError as exc: + if getattr(exc, "errno", None) == errno.EROFS: + result["read_only_fs"] = True + logger.warning("cover.jpg sidecar write failed for %s: %s", target_dir, exc) + + if not result["cover_written"] and not result["read_only_fs"]: + # No embedded art to extract → fetch it. download_cover_art swallows + # its own write errors, so it records read-only on the context dict + # (EROFS detection gap, Sokhi). force=True bypasses the import-time + # "Download cover.jpg" toggle — running the filler is an explicit ask. + cover_ctx = context if isinstance(context, dict) else {} + try: + download_cover_art(album_info, target_dir, cover_ctx, force=True) + result["cover_written"] = folder_has_cover_sidecar(target_dir) + except Exception as exc: + if getattr(exc, "errno", None) == errno.EROFS: + result["read_only_fs"] = True + logger.warning("cover.jpg write failed for %s: %s", target_dir, exc) + if cover_ctx.get("_cover_read_only"): result["read_only_fs"] = True - logger.warning("cover.jpg write failed for %s: %s", target_dir, exc) return result diff --git a/core/metadata/artwork.py b/core/metadata/artwork.py index 3698a064..758fe206 100644 --- a/core/metadata/artwork.py +++ b/core/metadata/artwork.py @@ -2,6 +2,7 @@ from __future__ import annotations +import errno import os import re import time @@ -469,9 +470,17 @@ def embed_album_art_metadata(audio_file, metadata: dict): return False -def download_cover_art(album_info: dict, target_dir: str, context: dict = None): +def download_cover_art(album_info: dict, target_dir: str, context: dict = None, force: bool = False): + """Write cover.jpg into ``target_dir``. + + ``force`` bypasses the import-time "Download cover.jpg to album folder" + toggle — used by the Cover Art Filler, whose whole job is to add cover art + (if you explicitly run the filler you want the sidecar regardless of the + auto-import preference). The import pipeline calls this WITHOUT force, so it + still honors the user's setting. + """ cfg = get_config_manager() - if cfg.get("metadata_enhancement.cover_art_download", True) is False: + if not force and cfg.get("metadata_enhancement.cover_art_download", True) is False: return try: @@ -566,4 +575,14 @@ def download_cover_art(album_info: dict, target_dir: str, context: dict = None): handle.write(image_data) logger.info("Cover art downloaded to: %s", cover_path) except Exception as exc: - logger.error("Error downloading cover.jpg: %s", exc) + # A read-only mount (EROFS) is a "can't write" condition the caller + # needs to surface (cover-art filler #804/Tim/Sokhi) — but we must NOT + # re-raise (import callers aren't wrapped here). Record it on the + # context so callers that care can detect it, instead of just spamming + # the log with a swallowed error. + if getattr(exc, "errno", None) == errno.EROFS: + if isinstance(context, dict): + context["_cover_read_only"] = True + logger.warning("cover.jpg write blocked — read-only filesystem: %s", cover_path) + else: + logger.error("Error downloading cover.jpg: %s", exc) diff --git a/core/metadata/common.py b/core/metadata/common.py index ebf6ee5d..407de668 100644 --- a/core/metadata/common.py +++ b/core/metadata/common.py @@ -3,6 +3,7 @@ from __future__ import annotations import os +import shutil import threading import weakref from types import SimpleNamespace @@ -142,13 +143,63 @@ def is_vorbis_like(audio_file: Any, symbols: Any) -> bool: return bool(vorbis_classes) and isinstance(audio_file, vorbis_classes) or is_ogg_opus(audio_file) -def save_audio_file(audio_file: Any, symbols: Any) -> None: +def _raw_audio_save(audio_file: Any, symbols: Any, target: Any = None) -> None: + """The plain mutagen save with the format-specific kwargs. ``target`` None → + save in place (the exact call used before #819, byte-for-byte unchanged); a + path → save into that file (the atomic temp copy).""" if isinstance(audio_file.tags, symbols.ID3): - audio_file.save(v1=0, v2_version=4) + audio_file.save(v1=0, v2_version=4) if target is None else audio_file.save(target, v1=0, v2_version=4) elif isinstance(audio_file, symbols.FLAC): - audio_file.save(deleteid3=True) + audio_file.save(deleteid3=True) if target is None else audio_file.save(target, deleteid3=True) else: - audio_file.save() + audio_file.save() if target is None else audio_file.save(target) + + +def save_audio_file(audio_file: Any, symbols: Any) -> None: + """Persist mutagen tag changes ATOMICALLY where possible (#819). + + mutagen's in-place ``save()`` rewrites the file; if the process is + interrupted or OOM-killed mid-write, the file is left truncated — audio AND + tags gone (CubeComming's large hi-res FLACs imported to an empty shell). + Instead: copy the original to a temp in the same directory, write the + modified tags into that copy, verify it's still a valid audio file, then + ``os.replace`` it in atomically. The original is never touched until that + final swap, so a crash can only ever orphan the temp — never destroy the + user's file. + + Falls back to the plain in-place save if the atomic path can't run (no + filename, copy fails, or a format mutagen can't save-to-path) so the file + is never left worse off than it is today. + """ + path = getattr(audio_file, "filename", None) + try: + path = os.fspath(path) if path else None + except TypeError: + path = None + if not path or not os.path.isfile(path): + _raw_audio_save(audio_file, symbols) + return + + tmp = f"{path}.sstmp" + try: + shutil.copy2(path, tmp) # snapshot original (audio + tags) + _raw_audio_save(audio_file, symbols, target=tmp) # write new tags into the copy + check = symbols.File(tmp) # verify it's still real audio + if check is None or getattr(getattr(check, "info", None), "length", 0) <= 0: + raise ValueError("atomic save produced a file with no audio") + os.replace(tmp, path) # atomic swap — original safe until here + except Exception as atomic_err: + # Original untouched (only tmp was written). Clean up + fall back to the + # original in-place save so any format/edge the atomic path can't handle + # behaves exactly as before. + try: + if os.path.exists(tmp): + os.remove(tmp) + except OSError: + pass + logger.warning("[Atomic Save] atomic path failed (%s) — in-place fallback for %s", + atomic_err, os.path.basename(path)) + _raw_audio_save(audio_file, symbols) def get_image_dimensions(data: bytes): diff --git a/core/metadata/discography_filters.py b/core/metadata/discography_filters.py index 4e1e0ecd..c5985fdf 100644 --- a/core/metadata/discography_filters.py +++ b/core/metadata/discography_filters.py @@ -28,8 +28,31 @@ single source of truth. from __future__ import annotations +import re from typing import Any, Dict, List, Optional +# Split a combined artist credit into its individual artists. Sources like +# iTunes return collabs as ONE string ("TRVNSPORTER, Narvent & SKVLENT"), not a +# list — so an exact full-string compare drops every collaborator's discography +# entry (#830). Split on the common credit separators; " and " / " with " are +# deliberately excluded (too many real band names contain them). +_ARTIST_CREDIT_SPLIT_RE = re.compile( + r"\s*[,&;/]\s*|\s+(?:feat\.?|ft\.?|featuring|vs\.?|x)\s+", + re.IGNORECASE, +) + + +def _artist_credit_components(name: str) -> List[str]: + """Return the individual artist names within a (possibly combined) credit, + always including the full string itself (so exact band names with internal + separators still match).""" + name = (name or "").strip() + if not name: + return [] + parts = [name] + parts.extend(p.strip() for p in _ARTIST_CREDIT_SPLIT_RE.split(name) if p.strip()) + return parts + from core.watchlist_scanner import ( is_acoustic_version, is_instrumental_version, @@ -47,10 +70,12 @@ def track_artist_matches(track_artists: Any, requested_artist_name: str) -> bool what the discography fetch returns), or the list-of-dicts shape that some upstreams pass directly. Both are accepted. - Returns True for primary-artist tracks AND feature appearances — - the requested artist need only be one of the listed artists. Only - drops tracks where the requested artist isn't named at all (the - cross-artist compilation case from #559). + Returns True for primary-artist tracks AND feature/collab appearances — + the requested artist need only be one of the credited artists, INCLUDING + when a source (iTunes, etc.) packs the collab into one combined string like + "TRVNSPORTER, Narvent & SKVLENT" (#830). Only drops tracks where the + requested artist isn't credited at all (the cross-artist compilation case + from #559). """ if not requested_artist_name: # No artist to compare against — don't filter; let the caller @@ -70,8 +95,13 @@ def track_artist_matches(track_artists: Any, requested_artist_name: str) -> bool name = entry.get('name', '') or '' else: name = str(entry or '') - if name.strip().lower() == target: - return True + # Match the requested artist as a component of the credit, so combined + # collab strings ("A, B & C") keep B's discography entry. Component + # matching is still exact per-name, so true contamination (the artist + # genuinely absent) is dropped exactly as before. + for component in _artist_credit_components(name): + if component.strip().lower() == target: + return True return False diff --git a/core/navidrome_client.py b/core/navidrome_client.py index 085a4257..5a0677ee 100644 --- a/core/navidrome_client.py +++ b/core/navidrome_client.py @@ -1102,12 +1102,16 @@ class NavidromeClient(MediaServerClient): return self.create_playlist(playlist_name, tracks) primary = existing_playlists[0] - existing_tracks = self.get_playlist_tracks(primary.id) + # #823 round 2: the old dedupe read `t.id` — but NavidromeTrack only + # defines `ratingKey`, so the existing-ids set was ALWAYS empty and + # every sync re-appended the whole matched list (every track N + # times). Same bug as the Jellyfin append; dedupe on ratingKey. existing_ids = { - str(t.id) for t in existing_tracks if hasattr(t, 'id') and t.id - } + str(getattr(t, 'ratingKey', '') or '') + for t in self.get_playlist_tracks(primary.id) + } - {''} - new_track_ids = [] + desired_ids = [] for t in tracks: tid = None if hasattr(t, 'ratingKey'): @@ -1116,8 +1120,11 @@ class NavidromeClient(MediaServerClient): tid = str(t.id) if t.id else None elif isinstance(t, dict): tid = str(t.get('id') or '') - if tid and tid not in existing_ids: - new_track_ids.append(tid) + if tid: + desired_ids.append(tid) + + from core.sync.playlist_edit import plan_playlist_append + new_track_ids = plan_playlist_append(existing_ids, desired_ids) if not new_track_ids: logger.info( diff --git a/core/repair_jobs/dead_file_cleaner.py b/core/repair_jobs/dead_file_cleaner.py index f0b3fb9d..ac9a0263 100644 --- a/core/repair_jobs/dead_file_cleaner.py +++ b/core/repair_jobs/dead_file_cleaner.py @@ -36,7 +36,17 @@ class DeadFileCleanerJob(RepairJob): icon = 'repair-icon-deadfile' default_enabled = True default_interval_hours = 24 - default_settings = {} + default_settings = { + # Mass-false-positive guard: if at least this fraction of tracks resolve + # to no file on disk, treat it as a path-mapping/mount problem (SoulSync + # can't SEE the library — e.g. Docker, or library.music_paths unset for + # this environment) rather than thousands of individually-deleted files, + # and abort without creating findings. Mirrors the transfer-folder abort. + 'max_unresolved_fraction': 0.5, + # ...but only once the library is at least this big — a small library can + # legitimately have a high dead fraction. + 'min_tracks_for_guard': 25, + } auto_fix = False def scan(self, context: JobContext) -> JobResult: @@ -87,9 +97,30 @@ class DeadFileCleanerJob(RepairJob): if context.config_manager: download_folder = context.config_manager.get('soulseek.download_path', '') + # Mass-false-positive guard thresholds (see default_settings). + max_unresolved_fraction = 0.5 + min_tracks_for_guard = 25 + if context.config_manager: + try: + max_unresolved_fraction = float(context.config_manager.get( + self.get_config_key('max_unresolved_fraction'), 0.5)) + except (TypeError, ValueError): + max_unresolved_fraction = 0.5 + try: + min_tracks_for_guard = int(context.config_manager.get( + self.get_config_key('min_tracks_for_guard'), 25)) + except (TypeError, ValueError): + min_tracks_for_guard = 25 + if context.report_progress: context.report_progress(phase=f'Checking {total} tracks...', total=total) + # Collect unresolvable tracks first; decide whether they're genuine dead + # files or a systemic path problem AFTER the full pass (below). A "None" + # from the resolver means "couldn't find it at any known base dir" — which + # for a mis-mounted library is EVERY track, not a real deletion. + dead_rows = [] + for i, row in enumerate(tracks): if context.check_stop(): return result @@ -112,44 +143,75 @@ class DeadFileCleanerJob(RepairJob): config_manager=context.config_manager) if resolved is None: - # File is truly missing — create finding - if context.report_progress: - context.report_progress( - log_line=f'Missing: {title or "Unknown"} — {os.path.basename(file_path)}', - log_type='error' - ) - if context.create_finding: - try: - inserted = context.create_finding( - job_id=self.job_id, - finding_type='dead_file', - severity='warning', - entity_type='track', - entity_id=str(track_id), - file_path=file_path, - title=f'Missing file: {title or "Unknown"}', - description=f'Track "{title}" by {artist_name or "Unknown"} points to a file that no longer exists', - details={ - 'track_id': track_id, - 'title': title, - 'artist': artist_name, - 'album': album_title, - 'original_path': file_path, - 'album_thumb_url': album_thumb or None, - 'artist_thumb_url': artist_thumb or None, - } - ) - if inserted: - result.findings_created += 1 - else: - result.findings_skipped_dedup += 1 - except Exception as e: - logger.debug("Error creating dead file finding for track %s: %s", track_id, e) - result.errors += 1 + dead_rows.append(row) if context.update_progress and (i + 1) % 100 == 0: context.update_progress(i + 1, total) + # Mass-false-positive guard: a large fraction of unresolvable paths almost + # always means SoulSync can't SEE the library (Docker mount, or + # Settings → Library → Music Paths not set for this environment), NOT that + # thousands of files were individually deleted. Refuse to flag and say so + # — same principle as the transfer-folder abort above. (#828: a Plex-on- + # macOS user in Docker had all 5,250 tracks flagged because their stored + # /Volumes/... paths don't exist inside the container.) + if (dead_rows + and result.scanned >= min_tracks_for_guard + and len(dead_rows) >= result.scanned * max_unresolved_fraction): + logger.error( + "Dead file scan: %d/%d tracks unresolvable (>= %.0f%%) — aborting without " + "creating findings; this is a path-mapping/mount problem, not deleted files.", + len(dead_rows), result.scanned, max_unresolved_fraction * 100) + result.errors += 1 + if context.report_progress: + context.report_progress( + phase='Aborted — too many unreachable paths', + log_line=(f"{len(dead_rows)} of {result.scanned} tracks point to paths SoulSync " + f"can't reach — almost always a path-mapping issue (Docker mount, or " + f"Settings → Library → Music Paths), not deleted files. No findings created."), + log_type='error' + ) + if context.update_progress: + context.update_progress(total, total) + return result + + # A small fraction unresolvable — treat as genuine dead files and report. + for row in dead_rows: + track_id, title, artist_name, album_title, file_path, album_thumb, artist_thumb = row + if context.report_progress: + context.report_progress( + log_line=f'Missing: {title or "Unknown"} — {os.path.basename(file_path)}', + log_type='error' + ) + if context.create_finding: + try: + inserted = context.create_finding( + job_id=self.job_id, + finding_type='dead_file', + severity='warning', + entity_type='track', + entity_id=str(track_id), + file_path=file_path, + title=f'Missing file: {title or "Unknown"}', + description=f'Track "{title}" by {artist_name or "Unknown"} points to a file that no longer exists', + details={ + 'track_id': track_id, + 'title': title, + 'artist': artist_name, + 'album': album_title, + 'original_path': file_path, + 'album_thumb_url': album_thumb or None, + 'artist_thumb_url': artist_thumb or None, + } + ) + if inserted: + result.findings_created += 1 + else: + result.findings_skipped_dedup += 1 + except Exception as e: + logger.debug("Error creating dead file finding for track %s: %s", track_id, e) + result.errors += 1 + if context.update_progress: context.update_progress(total, total) diff --git a/core/repair_jobs/missing_cover_art.py b/core/repair_jobs/missing_cover_art.py index eb3a8bf1..8e1e39e6 100644 --- a/core/repair_jobs/missing_cover_art.py +++ b/core/repair_jobs/missing_cover_art.py @@ -3,7 +3,8 @@ import os import re -from core.metadata.art_apply import album_has_art_on_disk +from core.metadata.art_apply import file_has_embedded_art, folder_has_cover_sidecar +from core.library.path_resolver import resolve_library_file_path from core.metadata_service import get_client_for_source, get_primary_source, get_source_priority from core.repair_jobs import register_job from core.repair_jobs.base import JobContext, JobResult, RepairJob @@ -137,6 +138,17 @@ class MissingCoverArtJob(RepairJob): context.update_progress(0, total) logger.info("Found %d albums missing cover art", total) + download_folder = (context.config_manager.get('soulseek.download_path', '') + if context.config_manager else None) + # Skip-reason breakdown so a "0 findings" scan tells us WHY each album + # was skipped instead of guessing (Sokhi cover.jpg saga). Logged in the + # summary line below. + skip_reasons = { + 'have_disk_art': 0, # local file has embedded art + sidecar (or sidecar not wanted) + 'no_local_db_has_art': 0, # file path didn't resolve; DB already has a thumb + 'no_art_source': 0, # needs fix, but no API art found and nothing embedded to extract + } + _diag_logged = 0 if context.report_progress: context.report_progress(phase=f'Searching artwork for {total} albums...', total=total) @@ -160,8 +172,73 @@ class MissingCoverArtJob(RepairJob): # Art can be missing in the DB (no thumb_url) and/or on disk (no # embedded art and no cover.jpg). Skip albums that already have both. db_missing = not (str(album_thumb).strip() if album_thumb else '') - disk_missing = bool(rep_path) and not album_has_art_on_disk(rep_path) - if not db_missing and not disk_missing: + # Resolve the representative path the SAME way the apply does + # (_fix_missing_cover_art) before checking disk art. Checking the raw + # DB path would fail on any path-mapped setup (docker mounts, a + # Plex/SoulSync path mismatch) — the file isn't found, album art + # reads as "missing", and EVERY album gets flagged while the apply + # (which resolves) then finds the art already present. Unresolvable → + # treat as no-local-file (don't claim disk-missing). + resolved_rep = resolve_library_file_path( + rep_path, + transfer_folder=getattr(context, 'transfer_folder', None), + download_folder=download_folder, + config_manager=context.config_manager, + ) if rep_path else None + # Match the apply (_fix_missing_cover_art line ~1376: `... or p`): if + # the path-mapping layer returns nothing but the raw DB path is + # already a real file (the common Docker case where the container + # path == the stored path), use it as-is. Without this the scan never + # sees the folder and skips the album, while the apply WOULD have + # written the cover.jpg — the exact gap behind Sokhi's 0-findings. + if not resolved_rep and rep_path and os.path.isfile(rep_path): + resolved_rep = rep_path + has_local = bool(resolved_rep) + # Check embedded art and the cover.jpg sidecar SEPARATELY (not the + # combined album_has_art_on_disk, which returns True if EITHER is + # present). An album can have embedded art but no cover.jpg — and if + # the user wants cover.jpg files, that's still a fixable "missing" + # (Sokhi: scans returned 0 because embedded-art albums were treated + # as fully arted and skipped, so their cover.jpg never got written). + has_embedded = file_has_embedded_art(resolved_rep) if has_local else False + has_sidecar = folder_has_cover_sidecar(os.path.dirname(resolved_rep)) if has_local else False + # cover.jpg sidecars are only a "missing" thing when the user has + # cover.jpg writing enabled (Boulder: "only scan for cover.jpgs when + # they're enabled"). Default on. + cover_sidecar_enabled = bool( + context.config_manager.get('metadata_enhancement.cover_art_download', True) + if context.config_manager else True) + + if has_local: + embed_missing = not has_embedded + sidecar_missing = cover_sidecar_enabled and not has_sidecar + # has_embedded + no sidecar → the apply writes cover.jpg from the + # existing embedded art, so it's fixable even if the API finds no + # art. embed_missing still requires API art (nothing to embed). + sidecar_from_embedded = sidecar_missing and has_embedded + needs_fix = embed_missing or sidecar_missing + else: + # Media-server-only album: the DB thumb is the only art. + embed_missing = db_missing + sidecar_from_embedded = False + needs_fix = db_missing + + # Diagnostic: dump the decision inputs for the first few albums so a + # confusing "0 findings" scan reveals path-resolution + on-disk state + # (Sokhi: scans kept returning 0 with no way to see why). + if _diag_logged < 5: + logger.info( + "[cover-diag] album=%r db_path=%r resolved=%r local=%s embedded=%s " + "sidecar=%s db_missing=%s cover_enabled=%s needs_fix=%s", + title, rep_path, resolved_rep, has_local, has_embedded, has_sidecar, + db_missing, cover_sidecar_enabled, needs_fix) + _diag_logged += 1 + + if not needs_fix: + if not has_local: + skip_reasons['no_local_db_has_art'] += 1 + else: + skip_reasons['have_disk_art'] += 1 result.skipped += 1 continue @@ -197,10 +274,14 @@ class MissingCoverArtJob(RepairJob): if artwork_url: break - if artwork_url: + # Fixable if we found API art (to embed/write), OR it's just a + # missing cover.jpg on an album that already has embedded art — the + # apply writes the sidecar from that embedded art, no API art needed. + if artwork_url or sidecar_from_embedded: if context.report_progress: context.report_progress( - log_line=f'Found art: {title or "Unknown"}', + log_line=(f'Found art: {title or "Unknown"}' if artwork_url + else f'Will write cover.jpg from embedded art: {title or "Unknown"}'), log_type='success' ) # Also search for an artist image so the finding can offer it as @@ -212,6 +293,9 @@ class MissingCoverArtJob(RepairJob): # Create finding for user to approve if context.create_finding: try: + _desc = (f'Album "{title}" by {artist_name or "Unknown"} has no cover art. ' + + ('Found artwork from API.' if artwork_url + else 'Will write cover.jpg from the existing embedded art.')) inserted = context.create_finding( job_id=self.job_id, finding_type='missing_cover_art', @@ -220,7 +304,7 @@ class MissingCoverArtJob(RepairJob): entity_id=str(album_id), file_path=None, title=f'Missing artwork: {title or "Unknown"}', - description=f'Album "{title}" by {artist_name or "Unknown"} has no cover art. Found artwork from API.', + description=_desc, details={ 'album_id': album_id, 'album_title': title, @@ -239,7 +323,8 @@ class MissingCoverArtJob(RepairJob): # apply can embed into the audio + write cover.jpg. 'album_folder': os.path.dirname(rep_path) if rep_path else None, 'db_missing': db_missing, - 'disk_missing': disk_missing, + 'embed_missing': embed_missing, + 'sidecar_from_embedded': sidecar_from_embedded, 'musicbrainz_release_id': None, } ) @@ -251,6 +336,7 @@ class MissingCoverArtJob(RepairJob): logger.debug("Error creating cover art finding for album %s: %s", album_id, e) result.errors += 1 else: + skip_reasons['no_art_source'] += 1 result.skipped += 1 if context.update_progress and (i + 1) % 5 == 0: @@ -261,6 +347,13 @@ class MissingCoverArtJob(RepairJob): logger.info("Cover art scan: %d albums checked, %d found art, %d skipped", result.scanned, result.findings_created, result.skipped) + if result.skipped: + logger.info( + "[cover-diag] skip breakdown — have_disk_art=%d (already have embedded+sidecar), " + "no_local_db_has_art=%d (file path didn't resolve, DB has thumb), " + "no_art_source=%d (needed art but none found/embedded)", + skip_reasons['have_disk_art'], skip_reasons['no_local_db_has_art'], + skip_reasons['no_art_source']) return result def _try_source(self, source, source_album_id, title, artist_name): diff --git a/core/repair_worker.py b/core/repair_worker.py index 311109c9..60c81ff8 100644 --- a/core/repair_worker.py +++ b/core/repair_worker.py @@ -1325,7 +1325,11 @@ class RepairWorker: artist_result = self._fix_artist_art(album_id, details) artwork_url = details.get('found_artwork_url') - if not artwork_url: + # sidecar_from_embedded: the album already has embedded art and just needs + # a cover.jpg sidecar — the apply writes it from the existing embedded art, + # so no API artwork_url is required (Sokhi #813). + sidecar_from_embedded = bool(details.get('sidecar_from_embedded')) + if not artwork_url and not sidecar_from_embedded: # 'both' but no album art — report the artist outcome if that ran. if artist_result is not None: return artist_result @@ -1390,7 +1394,14 @@ class RepairWorker: 'album_name': album_title, 'album_image_url': artwork_url, 'musicbrainz_release_id': mbid, } - folder = details.get('album_folder') or os.path.dirname(resolved[0]) + # Use the RESOLVED file's directory — NOT details['album_folder'], which + # is the raw DB path (e.g. Jellyfin's /data/music) and frequently does + # NOT exist inside the SoulSync container (only the resolved /app/... + # path does). Passing the raw folder made os.path.isdir() fail in + # apply_art_to_album_files, silently skipping the cover.jpg write while + # embedding (which uses the resolved paths) still worked — Sokhi's + # "embeds art but never writes cover.jpgs". + folder = os.path.dirname(resolved[0]) art_result = apply_art_to_album_files(resolved, metadata, album_info, folder=folder) embedded = art_result.get('embedded', 0) @@ -1407,12 +1418,34 @@ class RepairWorker: 'mount (NFS/SMB/mergerfs) is read-write, then recreate the ' 'container. (Database thumbnail was still updated.)'), 'art_result': art_result} - msg = f'Applied cover art: embedded into {embedded}/{len(resolved)} file(s)' - if art_result.get('cover_written'): - msg += ' + wrote cover.jpg' - if embedded == 0 and not art_result.get('cover_written'): - # DB updated but nothing reached disk (e.g. permissions). - msg = 'Updated database thumbnail, but could not write art to files (read-only?)' + skipped = art_result.get('skipped', 0) + failed = art_result.get('failed', 0) + cover_written = art_result.get('cover_written') + + wrote_parts = [] + if embedded: + wrote_parts.append(f'embedded into {embedded}/{len(resolved)} file(s)') + if cover_written: + wrote_parts.append('wrote cover.jpg') + + if wrote_parts: + msg = 'Applied cover art: ' + ' + '.join(wrote_parts) + elif failed: + # Real per-file write failures that were NOT a read-only mount + # (genuine EROFS is handled above) — almost always file/folder + # permissions or a locked file. + msg = (f'Updated database thumbnail, but could not write art to ' + f'{failed} file(s) — check file/folder permissions') + elif skipped: + # Every file already had embedded art and no new cover.jpg was + # needed — nothing to do, NOT a failure. This is the case that made + # the old "(read-only?)" message fire on perfectly writable + # libraries (Boulder on Windows, Sokhi): the files were simply + # already arted, so embedded==0 and cover_written==False. + msg = f'Cover art already present on all {skipped} file(s) — database thumbnail updated' + else: + # No file art applied and nothing found to write. + msg = 'Updated database thumbnail (no file artwork was applied)' if artist_result is not None and artist_result.get('success'): msg += ' + applied artist image' return {'success': True, 'action': 'applied_cover_art', 'message': msg, 'art_result': art_result} diff --git a/core/search/by_id.py b/core/search/by_id.py index 7b8f73d1..032c87f4 100644 --- a/core/search/by_id.py +++ b/core/search/by_id.py @@ -33,6 +33,7 @@ an injected ``client_resolver`` (defaulting to the orchestrator's from __future__ import annotations import logging +import re from typing import Any, Callable, NamedTuple, Optional from urllib.parse import parse_qs, urlparse @@ -42,13 +43,13 @@ logger = logging.getLogger(__name__) # providers whose public links a user would paste AND whose get-by-id returns # the common Spotify-shaped dict. Streaming download backends (Tidal/Qobuz) # return raw API shapes and aren't metadata-link sources, so they're omitted. -SUPPORTED_SOURCES = ('spotify', 'itunes', 'musicbrainz', 'deezer') +SUPPORTED_SOURCES = ('spotify', 'itunes', 'musicbrainz', 'deezer', 'discogs') # Domains we recognize — used to detect a pasted URL even when the user # omitted the scheme (e.g. "open.spotify.com/album/…"). _KNOWN_HOSTS = ( 'open.spotify.com', 'music.apple.com', 'itunes.apple.com', - 'musicbrainz.org', 'deezer.com', + 'musicbrainz.org', 'deezer.com', 'discogs.com', ) @@ -72,7 +73,7 @@ class LookupTarget(NamedTuple): def _kind_from_keyword(keyword: str) -> Optional[str]: """Map a URL/URI path keyword to a lookup kind.""" - if keyword in ('album', 'release', 'release-group'): + if keyword in ('album', 'release', 'release-group', 'master'): return 'album' if keyword in ('track', 'recording', 'song'): return 'track' @@ -130,6 +131,18 @@ def _parse_url(raw: str) -> list[LookupTarget]: # redirect; only handle canonical /album/ /track/ paths. return _by_keyword('deezer') + if 'discogs.com' in host: + # Discogs paths are /artist/-Slug, /release/-Slug, + # /master/-Slug — the id is embedded with a slug, so strip to the + # leading number. (Discogs has no standalone track URLs; tracks live + # inside a release, so only artist/album resolve.) + out = [] + for t in _by_keyword('discogs'): + m = re.match(r'(\d+)', t.id) + if m: + out.append(t._replace(id=m.group(1))) + return out + return [] @@ -254,7 +267,7 @@ def _fetch_album(client: Any, source: str, identifier: str) -> Optional[dict]: (the modal re-fetches the full tracklist on open).""" if source == 'deezer': return client.get_album_metadata(identifier, include_tracks=False) - if source in ('itunes', 'musicbrainz'): + if source in ('itunes', 'musicbrainz', 'discogs'): return client.get_album(identifier, include_tracks=False) return client.get_album(identifier) # spotify @@ -274,8 +287,8 @@ def _fetch_artist(client: Any, source: str, identifier: str) -> Optional[dict]: # Shown in the dropdown's empty state so the user knows what to do next. _MSG_NOT_A_LINK = ( - 'Paste a full link from Spotify, Apple Music, MusicBrainz, or Deezer ' - '(a bare ID is ambiguous).' + 'Paste a full link from Spotify, Apple Music, Deezer, Discogs, or ' + 'MusicBrainz (a bare ID is ambiguous).' ) _MSG_NOT_FOUND = "Couldn't resolve that link — double-check it's correct." diff --git a/core/search/sources.py b/core/search/sources.py index 006bf8f0..e5d66805 100644 --- a/core/search/sources.py +++ b/core/search/sources.py @@ -35,6 +35,7 @@ def search_kind(client, query: str, kind: str, source_name: Optional[str] = None artists.append({ "id": artist.id, "name": artist.name, + "source": source_name or "", "image_url": artist.image_url, "external_urls": artist.external_urls or {}, }) @@ -52,6 +53,7 @@ def search_kind(client, query: str, kind: str, source_name: Optional[str] = None "id": album.id, "name": album.name, "artist": artist_name, + "source": source_name or "", "image_url": album.image_url, "release_date": album.release_date, "total_tracks": album.total_tracks, @@ -78,6 +80,21 @@ def search_kind(client, query: str, kind: str, source_name: Optional[str] = None "id": track.id, "name": track.name, "artist": artist_name, + # The REAL artist list, not the joined display string above. + # Spotify/Tidal/iTunes searches return collabs as a list; + # collapsing them to one "A, B" string made the import + # pipeline tag downloads with a single combined artist + # (resolve_track_artists saw one value). The frontend keeps + # using "artist" for display. + "artists": list(track.artists or []), + # Which metadata source this result came from. Travels with + # the payload through Download Now -> download task -> + # import context, where extract_source_metadata needs it to + # run source-specific logic (the Deezer contributors + # upgrade for multi-artist tags — Netti93's report: without + # it get_import_source() resolved '' and collab tracks + # were tagged with only the primary artist until a Retag). + "source": source_name or "", "album": track.album, "duration_ms": track.duration_ms, "image_url": track.image_url, diff --git a/core/security/__init__.py b/core/security/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/core/security/launch_lock.py b/core/security/launch_lock.py new file mode 100644 index 00000000..8b8ce52a --- /dev/null +++ b/core/security/launch_lock.py @@ -0,0 +1,89 @@ +"""Server-side enforcement of the launch PIN (#832). + +Beckid: the admin "launch PIN" was a client-side overlay only — the +``launch_pin_required`` flag just told the frontend to draw a fixed-position +div over the app. Removing that div (Safari "Hide Distracting Items", devtools, +or any non-browser client like curl) gave full, unauthenticated access to every +``/api/*`` endpoint, because nothing on the server ever checked it. + +``request_is_locked`` is the pure decision the ``before_request`` gate uses: +given the request path/method and the session's verified state, should this +request be blocked? Kept pure (no Flask) so the allow/deny matrix is unit- +testable without standing up the whole app. + +Allow-list while locked (everything else → 401): + * ``/`` and ``/static/`` and ``/favicon*`` — the page shell + lock-screen + assets must load so the user can enter the PIN. + * The unlock flow itself — current-profile probe, profile list/select for the + picker, verify-launch-pin, reset-pin-via-credential, logout. + * The public REST API ``/api/v1/`` — those routes carry their OWN + ``@require_api_key`` auth and are built for headless automation, so a + launch-locked UI shouldn't break a legitimate key holder. EXCEPT + ``/api/v1/api-keys-internal*``, which are session-UI key management + ("no auth required") and MUST stay locked — otherwise an attacker could + mint a key and walk in through the public API. +""" + +from __future__ import annotations + +# GET endpoints the lock/picker screens need before a PIN is entered. +_ALLOWED_GET = frozenset({ + '/api/profiles', # profile picker list (multi-profile launch) + '/api/profiles/current', # how the frontend detects the lock state +}) + +# POST endpoints that drive selection + unlock. Selecting a profile only sets +# session['profile_id'] (+ any per-profile PIN check); it does NOT set +# launch_pin_verified, so it can't bypass the launch lock. +_ALLOWED_POST = frozenset({ + '/api/profiles/select', + '/api/profiles/verify-launch-pin', + '/api/profiles/reset-pin-via-credential', + '/api/profiles/logout', +}) + + +def is_html_navigation(method: str, accept: str, sec_fetch_mode: str) -> bool: + """True when a BLOCKED request is a top-level browser navigation (address + bar, link, refresh) rather than a programmatic fetch/XHR. + + Such a request should be bounced to the root lock screen, not handed a raw + JSON 401 — otherwise deep-linking/refreshing on a sub-page (e.g. /dashboard) + while locked dumps JSON in the user's face (#832 follow-up). Programmatic + fetches (Accept: */* or application/json) still get the JSON so the frontend + can react to the lock. + """ + if (method or 'GET').upper() != 'GET': + return False + if (sec_fetch_mode or '').strip().lower() == 'navigate': + return True + return 'text/html' in (accept or '').lower() + + +def request_is_locked(path: str, method: str, *, + require_pin: bool, pin_verified: bool) -> bool: + """True when the launch-PIN gate must reject this request with 401.""" + if not require_pin or pin_verified: + return False + + path = path or '' + method = (method or 'GET').upper() + + # Page shell + assets needed to render the lock screen. + if path == '/' or path.startswith('/static/') or path.startswith('/favicon'): + return False + + # Key-authed public API — its own auth governs it. The session-UI key + # management under it is the one exception that stays locked. + if path.startswith('/api/v1/') and not path.startswith('/api/v1/api-keys-internal'): + return False + + if method == 'GET' and path in _ALLOWED_GET: + return False + if method == 'POST' and path in _ALLOWED_POST: + return False + + return True + + +__all__ = ['request_is_locked', 'is_html_navigation'] diff --git a/core/spotify_client.py b/core/spotify_client.py index aa1b4889..17824d09 100644 --- a/core/spotify_client.py +++ b/core/spotify_client.py @@ -633,6 +633,13 @@ class SpotifyClient: watchlist) use THIS instead of ``is_spotify_authenticated()`` so the free source is reachable. Does NOT change auth semantics.""" from core.spotify_free_metadata import should_offer_spotify_metadata + # The enrichment worker's prefer-free opt-in (set on its own client) + # makes the no-auth source the active path even without auth or the + # 'no-auth Spotify' source choice — so metadata IS available to it. This + # only fires on a client carrying _prefer_free (the worker's), so + # interactive/watchlist availability is unchanged. + if getattr(self, '_prefer_free', False) and self._free_installed(): + return True try: authed = self.is_spotify_authenticated() except Exception: @@ -646,13 +653,21 @@ class SpotifyClient: term covers the brief window before the auth cache refreshes. When authed + healthy the official path returns first, so this never opens. - Three activations fall out of this: a no-auth user who chose Spotify + Activations that fall out of this: a no-auth user who chose Spotify Free (free is their source), a connected user mid-rate-limit (free - bridges the ban), and a connected user who has spent the enrichment - worker's real-API daily budget (``_budget_exhausted_use_free``, set by - the worker) — so a Spotify-Free user is never paused by the budget, it - just switches to the uncapped free source. See _free_wanted().""" + bridges the ban), a connected user who has spent the enrichment worker's + real-API daily budget (``_budget_exhausted_use_free``, set by the worker) + — so a Spotify-Free user is never paused by the budget — and the worker + opt-in below. See _free_wanted().""" from core.spotify_free_metadata import should_use_free_fallback + # Worker opt-in (metadata.spotify_free_enrichment): prefer the no-creds + # source for enrichment even while authed + healthy + under budget, to + # spare the official quota for interactive use. The flag IS the explicit + # opt-in, so it only needs the package installed — not the 'Spotify Free' + # metadata-source choice — and it's set only on the enrichment worker's + # own client, so interactive search/resolve stay official-first. + if getattr(self, '_prefer_free', False) and self._free_installed(): + return True if not self._free_available(): return False try: @@ -1426,7 +1441,11 @@ class SpotifyClient: if tracks: return tracks - use_spotify = self.is_spotify_authenticated() + # Skip the official API when the no-creds free source should serve this + # (no-auth / rate-limited — where auth is already False — plus the + # budget-bridge and the worker's prefer-free opt-in, where auth is True + # but we deliberately defer to free). The free branch below then runs. + use_spotify = self.is_spotify_authenticated() and not self._free_active() if use_spotify: try: @@ -1492,7 +1511,11 @@ class SpotifyClient: artists.sort(key=lambda a: (0 if a.name.lower().strip() == query_lower else 1)) return artists - use_spotify = self.is_spotify_authenticated() + # Skip the official API when the no-creds free source should serve this + # (no-auth / rate-limited — where auth is already False — plus the + # budget-bridge and the worker's prefer-free opt-in, where auth is True + # but we deliberately defer to free). The free branch below then runs. + use_spotify = self.is_spotify_authenticated() and not self._free_active() if use_spotify: try: @@ -1548,11 +1571,17 @@ class SpotifyClient: return [] @rate_limited - def search_albums(self, query: str, limit: int = 10, allow_fallback: bool = True) -> List[Album]: + def search_albums(self, query: str, limit: int = 10, allow_fallback: bool = True, + artist: str = None, album: str = None) -> List[Album]: """Search for albums. When allow_fallback is True, falls back to the configured metadata source if Spotify is unavailable or returns an error. + + ``artist`` + ``album`` (the names, passed separately) enable the no-creds + Spotify Free path: SpotipyFree has no album-name search, so when Free is + active it resolves the album via the artist's discography. Callers without + that context (a bare query) skip the Free album path. """ cache = get_metadata_cache() # Check Spotify cache first so cached data remains usable even when @@ -1568,7 +1597,11 @@ class SpotifyClient: if albums: return albums - use_spotify = self.is_spotify_authenticated() + # Skip the official API when the no-creds free source should serve this + # (no-auth / rate-limited — where auth is already False — plus the + # budget-bridge and the worker's prefer-free opt-in, where auth is True + # but we deliberately defer to free). The free branch below then runs. + use_spotify = self.is_spotify_authenticated() and not self._free_active() if use_spotify: try: @@ -1592,7 +1625,21 @@ class SpotifyClient: except Exception as e: _detect_and_set_rate_limit(e, 'search_albums') logger.error(f"Error searching albums via Spotify: {e}") - # Fall through to iTunes fallback + # Fall through to free / iTunes fallback + + # No-creds Spotify (SpotipyFree): keep Spotify catalog/matching when + # official Spotify can't serve us (no auth / rate-limited / budget spent), + # before the iTunes/Deezer fallback. Albums have no name-search upstream, + # so resolve via the artist's discography — needs artist + album names. + # Gated by _free_active() so it never runs while auth is healthy. + if allow_fallback and self._free_active() and artist and album: + try: + objs = [Album.from_spotify_album(a) + for a in self._free_meta.search_albums_via_artist(artist, album, min(limit, 10))] + if objs: + return objs + except Exception as e: + logger.debug("SpotipyFree album search failed: %s", e) # Fallback (iTunes or Deezer) if allow_fallback: @@ -1623,7 +1670,7 @@ class SpotifyClient: # Fallback cache hit — delegate to fallback client which reconstructs enhanced format return self._fallback.get_track_details(track_id) - if self.is_spotify_authenticated(): + if self.is_spotify_authenticated() and not self._free_active(): try: track_data = self.sp.track(track_id) @@ -1725,7 +1772,7 @@ class SpotifyClient: # Fallback cache hit — delegate to fallback client return self._fallback.get_album(album_id) - if self.is_spotify_authenticated(): + if self.is_spotify_authenticated() and not self._free_active(): try: album_data = self.sp.album(album_id) if album_data: @@ -1768,7 +1815,7 @@ class SpotifyClient: if cached: return cached - if self.is_spotify_authenticated(): + if self.is_spotify_authenticated() and not self._free_active(): try: # Get first page of tracks first_page = self.sp.album_tracks(album_id) @@ -1862,7 +1909,7 @@ class SpotifyClient: except Exception as e: logger.debug("artist albums cache reuse: %s", e) - if self.is_spotify_authenticated(): + if self.is_spotify_authenticated() and not self._free_active(): try: albums = [] raw_items = [] @@ -1980,7 +2027,7 @@ class SpotifyClient: return self._fallback.get_artist(artist_id) return None - if self.is_spotify_authenticated(): + if self.is_spotify_authenticated() and not self._free_active(): try: result = self.sp.artist(artist_id) if result: diff --git a/core/spotify_free_metadata.py b/core/spotify_free_metadata.py index 51e05682..90064c88 100644 --- a/core/spotify_free_metadata.py +++ b/core/spotify_free_metadata.py @@ -27,10 +27,31 @@ from __future__ import annotations import importlib.util import logging +from difflib import SequenceMatcher from typing import Any, Optional logger = logging.getLogger(__name__) + +def _norm_name(s: str) -> str: + """Lowercase + collapse whitespace for loose name comparison.""" + return ' '.join((s or '').lower().split()) + + +def rank_albums_by_name(albums: list[dict], album_name: str, limit: int = 10) -> list[dict]: + """Rank Spotify-shaped album dicts by name similarity to ``album_name`` (best + first) and return up to ``limit``. Pure (no network) so it's unit-testable. + + Used to turn an artist's whole discography into the most-relevant album + candidates, since SpotipyFree has no album-name search of its own.""" + target = _norm_name(album_name) + scored = [ + (SequenceMatcher(None, _norm_name((alb or {}).get('name', '')), target).ratio(), alb) + for alb in (albums or []) + ] + scored.sort(key=lambda t: t[0], reverse=True) + return [alb for _score, alb in scored[:limit]] + _installed_cache: Optional[bool] = None @@ -186,9 +207,46 @@ class SpotifyFreeMetadataClient: def search_albums(self, query: str, limit: int = 10) -> list[dict]: # No album-name search exists in SpotipyFree/spotapi. Albums are only - # reachable by id or via an artist's discography. + # reachable by id or via an artist's discography — see + # search_albums_via_artist() for the artist-scoped workaround. return [] + def search_albums_via_artist(self, artist_name: str, album_name: str, + limit: int = 10) -> list[dict]: + """Find an album by name using ONLY the free source: SpotipyFree has no + album-name search, so resolve the artist (best name match) and scan their + discography for the album. Returns Spotify-shaped album dicts ranked by + similarity to ``album_name`` (best first); the caller applies its own + match threshold. Empty when artist/album is missing or no artist matches. + + This is what lets the budget/rate-limit bridge match albums on Spotify + Free instead of being unable to (the gap that previously dropped album + enrichment straight through to the iTunes/Deezer fallback).""" + if not (artist_name and album_name): + return [] + try: + artists = self.search_artists(artist_name, limit=5) + except Exception as e: + logger.debug(f"SpotipyFree search_albums_via_artist artist lookup failed: {e}") + return [] + if not artists: + return [] + target_artist = _norm_name(artist_name) + best = max( + artists, + key=lambda a: SequenceMatcher( + None, _norm_name(a.get('name', '')), target_artist).ratio(), + ) + artist_id = best.get('id') + if not artist_id: + return [] + try: + albums = self.get_artist_albums_list(artist_id, limit=50) + except Exception as e: + logger.debug(f"SpotipyFree search_albums_via_artist discography failed: {e}") + return [] + return rank_albums_by_name(albums, album_name, limit) + # -- entity lookups --------------------------------------------------- def get_album(self, album_id: str, include_tracks: bool = True) -> Optional[dict]: try: diff --git a/core/spotify_worker.py b/core/spotify_worker.py index aa780495..ffc6b07d 100644 --- a/core/spotify_worker.py +++ b/core/spotify_worker.py @@ -213,11 +213,29 @@ class SpotifyWorker: interruptible_sleep(self._stop_event, min(remaining, 60)) # Check again every 60s max continue - # Is the worker serving via the no-creds Spotify Free source this + # Enrichment runs on the no-auth Spotify source by DEFAULT + # (metadata.spotify_free_enrichment, ON unless turned off): bulk + # enrichment is the workload that bans the real API, so we keep it + # off your connected account's quota and reserve official Spotify + # for interactive search + playlist sync. The flag overrides auth + # for the worker (authed users still enrich via the no-auth path) + # and also lets the worker run with no auth at all. _free_active() + # and is_spotify_metadata_available() both honor it; set only on + # the worker's OWN client, so interactive paths stay official-first. + # Harmless when the no-auth package isn't installed (the methods + # fall back to official, then iTunes/Deezer). + try: + from config.settings import config_manager as _cfg + self.client._prefer_free = bool( + _cfg.get('metadata.spotify_free_enrichment', True)) + except Exception: # noqa: S110 — prefer-free toggle is best-effort + self.client._prefer_free = True + + # Is the worker serving via the no-auth Spotify source this # iteration? The daily budget and post-ban cooldown both exist to # protect the REAL authenticated API from bans — they don't apply - # to free (a different, anonymous path). Computed once and reused - # below; the loop already probes auth, so no extra quota cost. + # to the no-auth path. Computed once and reused below; the loop + # already probes auth, so no extra quota cost. budget_exhausted = self._is_daily_budget_exhausted() # Daily budget is a REAL-API ban protection. When it's spent, if @@ -768,7 +786,10 @@ class SpotifyWorker: return query = f"{artist_name} {album_name}" if artist_name else album_name - results = self.client.search_albums(query, limit=5) + # Pass artist + album names separately too, so the no-creds Spotify Free + # path can resolve the album via the artist's discography (SpotipyFree has + # no album-name search) when bridging a budget/rate-limit ban. + results = self.client.search_albums(query, limit=5, artist=artist_name, album=album_name) if not results: self._mark_status('album', album_id, 'not_found') @@ -929,6 +950,14 @@ class SpotifyWorker: UPDATE albums SET year = ? WHERE id = ? AND (year IS NULL OR year = '' OR year = '0') """, (year, album_id)) + # #824: also store the FULL release date when Spotify has one + # (YYYY-MM or YYYY-MM-DD, not just a bare year). Only when empty — + # never clobber a manually-set release_date. + if len(album_obj.release_date) > 4: + cursor.execute(""" + UPDATE albums SET release_date = ? + WHERE id = ? AND (release_date IS NULL OR release_date = '') + """, (album_obj.release_date, album_id)) # Cache the authoritative expected track count for the Album # Completeness repair job (see set_album_api_track_count docstring). diff --git a/core/sync/playlist_edit.py b/core/sync/playlist_edit.py index 80325238..a3423302 100644 --- a/core/sync/playlist_edit.py +++ b/core/sync/playlist_edit.py @@ -109,6 +109,34 @@ def plan_playlist_reconcile( return {"add": add, "remove": remove} +def plan_playlist_append( + current_ids: List[str], + desired_ids: List[str], +) -> List[str]: + """Plan an append: which desired ids are NOT already in the playlist. + + Used by ``sync_mode='append'`` (#823 round 2): the Jellyfin/Emby and + Navidrome appends deduped with ``{t.id for t in existing}`` — but their + track wrappers only define ``ratingKey``, never ``id``, so the existing-ids + set was ALWAYS empty and every sync re-appended the full matched list + (every track N times, "skipped 0 already present"). Pure planner so the + dedupe logic is testable; the caller fetches current ids however its + server API works and applies the returned adds. + + Order-preserving and duplicate-safe: desired order is kept, ids already + present are dropped, and duplicates WITHIN desired are emitted once. + """ + current_set = {str(t) for t in current_ids} + out: List[str] = [] + seen = set() + for d in desired_ids: + tid = str(d) + if tid and tid not in current_set and tid not in seen: + seen.add(tid) + out.append(tid) + return out + + VALID_SYNC_MODES = ("replace", "append", "reconcile") @@ -129,6 +157,7 @@ __all__ = [ "plan_playlist_add", "remove_one_occurrence", "plan_playlist_reconcile", + "plan_playlist_append", "normalize_sync_mode", "VALID_SYNC_MODES", ] diff --git a/core/tag_writer.py b/core/tag_writer.py index 707454ac..b0d4c6fa 100644 --- a/core/tag_writer.py +++ b/core/tag_writer.py @@ -210,10 +210,20 @@ def build_tag_diff(file_tags: Dict[str, Any], db_data: Dict[str, Any]) -> List[D db_str = ', '.join(db_val) if db_val else '' db_val = db_str if db_str else None - # Special: year — DB stores int, file stores string - if db_key == 'year' and db_val is not None: - db_str = str(db_val) - db_val = str(db_val) + # Special: year / release date (#824). Prefer the full release_date when + # the DB has one — it's authoritative, compare it directly. Otherwise use + # the year int, for which a MORE-specific file date with the same year is + # preserved (not flagged as a change). DB year is int, file is string. + if db_key == 'year': + release_date = db_data.get('release_date') + if release_date: + db_val = str(release_date) + db_str = str(release_date).strip() + elif db_val is not None: + db_str = str(db_val) + db_val = str(db_val) + if file_str and file_str[:4] == db_str: + file_str = db_str # Only mark as changed if DB has a value AND it differs from file # (writer skips fields where DB value is empty, so don't show them as diffs) @@ -319,7 +329,10 @@ def write_tags_to_file(file_path: str, db_data: Dict[str, Any], album = guard_placeholder_overwrite(album, _current.get('album')) album_artist = guard_placeholder_overwrite(album_artist, _current.get('album_artist')) - year = db_data.get('year') + # Prefer the full release_date (e.g. 2023-09-01) when the DB has one; + # fall back to the year-only int. _date_to_write() then writes the full + # date and still preserves an equally-specific existing file date (#824). + year = db_data.get('release_date') or db_data.get('year') genres = db_data.get('genres') track_num = db_data.get('track_number') total_tracks = db_data.get('track_count') @@ -390,13 +403,12 @@ def write_tags_to_file(file_path: str, db_data: Dict[str, Any], if art_ok: written.append('cover_art') - # Save - if isinstance(audio.tags, ID3): - audio.save(v1=0, v2_version=4) - elif isinstance(audio, FLAC): - audio.save(deleteid3=True) - else: - audio.save() + # Save — atomically (#819): write into a temp copy + atomic replace so an + # interrupted/OOM-killed save can never truncate the user's file. Same + # format kwargs as before, just routed through the shared atomic helper. + from types import SimpleNamespace + from core.metadata.common import save_audio_file + save_audio_file(audio, SimpleNamespace(ID3=ID3, FLAC=FLAC, File=MutagenFile)) return {'success': True, 'written_fields': written} @@ -443,6 +455,20 @@ def _multi_artist_write_enabled() -> bool: return False +def _date_to_write(existing: Optional[str], year) -> str: + """Value to write for the date/year tag. Writes the DB year, BUT keeps an + existing, MORE-specific file date (e.g. ``2023-11-03``) when its year already + matches — so enrichment/retag never downgrades a real full release date to + just the year (#824). When the years differ (a genuine correction) or the + file has no date, the year is written as before.""" + year_str = str(year) + if existing: + existing = str(existing).strip() + if len(existing) > 4 and existing[:4] == year_str: + return existing + return year_str + + def _write_id3(audio, title, artist, album_artist, album, year, genre, track_num, total_tracks, disc_num, bpm, artists_list: Optional[List[str]] = None) -> List[str]: @@ -474,8 +500,9 @@ def _write_id3(audio, title, artist, album_artist, album, year, genre, audio.tags.add(TALB(encoding=3, text=[album])) written.append('album') if year is not None: + existing_date = _id3_text(audio.tags, 'TDRC') audio.tags.delall('TDRC') - audio.tags.add(TDRC(encoding=3, text=[str(year)])) + audio.tags.add(TDRC(encoding=3, text=[_date_to_write(existing_date, year)])) written.append('year') if genre: audio.tags.delall('TCON') @@ -521,7 +548,7 @@ def _write_vorbis(audio, title, artist, album_artist, album, year, genre, audio['album'] = [album] written.append('album') if year is not None: - audio['date'] = [str(year)] + audio['date'] = [_date_to_write(_vorbis_first(audio, 'date'), year)] written.append('year') if genre: audio['genre'] = [genre] @@ -565,7 +592,7 @@ def _write_mp4(audio, title, artist, album_artist, album, year, genre, audio['\xa9alb'] = [album] written.append('album') if year is not None: - audio['\xa9day'] = [str(year)] + audio['\xa9day'] = [_date_to_write(_mp4_first(audio, '\xa9day'), year)] written.append('year') if genre: audio['\xa9gen'] = [genre] diff --git a/core/text/title_match.py b/core/text/title_match.py index 95f232f2..85e1c10b 100644 --- a/core/text/title_match.py +++ b/core/text/title_match.py @@ -115,6 +115,77 @@ def strip_redundant_context_qualifiers(title: str, *context_texts: str) -> str: return re.sub(r"\s+", " ", out).strip() +# Qualifier tokens that mark a genuinely DIFFERENT recording/cut — these must +# keep blocking a match. Union of the matching-engine keyword lists plus the +# Spanish markers seen in real libraries (#825: 'En Directo…', 'Versión 1988', +# 'Dueto 2007'). Titles reaching the matcher are unidecode-normalized, so the +# ASCII forms ('version') cover the accented ones ('versión'). +_VERSION_MARKER_TOKENS = frozenset({ + # English + "remix", "mix", "rmx", "live", "acoustic", "unplugged", "instrumental", + "karaoke", "demo", "demos", "edit", "version", "versions", "remaster", + "remastered", "slowed", "reverb", "sped", "spedup", "speedup", "extended", + "club", "mashup", "bootleg", "cover", "covers", "reprise", "session", + "sessions", "mono", "stereo", "duet", "rework", "dub", "vip", "single", + "radio", "alt", "alternate", "alternative", "take", "edition", "orchestral", + "symphonic", "piano", "acapella", "cappella", "nightcore", + # Distinct-track qualifiers — '(Interlude)' etc. are SEPARATE short tracks + # that share the base name with the full song; never treat as subtitles. + "interlude", "intro", "outro", "skit", "freestyle", "medley", "snippet", + # Part/volume markers whose number can be non-numeric ('Pt. II') — the + # digit guard below only catches actual digits. + "pt", "part", "vol", "ii", "iii", "iv", "vi", "vii", "viii", + # Spanish (unidecode-normalized; 'versión' → 'version' is covered above) + "directo", "vivo", "dueto", +}) + + +def strip_subtitle_qualifiers(title: str, other_title: str) -> str: + """Remove bracketed qualifiers that are SUBTITLES, not version markers. + + #825 (carlosjfcasero): the wishlist held 'Llamando a la tierra (Serenade + From the Stars)' — the song's official subtitle — while the library track + was the bare 'Llamando a la tierra'. The qualifier appears in no album or + counterpart title, so :func:`strip_redundant_context_qualifiers` keeps it, + and the length-ratio penalty then crushes an obviously-same song to ~0.14. + The sync matcher reported it missing on every run (re-adding it to the + wishlist) and the cleanup — same matcher — could never remove it. + + A qualifier is stripped only when ALL of: + * its text does not appear in ``other_title`` (if it does, the direct + comparison already handles it); + * it contains no version-marker token ('(Live)', '(Versión 1988)', + '(Dueto 2007)' keep blocking — they are different recordings); + * it introduces no digit token absent from ``other_title`` ('(Pt. 2)', + '(2007)' are different releases, never subtitles). + + Inputs should be normalized the same way the caller compares them + (lowercased / unidecode'd), like strip_redundant_context_qualifiers. + """ + if not title: + return title + + other = (other_title or "").casefold() + other_tokens = set(_TOKEN_RE.findall(other)) + + def _drop(match: re.Match) -> str: + inner = match.group(1).strip().casefold() + if not inner: + return " " + # Restated in the counterpart title — leave for the direct comparison. + if re.search(r"\b" + re.escape(inner) + r"\b", other): + return match.group(0) + tokens = _TOKEN_RE.findall(inner) + if any(t in _VERSION_MARKER_TOKENS for t in tokens): + return match.group(0) + if any(any(c.isdigit() for c in t) and t not in other_tokens for t in tokens): + return match.group(0) + return " " + + out = _QUALIFIER_RE.sub(_drop, title) + return re.sub(r"\s+", " ", out).strip() + + def numeric_tokens_differ(title_a: str, title_b: str) -> bool: """True when the digit-bearing tokens of two titles differ — 'Vol.4' vs 'Vol.4.5', 'Album' vs 'Album 2'. A numeric difference is a different @@ -133,5 +204,6 @@ def numeric_tokens_differ(title_a: str, title_b: str) -> bool: __all__ = [ "titles_plausibly_same", "strip_redundant_context_qualifiers", + "strip_subtitle_qualifiers", "numeric_tokens_differ", ] diff --git a/core/tidal_download_client.py b/core/tidal_download_client.py index 6ae2027e..4192606a 100644 --- a/core/tidal_download_client.py +++ b/core/tidal_download_client.py @@ -302,18 +302,21 @@ class TidalDownloadClient(DownloadSourcePlugin): @classmethod def _track_matches_qualifiers(cls, track, qualifiers: List[str]) -> bool: - """Issue #589 — qualifier check must inspect both track.name AND - track.album.name. For MTV Unplugged-style releases the live / - unplugged signal lives in the album title, not the track title. - A track passes if every required qualifier appears as a whole - word in either the track name OR its album name. + """Issue #589 — qualifier check must inspect track.name, track.version + AND track.album.name. Tidal stores remix/live/edit qualifiers in a + dedicated `version` attribute (e.g. name="Emerge", version="Junkie XL + Remix"), and for MTV Unplugged-style releases the live / unplugged + signal lives in the album title. A track passes if every required + qualifier appears as a whole word in the track name, its version, OR + its album name. """ if not qualifiers: return True track_name = (getattr(track, 'name', '') or '').lower() + version = (getattr(track, 'version', '') or '').lower() album = getattr(track, 'album', None) album_name = (getattr(album, 'name', '') or '').lower() if album else '' - haystack = f"{track_name} {album_name}".strip() + haystack = f"{track_name} {version} {album_name}".strip() if not haystack: return False for kw in qualifiers: @@ -475,6 +478,17 @@ class TidalDownloadClient(DownloadSourcePlugin): def _tidal_to_track_result(self, track, quality_info: dict) -> TrackResult: artist_name = track.artist.name if track.artist else 'Unknown Artist' title = track.name or 'Unknown Title' + # Tidal keeps remix/live/edition qualifiers in a separate `version` + # attribute (e.g. name="Emerge", version="Junkie XL Remix"). The + # matcher only scores `title`, so without folding the version in, + # every remix/live/edit candidate presents as the bare base track + # and MusicMatchingEngine's strict version check rejects it against + # a "... (Junkie XL Remix)" request. Append the version (unless it's + # already in the name) so the candidate title is the full + # "Title (Version)". + version = getattr(track, 'version', None) + if version and version.strip() and version.strip().lower() not in title.lower(): + title = f"{title} ({version.strip()})" album_name = track.album.name if track.album else None duration_ms = int(track.duration * 1000) if track.duration else None @@ -548,6 +562,62 @@ class TidalDownloadClient(DownloadSourcePlugin): return init_uri, segment_uris + # Tidal aggressively rate-limits the trackManifests endpoint. When a + # batch fans out, a bare request fails 429 instantly, the quality tier is + # burned, the track is re-queued, and the retry hammers again — a + # self-amplifying storm (thousands of 429s, downloads stalled, only the + # lowest tier squeaking through). Honour the server's Retry-After (or back + # off exponentially) so downloads pace themselves to whatever rate Tidal + # allows instead of slamming the wall and instant-failing. + _MANIFEST_MAX_RETRIES = 5 + _MANIFEST_BACKOFF_BASE = 2.0 # seconds → 2, 4, 8, 16, 32 (capped) + _MANIFEST_BACKOFF_CAP = 30.0 + + @classmethod + def _retry_after_seconds(cls, response, attempt: int) -> float: + """Backoff delay for a rate-limited response: honour a numeric + Retry-After header when present, else exponential backoff (capped).""" + retry_after = response.headers.get('Retry-After') if response is not None else None + if retry_after: + try: + return min(max(float(retry_after), 0.0), cls._MANIFEST_BACKOFF_CAP) + except (TypeError, ValueError): + pass + return min(cls._MANIFEST_BACKOFF_BASE * (2 ** attempt), cls._MANIFEST_BACKOFF_CAP) + + def _sleep_with_shutdown(self, delay: float) -> bool: + """Sleep up to `delay` seconds in small slices. Returns True if a + shutdown was requested mid-sleep so the caller can bail early.""" + slept = 0.0 + while slept < delay: + if self.shutdown_check and self.shutdown_check(): + return True + chunk = min(0.5, delay - slept) + time.sleep(chunk) + slept += chunk + return False + + def _get_with_rate_limit_retry(self, url: str, *, params=None, headers=None, + timeout: int = 20): + """HTTP GET that backs off and retries on 429 (and transient 5xx), + honouring Retry-After. Returns the final Response (the caller still + calls raise_for_status); a persistent rate-limit after all retries + returns the last 429 response so existing error handling applies.""" + response = None + for attempt in range(self._MANIFEST_MAX_RETRIES + 1): + response = http_requests.get(url, params=params, headers=headers, timeout=timeout) + if response.status_code != 429 and response.status_code < 500: + return response + if attempt >= self._MANIFEST_MAX_RETRIES: + return response + delay = self._retry_after_seconds(response, attempt) + logger.warning( + f"Tidal returned {response.status_code} on manifest fetch — backing off " + f"{delay:.1f}s (attempt {attempt + 1}/{self._MANIFEST_MAX_RETRIES})") + if self._sleep_with_shutdown(delay): + return response + return response + def _get_hls_manifest(self, track_id: int, quality: str = 'lossless') -> Optional[Dict]: q_info = HLS_QUALITY_MAP.get(quality, HLS_QUALITY_MAP['lossless']) formats = q_info['formats'] @@ -573,7 +643,7 @@ class TidalDownloadClient(DownloadSourcePlugin): } try: - response = http_requests.get(url, params=params, headers=headers, timeout=20) + response = self._get_with_rate_limit_retry(url, params=params, headers=headers, timeout=20) response.raise_for_status() data = response.json() except http_requests.HTTPError as e: diff --git a/core/watchlist_scanner.py b/core/watchlist_scanner.py index 0ce55e89..c99841eb 100644 --- a/core/watchlist_scanner.py +++ b/core/watchlist_scanner.py @@ -354,8 +354,15 @@ def _normalize_album_for_match(name: str) -> str: return cleaned +# Capture the FULL trailing number, including multi-part / decimal volumes. +# Normalization strips the dot in "Vol.5.5" to "vol 5 5", so without the +# `(?:\s+\d+)*` the extractor grabbed only the last "5" — making "Vol.5", +# "Vol.5.5" and "Vol.4.5" all look like volume "5" and collapse together. That +# made the watchlist treat tracks from different character-song CD volumes as +# duplicates and skip them (Sokhi: partially-filled discography never completed). _VOLUME_MARKER_RE = re.compile( - r'\b(?:vol(?:ume)?|pt|part|disc|book|chapter|episode)\.?\s*(\d+)\b|\b(\d+)\s*$', + r'\b(?:vol(?:ume)?|pt|part|disc|book|chapter|episode)\.?\s*(\d+(?:\s+\d+)*)\b' + r'|\b(\d+(?:\s+\d+)*)\s*$', re.IGNORECASE, ) @@ -1313,6 +1320,19 @@ class WatchlistScanner: logger.info("Skipping album with placeholder tracks: %s", album_name) continue if not self._should_include_release(len(tracks), artist): + # Make the type-filter skip visible — otherwise a user + # with "Albums" toggled off just sees missing tracks + # with no explanation (Sokhi #815-adjacent: 14 singles, + # 0 albums because include_albums was off). + _n = len(tracks) + _kind = 'album' if _n >= 7 else ('EP' if _n >= 4 else 'single') + logger.info( + "Skipping %s '%s' (%d tracks) — release type filter " + "(albums=%s, eps=%s, singles=%s) excludes it", + _kind, album_name, _n, + getattr(artist, 'include_albums', True), + getattr(artist, 'include_eps', True), + getattr(artist, 'include_singles', True)) continue album_image_url = '' @@ -1356,14 +1376,34 @@ class WatchlistScanner: if scan_state is not None: scan_state['tracks_found_this_scan'] += 1 - if self.add_track_to_wishlist(track, album_data, artist): + added = self.add_track_to_wishlist( + track, album_data, artist, + scan_run_id=(scan_state or {}).get('scan_run_id', ''), + ) + + track_artists = track.get('artists', []) + track_artist_name = track_artists[0].get('name', 'Unknown Artist') if track_artists else 'Unknown Artist' + + # #831: per-run ledger so the completed-scan + # summary can list WHICH tracks the counts mean. + # 'skipped' = found-new but add_to_wishlist + # declined (already queued in the wishlist, or + # the artist is blocklisted). Capped for sanity. + if scan_state is not None: + events = scan_state.setdefault('scan_track_events', []) + if len(events) < 500: + events.append({ + 'track_name': track_name, + 'artist_name': track_artist_name, + 'album_name': album_name, + 'album_image_url': album_image_url, + 'status': 'added' if added else 'skipped', + }) + + if added: artist_added_tracks += 1 if scan_state is not None: scan_state['tracks_added_this_scan'] += 1 - - track_artists = track.get('artists', []) - track_artist_name = track_artists[0].get('name', 'Unknown Artist') if track_artists else 'Unknown Artist' - if scan_state is not None: scan_state['recent_wishlist_additions'].insert(0, { 'track_name': track_name, 'artist_name': track_artist_name, @@ -2204,7 +2244,8 @@ class WatchlistScanner: logger.warning(f"Error checking if track exists: {track_name}: {e}") return True # Assume missing if we can't check - def add_track_to_wishlist(self, track, album, watchlist_artist: WatchlistArtist) -> bool: + def add_track_to_wishlist(self, track, album, watchlist_artist: WatchlistArtist, + scan_run_id: str = '') -> bool: """Add a missing track to the wishlist""" try: # Handle both dict and object track/album formats @@ -2286,7 +2327,9 @@ class WatchlistScanner: 'watchlist_artist_name': watchlist_artist.artist_name, 'watchlist_artist_id': watchlist_artist.spotify_artist_id, 'album_name': album_name, - 'scan_timestamp': datetime.now().isoformat() + 'scan_timestamp': datetime.now().isoformat(), + # #831: groups wishlist rows by the scan run that added them. + 'scan_run_id': scan_run_id or '', }, profile_id=getattr(watchlist_artist, 'profile_id', 1) ) diff --git a/core/wishlist/processing.py b/core/wishlist/processing.py index b9aabad2..cac989fe 100644 --- a/core/wishlist/processing.py +++ b/core/wishlist/processing.py @@ -61,7 +61,7 @@ class WishlistAutoProcessingRuntime: update_automation_progress: Callable[..., Any] automation_engine: Any missing_download_executor: Any - run_full_missing_tracks_process: Callable[[str, str, list[dict[str, Any]]], Any] + run_full_missing_tracks_process: Callable[..., Any] # (batch_id, playlist_id, tracks, serialize=False) get_batch_max_concurrent: Callable[[], int] get_active_server: Callable[[], str] current_time_fn: Callable[[], float] @@ -246,11 +246,14 @@ def _run_wishlist_cycle( f"'{album_name}' ({len(group.tracks)} tracks) → {album_batch_id}" ) submitted.append(album_batch_id) - # Album bundles block their worker for the whole search+download → dedicated - # pool (falls back to the shared pool when unset). See #740. + # Album bundles block their worker for the whole search+download → + # dedicated pool (falls back to the shared pool when unset). serialize=True + # makes the worker actually HOLD its pool slot until the album drains, so + # only a few albums are in flight at once instead of every album flooding + # the shared download pool with 'searching' tracks (#740 / Sokhi). album_executor.submit( runtime.run_full_missing_tracks_process, - album_batch_id, playlist_id, group.tracks, + album_batch_id, playlist_id, group.tracks, True, ) residual_tracks = grouping.residual_tracks if grouping is not None else tracks @@ -626,7 +629,7 @@ class WishlistManualDownloadRuntime: download_batches: Dict[str, Dict[str, Any]] tasks_lock: Any missing_download_executor: Any - run_full_missing_tracks_process: Callable[[str, str, list[dict[str, Any]]], Any] + run_full_missing_tracks_process: Callable[..., Any] # (batch_id, playlist_id, tracks, serialize=False) get_batch_max_concurrent: Callable[[], int] add_activity_item: Callable[[Any, Any, Any, Any], Any] active_server: str diff --git a/core/wishlist/routes.py b/core/wishlist/routes.py index 53a36af2..c05a626c 100644 --- a/core/wishlist/routes.py +++ b/core/wishlist/routes.py @@ -423,6 +423,33 @@ def add_album_track_to_wishlist( track_data = _build_track_data(track, album) + # #825: don't add a track that's already in the library, unless the user + # has opted into duplicates. The manual album "add to wishlist" modal + # otherwise dumped owned tracks straight into the wishlist with no check + # (carlosjfcasero) — and the auto-cleanup may not reliably remove them. + # Respects the same wishlist.allow_duplicate_tracks toggle the watchlist + # scan + cleanup use: OFF → skip owned, ON → add anyway. (The quality + # re-download flow uses a different endpoint, so it's unaffected.) + try: + from config.settings import config_manager as _cfg + if not _cfg.get('wishlist.allow_duplicate_tracks', True): + _db = runtime.get_music_database() + _existing, _conf = _db.check_track_exists( + track.get('name', ''), artist.get('name', ''), + confidence_threshold=0.7, + server_source=runtime.active_server, + album=album.get('name', ''), + ) + if _existing and _conf >= 0.7: + runtime.logger.info( + "[Wishlist Add] skipping '%s' by '%s' — already in library " + "(allow_duplicate_tracks is off)", + track.get('name'), artist.get('name')) + return {"success": True, "skipped": True, + "message": f"'{track.get('name')}' is already in your library"}, 200 + except Exception as _own_err: + runtime.logger.debug("Wishlist add ownership check failed (adding anyway): %s", _own_err) + enhanced_source_context = { **(source_context or {}), "artist_id": artist.get("id"), diff --git a/database/music_database.py b/database/music_database.py index 1e7c3a6b..d87e92de 100644 --- a/database/music_database.py +++ b/database/music_database.py @@ -649,6 +649,27 @@ class MusicDatabase: logger.info(f"Added {_col} column to library_history") cursor.execute("CREATE INDEX IF NOT EXISTS idx_lh_origin ON library_history (origin, created_at DESC)") + # Watchlist scan history (#831 round 2) — one row per scan run with + # its full track ledger (added/skipped), so the Watchlist page can + # show what every past run did. Wishlist rows erode as tracks + # download, so this is the durable record. + cursor.execute(""" + CREATE TABLE IF NOT EXISTS watchlist_scan_runs ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + run_id TEXT NOT NULL UNIQUE, + profile_id INTEGER DEFAULT 1, + status TEXT NOT NULL, + started_at TIMESTAMP, + completed_at TIMESTAMP, + total_artists INTEGER DEFAULT 0, + artists_scanned INTEGER DEFAULT 0, + tracks_found INTEGER DEFAULT 0, + tracks_added INTEGER DEFAULT 0, + track_events TEXT + ) + """) + cursor.execute("CREATE INDEX IF NOT EXISTS idx_wsr_completed ON watchlist_scan_runs (completed_at DESC)") + # Auto-import history — tracks auto-import scan results and processing status cursor.execute(""" CREATE TABLE IF NOT EXISTS auto_import_history ( @@ -1020,6 +1041,15 @@ class MusicDatabase: cursor.execute("ALTER TABLE albums ADD COLUMN api_track_count INTEGER DEFAULT NULL") logger.info("Repaired missing api_track_count column on albums table") + # Full release date (#824). Additive + nullable: NULL means "only the + # year is known", and every reader falls back to albums.year, so this + # is safe to ship dormant. Populated by enrichment + manual edit; + # consumed by the tag writer to write the full date (e.g. 2023-09-01) + # instead of truncating it to the year. + if album_cols and 'release_date' not in album_cols: + cursor.execute("ALTER TABLE albums ADD COLUMN release_date TEXT DEFAULT NULL") + logger.info("Added release_date column to albums table (#824)") + # Canonical album version (#765 / #767-Bug2). Additive + nullable: # a NULL canonical means "unresolved" and every tool falls back to # today's behavior, so this is safe to ship dormant. Columns are @@ -1352,6 +1382,7 @@ class MusicDatabase: artist_id TEXT NOT NULL, title TEXT NOT NULL, year INTEGER, + release_date TEXT, thumb_url TEXT, genres TEXT, track_count INTEGER, @@ -6250,11 +6281,47 @@ class MusicDatabase: )) return tracks - + except Exception as e: logger.error(f"Error getting tracks for album {album_id}: {e}") return [] - + + def get_album_by_spotify_album_id(self, spotify_album_id: str) -> Optional[DatabaseAlbum]: + """Fetch a single album by its (enriched) Spotify album id, or None. + + Used by the download path builder (#829) to reuse an album's existing + on-disk folder when re-downloading into the same album — matching the + exact stored Spotify id before falling back to fuzzy name+artist. + """ + if not spotify_album_id: + return None + try: + conn = self._get_connection() + cursor = conn.cursor() + cursor.execute(""" + SELECT albums.*, artists.name as artist_name + FROM albums + JOIN artists ON albums.artist_id = artists.id + WHERE albums.spotify_album_id = ? + LIMIT 1 + """, (spotify_album_id,)) + row = cursor.fetchone() + if not row: + return None + genres = json.loads(row['genres']) if row['genres'] else None + album = DatabaseAlbum( + id=row['id'], artist_id=row['artist_id'], title=row['title'], + year=row['year'], thumb_url=row['thumb_url'], genres=genres, + track_count=row['track_count'], duration=row['duration'], + created_at=datetime.fromisoformat(row['created_at']) if row['created_at'] else None, + updated_at=datetime.fromisoformat(row['updated_at']) if row['updated_at'] else None, + ) + album.artist_name = row['artist_name'] + return album + except Exception as e: + logger.error(f"Error getting album by spotify_album_id {spotify_album_id}: {e}") + return None + def search_artists(self, query: str, limit: int = 50, server_source: str = None) -> List[DatabaseArtist]: """Search artists by name, optionally filtered by server source. Uses diacritic-insensitive matching so 'Tiesto' finds 'Tiësto'.""" @@ -7622,6 +7689,24 @@ class MusicDatabase: ctx_sim *= ctx_ratio # 'Believe' vs 'Believe In Me' still penalised best_title_similarity = max(best_title_similarity, ctx_sim) + # #825: a bracketed qualifier that is a SUBTITLE — not a version + # marker and not numeric — is the same song. 'Llamando a la tierra + # (Serenade From the Stars)' vs the library's bare 'Llamando a la + # tierra': the subtitle restates nothing (so #808 keeps it) and the + # length penalty crushed the pair to ~0.14 — sync re-added it to + # the wishlist forever and cleanup (same matcher) never removed it. + # Version qualifiers ('(Live)', '(Versión 1988)', '(Dueto 2007)') + # are kept by the helper, so their mismatch penalty still stands. + from core.text.title_match import strip_subtitle_qualifiers + sub_search = strip_subtitle_qualifiers(search_title_norm, db_title_norm) + sub_db = strip_subtitle_qualifiers(db_title_norm, search_title_norm) + if (sub_search, sub_db) != (search_title_norm, db_title_norm) and sub_search and sub_db: + sub_sim = self._string_similarity(sub_search, sub_db) + sub_ratio = min(len(sub_search), len(sub_db)) / max(len(sub_search), len(sub_db)) + if sub_ratio < 0.7: + sub_sim *= sub_ratio # stripped forms still length-guarded + best_title_similarity = max(best_title_similarity, sub_sim) + # Word-level guard: SequenceMatcher's char ratio over-credits # different songs that share a long substring or only a stopword # ("Dani California" vs "Californication" = 0.67; "Under The Bridge" @@ -10507,6 +10592,7 @@ class MusicDatabase: MIN(a.id) as id, a.title, a.year, + MAX(a.release_date) as release_date, SUM(a.track_count) as track_count, MAX(a.thumb_url) as thumb_url, MAX(a.musicbrainz_release_id) as musicbrainz_release_id, @@ -10566,6 +10652,7 @@ class MusicDatabase: 'id': album_row['id'], 'title': album_row['title'], 'year': album_row['year'], + 'release_date': album_row['release_date'], 'image_url': album_row['thumb_url'], 'owned': True, # All albums in our DB are owned 'track_count': album_row['track_count'], @@ -10648,7 +10735,7 @@ class MusicDatabase: # Field whitelists for safe updates ARTIST_EDITABLE_FIELDS = {'name', 'genres', 'summary', 'style', 'mood', 'label'} - ALBUM_EDITABLE_FIELDS = {'title', 'year', 'genres', 'style', 'mood', 'label', 'explicit', 'record_type', 'track_count'} + ALBUM_EDITABLE_FIELDS = {'title', 'year', 'release_date', 'genres', 'style', 'mood', 'label', 'explicit', 'record_type', 'track_count'} TRACK_EDITABLE_FIELDS = {'title', 'track_number', 'bpm', 'explicit', 'style', 'mood'} def get_artist_full_detail(self, artist_id) -> Dict[str, Any]: @@ -12478,6 +12565,75 @@ class MusicDatabase: logger.debug(f"Error adding library history entry: {e}") return False + def save_watchlist_scan_run(self, run_id, profile_id=1, status='completed', + started_at=None, completed_at=None, + total_artists=0, artists_scanned=0, + tracks_found=0, tracks_added=0, + track_events=None, keep_last=100) -> bool: + """Persist one watchlist scan run + its track ledger (#831 round 2). + + Idempotent on run_id (re-saving a run replaces it). Prunes the table to + the most recent ``keep_last`` runs so history can't grow unbounded.""" + try: + conn = self._get_connection() + cursor = conn.cursor() + cursor.execute(""" + INSERT OR REPLACE INTO watchlist_scan_runs + (run_id, profile_id, status, started_at, completed_at, + total_artists, artists_scanned, tracks_found, tracks_added, + track_events) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + """, (run_id, profile_id, status, started_at, completed_at, + total_artists, artists_scanned, tracks_found, tracks_added, + json.dumps(track_events or []))) + cursor.execute(""" + DELETE FROM watchlist_scan_runs WHERE id NOT IN ( + SELECT id FROM watchlist_scan_runs + ORDER BY completed_at DESC, id DESC LIMIT ? + ) + """, (keep_last,)) + conn.commit() + return True + except Exception as e: + logger.error(f"Error saving watchlist scan run {run_id}: {e}") + return False + + def get_watchlist_scan_runs(self, limit=30, profile_id=None): + """Recent watchlist scan runs, newest first — WITHOUT track ledgers + (fetch those per-run via get_watchlist_scan_run_events).""" + try: + conn = self._get_connection() + cursor = conn.cursor() + where = "WHERE profile_id = ?" if profile_id is not None else "" + params = ([profile_id] if profile_id is not None else []) + [limit] + cursor.execute(f""" + SELECT run_id, profile_id, status, started_at, completed_at, + total_artists, artists_scanned, tracks_found, tracks_added + FROM watchlist_scan_runs {where} + ORDER BY completed_at DESC, id DESC LIMIT ? + """, params) + return [dict(row) for row in cursor.fetchall()] + except Exception as e: + logger.error(f"Error getting watchlist scan runs: {e}") + return [] + + def get_watchlist_scan_run_events(self, run_id): + """The track ledger (added/skipped events) for one scan run.""" + try: + conn = self._get_connection() + cursor = conn.cursor() + cursor.execute( + "SELECT track_events FROM watchlist_scan_runs WHERE run_id = ?", + (run_id,)) + row = cursor.fetchone() + if not row or not row['track_events']: + return [] + events = json.loads(row['track_events']) + return events if isinstance(events, list) else [] + except Exception as e: + logger.error(f"Error getting watchlist scan run events for {run_id}: {e}") + return [] + def get_origin_cleanup_candidates(self): """Origin-tracked downloads (watchlist/playlist) annotated with the matching library track's play_count, for the Expired Download Cleaner. diff --git a/services/sync_service.py b/services/sync_service.py index 803f2e8e..b48aa051 100644 --- a/services/sync_service.py +++ b/services/sync_service.py @@ -665,6 +665,10 @@ class PlaylistSyncService: _try_title, _try_artist, confidence_threshold=0.7, server_source=active_server, candidate_tracks=artist_candidates, + # #825: album context enables the album-aware fallback + # (multi-artist albums filed under another artist) — + # the cleanup path already passes it; sync didn't. + album=getattr(spotify_track, 'album', None) or None, ) if _cand_conf > confidence: db_track, confidence = _cand_track, _cand_conf diff --git a/tests/automation/test_handlers_playlist.py b/tests/automation/test_handlers_playlist.py index a690d7b6..e13179e3 100644 --- a/tests/automation/test_handlers_playlist.py +++ b/tests/automation/test_handlers_playlist.py @@ -738,6 +738,12 @@ class TestSyncPlaylist: import time time.sleep(0.01) assert len(sync_calls) == 1 + # #823 — the handler must NOT force a sync mode; it leaves it unset so + # _run_sync_task resolves the user's configured global mode (else the + # automated sync always 'replace'd and wiped the playlist image/desc). + args, kwargs = sync_calls[0] + assert kwargs.get('sync_mode') is None # not forced via kwarg + assert len(args) == 6 # no 7th positional sync_mode either def test_organize_by_playlist_passes_skip_wishlist_add(self): discovered_track = { diff --git a/tests/downloads/test_album_serialize_drain.py b/tests/downloads/test_album_serialize_drain.py new file mode 100644 index 00000000..364452ff --- /dev/null +++ b/tests/downloads/test_album_serialize_drain.py @@ -0,0 +1,73 @@ +"""Album-bundle serialization wait (#740 / Sokhi "too many searching"). + +_wait_for_batch_drain holds the album-pool worker until the batch's tasks all +reach a terminal state — so only a few albums are in flight at once instead of +every album flooding the shared download pool. It's a passive wait that must +also bail on shutdown / a removed batch / a safety cap. +""" + +import threading +import time + +import pytest + +from core.runtime_state import download_batches, download_tasks, tasks_lock +from core.downloads import master, monitor + + +def _set_batch(bid, task_statuses): + with tasks_lock: + download_batches[bid] = {'queue': list(task_statuses.keys())} + for tid, st in task_statuses.items(): + download_tasks[tid] = {'status': st} + + +@pytest.fixture(autouse=True) +def _cleanup(): + yield + with tasks_lock: + for bid in ('b1', 'b2', 'b3', 'b4'): + download_batches.pop(bid, None) + for tid in ('t1', 't2', 't3'): + download_tasks.pop(tid, None) + + +def test_returns_immediately_when_all_terminal(): + _set_batch('b1', {'t1': 'completed', 't2': 'failed', 't3': 'not_found'}) + start = time.time() + master._wait_for_batch_drain('b1', poll_seconds=0.05, max_wait_seconds=5) + assert time.time() - start < 1.0 # nothing in flight → no block + + +def test_returns_when_batch_missing(): + master._wait_for_batch_drain('nope', poll_seconds=0.05, max_wait_seconds=5) # no hang + + +def test_waits_until_tasks_go_terminal(): + _set_batch('b2', {'t1': 'searching', 't2': 'downloading'}) + + def finish(): + time.sleep(0.25) + with tasks_lock: + download_tasks['t1']['status'] = 'completed' + download_tasks['t2']['status'] = 'failed' + + threading.Thread(target=finish, daemon=True).start() + start = time.time() + master._wait_for_batch_drain('b2', poll_seconds=0.05, max_wait_seconds=5) + assert 0.2 < time.time() - start < 3.0 # held the slot until they finished + + +def test_bails_on_shutdown(monkeypatch): + _set_batch('b3', {'t1': 'searching'}) # never terminal + monkeypatch.setattr(monitor, 'IS_SHUTTING_DOWN', True) + start = time.time() + master._wait_for_batch_drain('b3', poll_seconds=0.05, max_wait_seconds=10) + assert time.time() - start < 1.0 # didn't block app shutdown + + +def test_respects_safety_cap(): + _set_batch('b4', {'t1': 'searching'}) # never terminal + start = time.time() + master._wait_for_batch_drain('b4', poll_seconds=0.05, max_wait_seconds=0.3) + assert 0.3 <= time.time() - start < 2.0 # released the slot after the cap diff --git a/tests/downloads/test_track_link.py b/tests/downloads/test_track_link.py new file mode 100644 index 00000000..39dfe8a1 --- /dev/null +++ b/tests/downloads/test_track_link.py @@ -0,0 +1,80 @@ +"""Parse pasted Tidal/Qobuz track links for the manual download search (#813).""" + +from core.downloads.track_link import parse_download_track_link as p + + +def test_tidal_track_url_with_region_suffix(): + assert p('https://tidal.com/track/434945950/u') == ('tidal', '434945950') + + +def test_tidal_browse_and_listen_hosts(): + assert p('https://tidal.com/browse/track/434945950') == ('tidal', '434945950') + assert p('https://listen.tidal.com/track/434945950') == ('tidal', '434945950') + + +def test_qobuz_track_urls(): + assert p('https://open.qobuz.com/track/12345678') == ('qobuz', '12345678') + assert p('https://play.qobuz.com/track/12345678') == ('qobuz', '12345678') + + +def test_scheme_less(): + assert p('tidal.com/track/999') == ('tidal', '999') + + +def test_id_with_slug_suffix(): + assert p('https://www.qobuz.com/track/555-some-slug') == ('qobuz', '555') + + +def test_non_track_links_rejected(): + assert p('https://tidal.com/album/123') is None # album, not track + assert p('https://tidal.com/artist/123') is None + assert p('https://open.spotify.com/track/abc') is None # unsupported source + assert p('https://example.com/track/123') is None + + +def test_garbage_rejected(): + assert p('') is None + assert p('just some text') is None + assert p('Habbit (T-Mass Remix)') is None + + +# ── query_from_track_payload (pure per-source parsing) ── + +from core.downloads.track_link import query_from_track_payload as q + + +def test_tidal_payload_appends_version(): + # Tidal attributes: title + version → remix link searches for the remix. + raw = {'title': 'Habbit', 'version': 'T-Mass Remix', + 'artists': [{'name': 'Rain Man'}, {'name': 'Krysta Youngs'}]} + assert q('tidal', raw) == 'Rain Man Habbit (T-Mass Remix)' + + +def test_tidal_payload_no_version_no_artist(): + assert q('tidal', {'title': 'Bloom'}) == 'Bloom' + + +def test_tidal_payload_singular_artist(): + assert q('tidal', {'title': 'X', 'artist': {'name': 'Jinco'}}) == 'Jinco X' + + +def test_tidal_version_already_in_title_not_doubled(): + raw = {'title': 'Bloom (Nurko Remix)', 'version': 'Nurko Remix', + 'artists': [{'name': 'Dabin'}]} + assert q('tidal', raw) == 'Dabin Bloom (Nurko Remix)' + + +def test_qobuz_payload_performer(): + raw = {'title': "What's Good For Me", 'performer': {'name': 'Jinco'}} + assert q('qobuz', raw) == "Jinco What's Good For Me" + + +def test_qobuz_payload_falls_back_to_album_artist(): + raw = {'title': 'Song', 'album': {'artist': {'name': 'Some Artist'}}} + assert q('qobuz', raw) == 'Some Artist Song' + + +def test_payload_non_dict_or_empty(): + assert q('tidal', None) is None + assert q('tidal', {}) is None + assert q('qobuz', 'garbage') is None diff --git a/tests/imports/test_import_context.py b/tests/imports/test_import_context.py index e71584b7..5ce80835 100644 --- a/tests/imports/test_import_context.py +++ b/tests/imports/test_import_context.py @@ -338,3 +338,20 @@ def test_detect_album_info_web_forces_album_when_track_and_artist_differ(): assert album_info["album_name"] == "Album One" assert album_info["track_number"] == 4 assert album_info["disc_number"] == 2 + + +def test_get_import_source_reads_underscore_source_from_nested_dicts(): + """Netti93 multi-artist fix: many track payloads carry '_source' (the + discography/wishlist dicts) or 'source' only inside track_info (search + results). get_import_source must resolve all of them — previously only + the context-level keys worked, so direct downloads resolved '' and + source-specific metadata logic never ran.""" + from core.imports.context import get_import_source + + assert get_import_source({"track_info": {"source": "deezer"}}) == "deezer" + assert get_import_source({"track_info": {"_source": "deezer"}}) == "deezer" + assert get_import_source({"original_search_result": {"_source": "itunes"}}) == "itunes" + assert get_import_source({"_source": "tidal"}) == "tidal" + # context-level 'source' still wins over nested + assert get_import_source({"source": "spotify", "track_info": {"_source": "deezer"}}) == "spotify" + assert get_import_source({}) == "" diff --git a/tests/matching/test_divergent_version.py b/tests/matching/test_divergent_version.py new file mode 100644 index 00000000..f3d5764d --- /dev/null +++ b/tests/matching/test_divergent_version.py @@ -0,0 +1,100 @@ +"""Divergent-version matching: two DIFFERENT versions of the same base +title must NOT match. + +Context: Tidal stores remix/live/edit qualifiers in a dedicated `version` +field which `_tidal_to_track_result` now folds into the candidate title so +the matcher can see it. That fix made the *correct* version win — but it +also makes OTHER versions of the same song visible ("We Are The People +(Shazam Remix)" vs "(southstar Remix)"). Neither title is a prefix of the +other, so the original prefix-based version check missed them and the raw +ratio stayed high (~0.8) off the shared base. Without discrimination, when +the requested version is absent a different remix could outscore the +threshold and the wrong cut would be downloaded. + +These pin: different descriptors reject, the correct one still wins, and +the existing original-vs-version / remaster behaviour is preserved. +""" + +from __future__ import annotations + +import pytest + +from core.matching_engine import MusicMatchingEngine + +me = MusicMatchingEngine() + + +# ── similarity_score: divergent version tails (already-normalised input) ── + +def test_different_remix_descriptors_rejected(): + assert me.similarity_score( + 'we are the people shazam remix', + 'we are the people southstar remix', + ) == 0.30 + + +def test_different_live_performances_rejected(): + assert me.similarity_score( + 'all night live at pukkelpop', + 'all night live at wembley', + ) == 0.30 + + +def test_different_version_types_same_base_rejected(): + # Same song, different version TYPE (remix vs live) — different cut. + assert me.similarity_score('song title remix', 'song title live') == 0.30 + + +def test_same_base_non_version_tails_not_penalised(): + # "one" / "two" are not version words — leave the raw ratio alone. + assert me.similarity_score('song one', 'song two') != 0.30 + + +# ── regression: original-vs-version + remaster behaviour preserved ── + +def test_original_vs_remix_still_rejected(): + assert me.similarity_score('we are the people', 'we are the people remix') == 0.30 + + +def test_remaster_still_light_penalty(): + assert me.similarity_score('song title', 'song title remastered') == 0.75 + + +def test_identical_titles_still_perfect(): + assert me.similarity_score('emerge junkie xl remix', 'emerge junkie xl remix') == 1.0 + + +# ── end-to-end via score_track_match (raw titles, real weighting) ── + +_ARTIST = 'Empire Of The Sun' + + +def test_wrong_remix_scored_below_threshold(): + # Requested Shazam Remix, only a different remix available → must land + # well under the 0.55/0.60 acceptance gate so it is never downloaded. + conf, _ = me.score_track_match( + 'We Are The People (Shazam Remix)', [_ARTIST], 344_000, + 'We Are The People (southstar Remix)', [_ARTIST], 236_000, + ) + assert conf < 0.55, f'wrong remix scored {conf:.2f}, should be < 0.55' + + +def test_correct_remix_still_wins(): + conf, _ = me.score_track_match( + 'We Are The People (Shazam Remix)', [_ARTIST], 344_000, + 'We Are The People (Shazam Remix)', [_ARTIST], 344_000, + ) + assert conf >= 0.90, f'correct remix scored {conf:.2f}, should be >= 0.90' + + +@pytest.mark.parametrize('wanted,candidate', [ + ('We Are The People (Shazam Remix)', 'We Are The People (ARTBAT Remix)'), + ('All Night (Live @ Pukkelpop)', 'All Night (Umek Remix)'), + ('Emerge (Junkie XL Remix)', 'Emerge (DFA Version)'), +]) +def test_wrong_version_below_correct(wanted, candidate): + artist = 'X' + wrong, _ = me.score_track_match(wanted, [artist], 0, candidate, [artist], 0) + right, _ = me.score_track_match(wanted, [artist], 0, wanted, [artist], 0) + assert right > wrong + assert wrong < 0.60, f'{candidate!r} scored {wrong:.2f} vs {wanted!r}' diff --git a/tests/metadata/test_discography_filters.py b/tests/metadata/test_discography_filters.py index 400e0885..8c297d05 100644 --- a/tests/metadata/test_discography_filters.py +++ b/tests/metadata/test_discography_filters.py @@ -61,6 +61,35 @@ class TestTrackArtistMatches: assert track_artist_matches(['DRAKE'], 'Drake') is True assert track_artist_matches(['Drake'], 'drake') is True + def test_combined_collab_credit_string_matches_component(self): + """#830 (Vicky-2418): iTunes packs a collab into ONE string. The + requested artist is one of several credited — must match. This is the + exact real case from the report (Narvent's 'Miss You (Ambient Remix)').""" + credit = ['TRVNSPORTER, Narvent & SKVLENT'] + assert track_artist_matches(credit, 'Narvent') is True + assert track_artist_matches(credit, 'TRVNSPORTER') is True + assert track_artist_matches(credit, 'SKVLENT') is True + + def test_combined_credit_still_drops_absent_artist(self): + """The #559 contamination guard survives: an artist genuinely absent + from a combined credit is still dropped.""" + assert track_artist_matches(['TRVNSPORTER, Narvent & SKVLENT'], 'Drake') is False + + def test_feat_forms_match_component(self): + for credit in (['Drake feat. Narvent'], ['Drake ft. Narvent'], + ['Drake featuring Narvent'], ['Drake x Narvent']): + assert track_artist_matches(credit, 'Narvent') is True + + def test_no_substring_false_positive(self): + """Component matching is exact per-name — a longer name that merely + contains the target must NOT match.""" + assert track_artist_matches(['Narventos'], 'Narvent') is False + + def test_band_name_with_internal_separator_still_matches_exactly(self): + """A real band name containing a separator still matches as the full + string (we keep the whole credit as a candidate alongside the split).""" + assert track_artist_matches(['Florence + the Machine'], 'Florence + the Machine') is True + def test_match_handles_whitespace_padding(self): """Trailing whitespace in either side mustn't break the match.""" assert track_artist_matches([' Drake '], 'Drake') is True @@ -87,12 +116,14 @@ class TestTrackArtistMatches: assert track_artist_matches([{'name': 'Drake'}], 'Drake') is True assert track_artist_matches([{'name': 'Random'}], 'Drake') is False - def test_substring_does_not_match(self): - """A song by "Drake & Future" should not match "Drake" via - substring — that's exactly the false-positive case the bug - report describes. Exact full-name match only.""" - assert track_artist_matches(['Drake & Future'], 'Drake') is False + def test_substring_does_not_match_but_component_does(self): + """No SUBSTRING matching — "Drakeo the Ruler" must not match "Drake". + But a combined collab credit IS component-matched (#830): "Drake & + Future" matches "Drake" because Drake is genuinely one of the credited + artists. The #559 guard drops artists who aren't credited AT ALL, not + legit collaborators packed into one string by sources like iTunes.""" assert track_artist_matches(['Drakeo the Ruler'], 'Drake') is False + assert track_artist_matches(['Drake & Future'], 'Drake') is True # --------------------------------------------------------------------------- diff --git a/tests/metadata/test_multi_artist_tag_settings.py b/tests/metadata/test_multi_artist_tag_settings.py index b8de390f..dc835bed 100644 --- a/tests/metadata/test_multi_artist_tag_settings.py +++ b/tests/metadata/test_multi_artist_tag_settings.py @@ -561,3 +561,93 @@ class TestDeezerDirectDownloadFlow: assert meta["_artists_list"] == ["FAYAN", "Dalton"] assert meta["artist"] == "FAYAN;Dalton" assert meta["title"] == "VERLIEBT IN MICH" + + +class TestSourceResolutionFromRealDownloadPayload: + """Netti93 round 3: the prior tests set context['source'] — a field the + REAL Search → Download Now flow never had. The serialized search results + carried no source at all, so get_import_source() resolved '' and the + Deezer contributors upgrade never ran on direct downloads (single-artist + tags until a Retag). These pin the actual payload shapes end to end.""" + + def _staging_context(self, track_info): + # Shape built by core/downloads/staging.py:457 for a stream download. + return { + "track_info": track_info, + "spotify_artist": {"name": "August Burns Red", "id": None}, + "spotify_album": {"name": "Death Below"}, + "original_search_result": { + "username": "tidal", "filename": "/x.flac", + "title": "Sonic Salvation", "artist": "August Burns Red", + "spotify_clean_title": "Sonic Salvation", + "spotify_clean_album": "Death Below", + "spotify_clean_artist": "August Burns Red", + "track_number": 1, "disc_number": 1, + }, + "is_album_download": False, + "staging_source": True, + } + + def _fake_deezer(self): + return SimpleNamespace(get_track_details=MagicMock(return_value={ + "id": "3966840171", "name": "Sonic Salvation", + "artists": ["August Burns Red", "Polaris"], + })) + + def test_source_in_track_info_triggers_upgrade(self): + """The fixed flow: serialized search result carries source='deezer' + inside track_info (no context-level source) → upgrade fires.""" + from core.metadata import source as src_module + + track_info = { + "id": "3966840171", "name": "Sonic Salvation", + "source": "deezer", + "artists": ["August Burns Red"], + "album": {"name": "Death Below", "id": None}, + } + fake_deezer = self._fake_deezer() + with patch.object(src_module, "get_config_manager", + return_value=_make_cfg({"metadata_enhancement.tags.write_multi_artist": True})), \ + patch("core.metadata.get_deezer_client", return_value=fake_deezer): + meta = src_module.extract_source_metadata( + self._staging_context(track_info), {"name": "August Burns Red", "id": ""}, {}) + + assert meta["_artists_list"] == ["August Burns Red", "Polaris"] + fake_deezer.get_track_details.assert_called_once_with("3966840171") + + def test_underscore_source_shape_also_triggers_upgrade(self): + """Discography/wishlist payloads carry '_source' instead of 'source' — + get_import_source must honor that shape too.""" + from core.metadata import source as src_module + + track_info = { + "id": "3966840171", "name": "Sonic Salvation", + "_source": "deezer", + "artists": ["August Burns Red"], + } + fake_deezer = self._fake_deezer() + with patch.object(src_module, "get_config_manager", return_value=_make_cfg()), \ + patch("core.metadata.get_deezer_client", return_value=fake_deezer): + meta = src_module.extract_source_metadata( + self._staging_context(track_info), {"name": "August Burns Red", "id": ""}, {}) + + assert meta["_artists_list"] == ["August Burns Red", "Polaris"] + fake_deezer.get_track_details.assert_called_once_with("3966840171") + + def test_sourceless_payload_still_no_upgrade(self): + """A payload with no source anywhere (pre-fix shape) must not crash — + and must not call the Deezer API blindly.""" + from core.metadata import source as src_module + + track_info = { + "id": "3966840171", "name": "Sonic Salvation", + "artists": ["August Burns Red"], + } + fake_deezer = self._fake_deezer() + with patch.object(src_module, "get_config_manager", return_value=_make_cfg()), \ + patch("core.metadata.get_deezer_client", return_value=fake_deezer): + meta = src_module.extract_source_metadata( + self._staging_context(track_info), {"name": "August Burns Red", "id": ""}, {}) + + assert meta["_artists_list"] == ["August Burns Red"] + fake_deezer.get_track_details.assert_not_called() diff --git a/tests/search/test_search_by_id.py b/tests/search/test_search_by_id.py index 172cb5b8..07f1a425 100644 --- a/tests/search/test_search_by_id.py +++ b/tests/search/test_search_by_id.py @@ -449,3 +449,53 @@ def test_resolve_get_album_returning_none_yields_not_found(): ) assert res['available'] is False assert res['albums'] == [] + + +# ── Discogs (#813 — extend paste-link to Discogs) ────────────────────────── + +def test_parse_discogs_release_url_strips_slug(): + out = by_id.parse_metadata_identifier( + 'https://www.discogs.com/release/678910-Some-Album-Title') + assert out == [by_id.LookupTarget('discogs', 'album', '678910')] + + +def test_parse_discogs_master_url_is_album(): + out = by_id.parse_metadata_identifier( + 'https://www.discogs.com/master/555-A-Master') + assert out == [by_id.LookupTarget('discogs', 'album', '555')] + + +def test_parse_discogs_artist_url(): + out = by_id.parse_metadata_identifier( + 'https://www.discogs.com/artist/12345-Some-Artist') + assert out == [by_id.LookupTarget('discogs', 'artist', '12345')] + + +def test_parse_discogs_url_no_scheme(): + out = by_id.parse_metadata_identifier('discogs.com/release/999-X') + assert out == [by_id.LookupTarget('discogs', 'album', '999')] + + +def test_resolve_discogs_release_uses_get_album_with_numeric_id(): + client = _FakeClient(album={'id': '678910', 'name': 'Some Album', + 'artists': [{'name': 'Some Artist'}]}) + res = by_id.resolve_identifier( + 'https://www.discogs.com/release/678910-Some-Album-Title', deps=None, + client_resolver=_resolver_from({'discogs': client})) + assert res['available'] is True and res['source'] == 'discogs' + assert res['albums'] and res['albums'][0]['name'] == 'Some Album' + assert client.album_calls == ['678910'] # numeric id, slug stripped + + +def test_resolve_discogs_artist(): + client = _FakeClient(artist={'id': '12345', 'name': 'Some Artist'}) + res = by_id.resolve_identifier( + 'https://www.discogs.com/artist/12345-Some-Artist', deps=None, + client_resolver=_resolver_from({'discogs': client})) + assert res['available'] is True and res['source'] == 'discogs' + assert res['artists'] and res['artists'][0]['name'] == 'Some Artist' + assert client.artist_calls == ['12345'] + + +def test_discogs_in_supported_sources(): + assert 'discogs' in by_id.SUPPORTED_SOURCES diff --git a/tests/search/test_search_sources.py b/tests/search/test_search_sources.py index f6949027..eb5fb7fd 100644 --- a/tests/search/test_search_sources.py +++ b/tests/search/test_search_sources.py @@ -84,6 +84,7 @@ def test_search_kind_artists_returns_normalized_dicts(): assert result == [{ 'id': 'id1', 'name': 'Pink Floyd', + 'source': 'spotify', 'image_url': 'thumb.jpg', 'external_urls': {'spotify': 'url'}, }] @@ -137,6 +138,8 @@ def test_search_kind_tracks_returns_full_shape(): 'id': 't1', 'name': 'Money', 'artist': 'Pink Floyd', + 'artists': ['Pink Floyd'], + 'source': '', 'album': 'DSOTM', 'duration_ms': 383000, 'image_url': 'm.jpg', @@ -201,3 +204,45 @@ def test_search_source_all_fail_returns_empty_lists(): client = _Client(fail={'artists', 'albums', 'tracks'}) result = sources.search_source('q', client, 'spotify') assert result == {'artists': [], 'albums': [], 'tracks': [], 'available': True} + + +# ── source field on serialized results (Netti93 multi-artist fix) ─────────── +# Search results must carry which metadata source they came from: the payload +# travels Download Now → download task → import context, where the Deezer +# contributors upgrade (multi-artist tags) is gated on source == 'deezer'. +# Without it get_import_source() resolved '' and collab tracks were tagged +# with only the primary artist until a Retag. + +def _full_client(): + return _Client( + artists=[_Artist('id1', 'A')], + albums=[_Album('a1', 'Album', artists=['A'])], + tracks=[_Track('t1', 'T', artists=['A'], album='Album')], + ) + + +def test_serialized_tracks_carry_source(): + out = sources.search_kind(_full_client(), "q", "tracks", source_name="deezer") + assert out and all(t["source"] == "deezer" for t in out) + + +def test_serialized_albums_and_artists_carry_source(): + albums = sources.search_kind(_full_client(), "q", "albums", source_name="deezer") + artists = sources.search_kind(_full_client(), "q", "artists", source_name="deezer") + assert albums and all(a["source"] == "deezer" for a in albums) + assert artists and all(a["source"] == "deezer" for a in artists) + + +def test_serialized_source_empty_when_unnamed(): + # hydrabase path calls without a source_name — emit '' not the class name. + out = sources.search_kind(_full_client(), "q", "tracks") + assert out and all(t["source"] == "" for t in out) + + +def test_serialized_tracks_carry_real_artists_list(): + """Collabs must survive as a LIST — the joined "A, B" display string made + downloads tag a single combined artist (Marcus's report).""" + client = _Client(tracks=[_Track('t1', 'Collab', artists=['Artist A', 'Artist B'], album='X')]) + out = sources.search_kind(client, 'q', 'tracks', source_name='spotify') + assert out[0]['artists'] == ['Artist A', 'Artist B'] + assert out[0]['artist'] == 'Artist A, Artist B' # display string unchanged diff --git a/tests/test_art_apply.py b/tests/test_art_apply.py index 2be5d9e4..d9177fb6 100644 --- a/tests/test_art_apply.py +++ b/tests/test_art_apply.py @@ -109,7 +109,7 @@ def test_apply_embeds_into_each_file_and_writes_cover(tmp_path, monkeypatch): monkeypatch.setattr(aa, 'embed_album_art_metadata', lambda a, m: embed_calls.append(m) or True) # download_cover_art is the standard cover.jpg writer — stub it to drop one. monkeypatch.setattr(aa, 'download_cover_art', - lambda album_info, folder, ctx=None: open(f"{folder}/cover.jpg", 'wb').close()) + lambda album_info, folder, ctx=None, **k: open(f"{folder}/cover.jpg", 'wb').close()) meta = {'artist': 'A', 'album': 'B', 'album_art_url': 'http://x/y.jpg'} res = aa.apply_art_to_album_files([str(f1), str(f2)], meta, {'album_name': 'B'}, folder=str(tmp_path)) @@ -194,6 +194,27 @@ def test_apply_flags_erofs_from_actual_write(tmp_path, monkeypatch): assert res['failed'] == 2 # first EROFS fails it + bails the rest +def test_cover_only_read_only_is_detected(tmp_path, monkeypatch): + """The gap that left Sokhi stuck (#804): tracks already have art so the + embed loop is skipped, but the cover.jpg write hits a read-only mount. + download_cover_art swallows that EROFS onto the context — apply must still + surface read_only_fs (not report a silent success).""" + f = tmp_path / 'a.flac'; f.write_bytes(b'') + audio = SimpleNamespace(pictures=['pic'], tags={'ok': 1}) # already has art → embed skipped + monkeypatch.setattr(aa, 'get_mutagen_symbols', lambda: _fake_symbols(audio)) + + def _fake_download(album_info, target_dir, ctx, **k): + # mimic download_cover_art's EROFS handling: record on context, swallow + if isinstance(ctx, dict): + ctx['_cover_read_only'] = True + monkeypatch.setattr(aa, 'download_cover_art', _fake_download) + + res = aa.apply_art_to_album_files([str(f)], {}, {}, folder=str(tmp_path)) + + assert res['embedded'] == 0 and res['skipped'] == 1 # nothing embedded + assert res['read_only_fs'] is True # but read-only surfaced + + def test_apply_normal_failure_not_flagged_read_only(tmp_path, monkeypatch): def _boom(): raise PermissionError(13, 'Permission denied') @@ -210,3 +231,106 @@ def test_apply_normal_failure_not_flagged_read_only(tmp_path, monkeypatch): assert res['failed'] == 1 + + +def test_filler_forces_cover_sidecar_write(tmp_path, monkeypatch): + # The Cover Art Filler must write cover.jpg regardless of the import-time + # "Download cover.jpg" toggle — so apply passes force=True (Sokhi). + f = tmp_path / 'a.mp3'; f.write_bytes(b'') + audio = SimpleNamespace(pictures=[], tags={'ok': 1}, save=lambda: None) + monkeypatch.setattr(aa, 'get_mutagen_symbols', lambda: _fake_symbols(audio)) + monkeypatch.setattr(aa, 'embed_album_art_metadata', lambda *a, **k: True) + captured = {} + monkeypatch.setattr(aa, 'download_cover_art', + lambda album_info, target_dir, ctx, **k: captured.update(k)) + + aa.apply_art_to_album_files([str(f)], {}, {}, folder=str(tmp_path)) + + assert captured.get('force') is True + + +# ── cover.jpg from embedded art (#813/Sokhi: files have art, no sidecar) ── + +def test_extract_embedded_art_flac(tmp_path, monkeypatch): + f = tmp_path / 'a.flac'; f.write_bytes(b'') + audio = SimpleNamespace(pictures=[SimpleNamespace(data=b'IMGBYTES')], tags=None) + monkeypatch.setattr(aa, 'get_mutagen_symbols', lambda: _fake_symbols(audio)) + assert aa.extract_embedded_art(str(f)) == b'IMGBYTES' + + +def test_extract_embedded_art_none_when_no_pictures(tmp_path, monkeypatch): + f = tmp_path / 'a.flac'; f.write_bytes(b'') + audio = SimpleNamespace(pictures=[], tags=None) + monkeypatch.setattr(aa, 'get_mutagen_symbols', lambda: _fake_symbols(audio)) + assert aa.extract_embedded_art(str(f)) is None + + +def test_apply_writes_cover_jpg_from_embedded_art(tmp_path, monkeypatch): + # Files already have embedded art, no cover.jpg → apply extracts it and + # writes the sidecar WITHOUT an API fetch (consistent + offline). + f = tmp_path / '01.flac'; f.write_bytes(b'') + audio = SimpleNamespace(pictures=[SimpleNamespace(data=b'EMBEDDED')], tags=None, save=lambda: None) + monkeypatch.setattr(aa, 'get_mutagen_symbols', lambda: _fake_symbols(audio)) + monkeypatch.setattr(aa, 'embed_album_art_metadata', lambda *a, **k: True) + dl_called = [] + monkeypatch.setattr(aa, 'download_cover_art', lambda *a, **k: dl_called.append(1)) + + res = aa.apply_art_to_album_files([str(f)], {}, {}, folder=str(tmp_path)) + + assert (tmp_path / 'cover.jpg').read_bytes() == b'EMBEDDED' + assert res['cover_written'] is True + assert dl_called == [] # used embedded art — no API call + + +def test_apply_falls_back_to_download_when_no_embedded(tmp_path, monkeypatch): + # No embedded art to extract → fetch the sidecar via download_cover_art. + f = tmp_path / '01.flac'; f.write_bytes(b'') + audio = SimpleNamespace(pictures=[], tags=None, add_tags=lambda: None, save=lambda: None) + monkeypatch.setattr(aa, 'get_mutagen_symbols', lambda: _fake_symbols(audio)) + monkeypatch.setattr(aa, 'embed_album_art_metadata', lambda *a, **k: True) + dl_called = [] + + def fake_dl(album_info, target, ctx=None, **k): + dl_called.append(1) + open(f"{target}/cover.jpg", 'wb').close() + monkeypatch.setattr(aa, 'download_cover_art', fake_dl) + + res = aa.apply_art_to_album_files([str(f)], {'album_art_url': 'http://x/y.jpg'}, + {'album_name': 'B'}, folder=str(tmp_path)) + assert dl_called == [1] # no embedded art → API fetch + assert res['cover_written'] is True + + +def test_apply_skips_cover_when_sidecar_exists(tmp_path, monkeypatch): + # cover.jpg already present → don't extract or download. + (tmp_path / 'cover.jpg').write_bytes(b'EXISTING') + f = tmp_path / '01.flac'; f.write_bytes(b'') + audio = SimpleNamespace(pictures=[SimpleNamespace(data=b'X')], tags=None, save=lambda: None) + monkeypatch.setattr(aa, 'get_mutagen_symbols', lambda: _fake_symbols(audio)) + monkeypatch.setattr(aa, 'embed_album_art_metadata', lambda *a, **k: True) + dl_called = [] + monkeypatch.setattr(aa, 'download_cover_art', lambda *a, **k: dl_called.append(1)) + + aa.apply_art_to_album_files([str(f)], {}, {}, folder=str(tmp_path)) + assert (tmp_path / 'cover.jpg').read_bytes() == b'EXISTING' # untouched + assert dl_called == [] + + +def test_apply_cover_falls_back_to_file_dir_when_folder_doesnt_exist(tmp_path, monkeypatch): + # The Sokhi bug: the caller passed the raw DB folder (e.g. Jellyfin's + # /data/music/...) which doesn't exist in the container, so the cover.jpg + # write was silently skipped. Now it falls back to the real directory of + # the files and writes there anyway. + real_dir = tmp_path / 'real' + real_dir.mkdir() + f = real_dir / '01.flac'; f.write_bytes(b'') + audio = SimpleNamespace(pictures=[SimpleNamespace(data=b'EMB')], tags=None, save=lambda: None) + monkeypatch.setattr(aa, 'get_mutagen_symbols', lambda: _fake_symbols(audio)) + monkeypatch.setattr(aa, 'embed_album_art_metadata', lambda *a, **k: True) + monkeypatch.setattr(aa, 'download_cover_art', lambda *a, **k: None) + + res = aa.apply_art_to_album_files([str(f)], {}, {}, + folder='/data/music/does/not/exist/in/container') + + assert (real_dir / 'cover.jpg').read_bytes() == b'EMB' # written to the REAL dir + assert res['cover_written'] is True diff --git a/tests/test_atomic_audio_save.py b/tests/test_atomic_audio_save.py new file mode 100644 index 00000000..396dafb8 --- /dev/null +++ b/tests/test_atomic_audio_save.py @@ -0,0 +1,137 @@ +"""Atomic tag saves: an interrupted/OOM-killed save must never destroy the +user's file (#819 — CubeComming's hi-res FLACs imported to an empty shell). + +save_audio_file writes the modified tags into a temp COPY, verifies it's still +valid audio, then os.replace()s it in — the original is untouched until that +atomic swap. +""" + +from __future__ import annotations + +import os +import shutil +import subprocess +from types import SimpleNamespace + +import pytest + +from core.metadata.common import save_audio_file + + +def _symbols(file_length): + """Symbols whose File() reports the given decoded length (None → invalid).""" + return SimpleNamespace( + ID3=type("ID3", (), {}), FLAC=type("FLAC", (), {}), + File=lambda p: (None if file_length is None + else SimpleNamespace(info=SimpleNamespace(length=file_length))), + ) + + +def test_atomic_replace_on_success(tmp_path): + f = tmp_path / "song.flac" + f.write_bytes(b"ORIGINAL-AUDIO") + saved_to = [] + + class Audio: + filename = str(f) + tags = None + + def save(self, target=None, **k): + saved_to.append(target) + # mimic mutagen writing modified tags into the temp copy + with open(target, "ab") as h: + h.write(b"+TAGS") + + save_audio_file(Audio(), _symbols(180.0)) + + assert f.read_bytes() == b"ORIGINAL-AUDIO+TAGS" # replaced with the tagged copy + assert saved_to == [str(f) + ".sstmp"] # wrote the temp, NOT in place + assert not (tmp_path / "song.flac.sstmp").exists() # temp cleaned up + + +def test_original_survives_save_failure(tmp_path): + # The #819 scenario: the save blows up mid-write. The original must be intact. + f = tmp_path / "song.flac" + f.write_bytes(b"ORIGINAL") + inplace = [] + + class Audio: + filename = str(f) + tags = None + + def save(self, target=None, **k): + if target is not None: + raise OSError("simulated interrupted/OOM save") + inplace.append(True) # fallback in-place save (writes nothing here) + + save_audio_file(Audio(), _symbols(180.0)) + + assert f.read_bytes() == b"ORIGINAL" # never destroyed + assert not (tmp_path / "song.flac.sstmp").exists() # temp removed + assert inplace == [True] # fell back to in-place + + +def test_corrupt_temp_rejected(tmp_path): + # save-to-temp "succeeds" but produces a file with no audio → must NOT + # replace the original; fall back instead. + f = tmp_path / "song.flac" + f.write_bytes(b"ORIGINAL") + inplace = [] + + class Audio: + filename = str(f) + tags = None + + def save(self, target=None, **k): + if target is None: + inplace.append(True) + + save_audio_file(Audio(), _symbols(0)) # File().info.length == 0 → invalid + + assert f.read_bytes() == b"ORIGINAL" + assert not (tmp_path / "song.flac.sstmp").exists() + assert inplace == [True] + + +def test_no_filename_plain_save(): + saves = [] + + class Audio: + filename = None + tags = None + + def save(self, target=None, **k): + saves.append(target) + + save_audio_file(Audio(), _symbols(180.0)) + assert saves == [None] # nothing to be atomic about → plain in-place + + +# ── real mutagen round-trip (only if ffmpeg can make a FLAC) ── + +def _make_flac(path): + ff = shutil.which("ffmpeg") + if not ff: + return False + r = subprocess.run( + [ff, "-f", "lavfi", "-i", "sine=frequency=440:duration=1", "-y", str(path)], + capture_output=True) + return r.returncode == 0 and os.path.getsize(path) > 0 + + +def test_real_flac_atomic_save_preserves_audio(tmp_path): + from mutagen.flac import FLAC + f = tmp_path / "real.flac" + if not _make_flac(f): + pytest.skip("ffmpeg unavailable — cannot build a real FLAC") + orig_len = FLAC(str(f)).info.length + + audio = FLAC(str(f)) + audio["title"] = "Atomic Test" + save_audio_file(audio, SimpleNamespace(ID3=type("ID3", (), {}), FLAC=FLAC, + File=__import__("mutagen").File)) + + reread = FLAC(str(f)) + assert reread.info.length == pytest.approx(orig_len, abs=0.05) # audio intact + assert reread["title"] == ["Atomic Test"] # tag written + assert not (tmp_path / "real.flac.sstmp").exists() diff --git a/tests/test_cover_art_targets.py b/tests/test_cover_art_targets.py index 3c4de992..0538fd8d 100644 --- a/tests/test_cover_art_targets.py +++ b/tests/test_cover_art_targets.py @@ -128,3 +128,63 @@ def test_artist_action_without_found_artist_url_fails_cleanly(tmp_path): assert res['success'] is False album_thumb, artist_thumb = _thumbs(w) assert artist_thumb == 'http://old/artist.jpg' # nothing changed + + +# ── apply-result message accuracy (Sokhi/Boulder: "read-only?" on writable fs) ── + +def _add_track(w, path): + conn = w.db._get_connection() + c = conn.cursor() + c.execute("INSERT INTO tracks VALUES ('t1', 'al1', ?)", (str(path),)) + conn.commit() + conn.close() + + +def _apply_returns(monkeypatch, **art_result): + import core.metadata.art_apply as aa + base = {'embedded': 0, 'failed': 0, 'skipped': 0, 'cover_written': False, 'read_only_fs': False} + base.update(art_result) + monkeypatch.setattr(aa, 'apply_art_to_album_files', lambda *a, **k: base) + + +def test_already_arted_reports_present_not_readonly(tmp_path, monkeypatch): + # The bug: all files already had art (skipped) → embedded 0, cover 0 → the + # old message cried "(read-only?)" on a perfectly writable library. + w = _worker(tmp_path) + f = tmp_path / 'song.mp3'; f.write_bytes(b'x') + _add_track(w, f) + _apply_returns(monkeypatch, skipped=1) + res = w._fix_missing_cover_art('album', 'al1', None, {**DETAILS, '_fix_action': 'album'}) + assert res['success'] is True + assert 'already present' in res['message'].lower() + assert 'read-only' not in res['message'].lower() + + +def test_failed_writes_blame_permissions_not_readonly(tmp_path, monkeypatch): + w = _worker(tmp_path) + f = tmp_path / 'song.mp3'; f.write_bytes(b'x') + _add_track(w, f) + _apply_returns(monkeypatch, failed=1) + res = w._fix_missing_cover_art('album', 'al1', None, {**DETAILS, '_fix_action': 'album'}) + assert res['success'] is True + assert 'permission' in res['message'].lower() + assert 'read-only' not in res['message'].lower() + + +def test_genuine_read_only_still_hard_fails(tmp_path, monkeypatch): + w = _worker(tmp_path) + f = tmp_path / 'song.mp3'; f.write_bytes(b'x') + _add_track(w, f) + _apply_returns(monkeypatch, read_only_fs=True) + res = w._fix_missing_cover_art('album', 'al1', None, {**DETAILS, '_fix_action': 'album'}) + assert res['success'] is False + assert 'read-only' in res['error'].lower() + + +def test_embedded_success_message(tmp_path, monkeypatch): + w = _worker(tmp_path) + f = tmp_path / 'song.mp3'; f.write_bytes(b'x') + _add_track(w, f) + _apply_returns(monkeypatch, embedded=1) + res = w._fix_missing_cover_art('album', 'al1', None, {**DETAILS, '_fix_action': 'album'}) + assert res['success'] is True and 'embedded into 1' in res['message'] diff --git a/tests/test_dead_file_cleaner_guard.py b/tests/test_dead_file_cleaner_guard.py new file mode 100644 index 00000000..1ea90afe --- /dev/null +++ b/tests/test_dead_file_cleaner_guard.py @@ -0,0 +1,135 @@ +"""Dead File Cleaner — mass-false-positive guard (#828). + +macstainless: a Plex-on-macOS user running SoulSync in Docker had all 5,250 +tracks flagged "dead" because their stored /Volumes/... paths don't exist inside +the container. The resolver returning None means "couldn't find it at any known +base dir" — for a mis-mounted library that's EVERY track, not a real deletion. +The job now refuses to flag when a large fraction is unresolvable (a path-mapping +problem) and reports it as such, mirroring the existing transfer-folder abort. +""" + +from __future__ import annotations + +from core.repair_jobs.base import JobContext +from core.repair_jobs.dead_file_cleaner import DeadFileCleanerJob + + +class _Cur: + def __init__(self, rows): + self._rows = rows + + def execute(self, *a, **k): + pass + + def fetchall(self): + return self._rows + + def fetchone(self): + return [len(self._rows)] + + def close(self): + pass + + +class _Conn: + def __init__(self, rows): + self._rows = rows + + def cursor(self): + return _Cur(self._rows) + + def close(self): + pass + + +class _Db: + def __init__(self, rows): + self._rows = rows + + def _get_connection(self): + return _Conn(self._rows) + + +class _Cfg: + def __init__(self, overrides=None): + self._o = overrides or {} + + def get(self, key, default=None): + return self._o.get(key, default) + + +def _row(i, path): + # (track_id, title, artist, album, file_path, album_thumb, artist_thumb) + return (i, f"Track {i}", "Yellowcard", "Ocean Avenue", path, None, None) + + +def _run(rows, transfer_folder, cfg_overrides=None): + findings = [] + cfg = _Cfg({'soulseek.download_path': '', **(cfg_overrides or {})}) + ctx = JobContext( + db=_Db(rows), + transfer_folder=str(transfer_folder), + config_manager=cfg, + create_finding=lambda **kw: (findings.append(kw) or True), + ) + res = DeadFileCleanerJob().scan(ctx) + return res, findings + + +def test_mass_unresolvable_aborts_without_findings(tmp_path): + # 30 tracks all pointing to a /Volumes path that doesn't exist in this env + # -> systemic path problem -> abort, zero findings, one error. + rows = [_row(i, f"/Volumes/Core/Music/Plex/Yellowcard/{i}.mp3") for i in range(30)] + res, findings = _run(rows, tmp_path) + assert findings == [] + assert res.findings_created == 0 + assert res.errors >= 1 + assert res.scanned == 30 + + +def test_few_unresolvable_creates_findings(tmp_path): + # 4 real (resolvable) files + 1 genuinely missing -> fraction 0.2 < 0.5 -> + # the one dead file IS reported. + rows = [] + for i in range(4): + f = tmp_path / f"real_{i}.mp3" + f.write_text("x") + rows.append(_row(i, str(f))) + rows.append(_row(99, "/no/such/path/dead.mp3")) + res, findings = _run(rows, tmp_path, + {'repair.jobs.dead_file_cleaner.min_tracks_for_guard': 4}) + assert res.findings_created == 1 + assert len(findings) == 1 + assert findings[0]['entity_id'] == '99' + assert res.errors == 0 + + +def test_small_library_all_dead_still_reports(tmp_path): + # 3 dead tracks, below the default min_tracks_for_guard (25) -> guard doesn't + # apply -> all 3 reported (a tiny library can legitimately be all-dead). + rows = [_row(i, f"/no/such/{i}.mp3") for i in range(3)] + res, findings = _run(rows, tmp_path) + assert res.findings_created == 3 + + +def test_guard_thresholds_configurable(tmp_path): + # Lower min to 4; all 4 dead -> fraction 1.0 >= 0.5 -> abort. + rows = [_row(i, f"/no/such/{i}.mp3") for i in range(4)] + res, findings = _run(rows, tmp_path, + {'repair.jobs.dead_file_cleaner.min_tracks_for_guard': 4}) + assert res.findings_created == 0 + assert res.errors >= 1 + + +def test_healthy_library_no_abort_no_findings(tmp_path): + # 30 fully-resolvable tracks -> 0 dead -> neither aborts nor flags anything. + rows = [] + for i in range(30): + f = tmp_path / f"ok_{i}.mp3" + f.write_text("x") + rows.append(_row(i, str(f))) + res, findings = _run(rows, tmp_path) + assert res.findings_created == 0 + assert res.errors == 0 + assert res.scanned == 30 + assert findings == [] diff --git a/tests/test_existing_album_folder.py b/tests/test_existing_album_folder.py new file mode 100644 index 00000000..24b6f82b --- /dev/null +++ b/tests/test_existing_album_folder.py @@ -0,0 +1,114 @@ +"""Reuse an album's existing on-disk folder for new downloads (#829). + +Tacobell444: tracks added to an album across batches split into different folders +when $albumtype/$year drift. The resolver finds the album's existing single +folder (under the transfer dir) so the new track joins it. These pin the safety +rails: strict match, transfer-dir-only, single-folder-only, id-first. +""" + +from __future__ import annotations + +import os +from types import SimpleNamespace + +from core.library.existing_album_folder import resolve_existing_album_folder + + +class _FakeDb: + def __init__(self, album=None, album_conf=0.0, tracks=None, by_spotify=None): + self._album = album + self._album_conf = album_conf + self._tracks = tracks or [] + self._by_spotify = by_spotify + + def get_album_by_spotify_album_id(self, sid): + return self._by_spotify + + def check_album_exists_with_editions(self, title, artist, confidence_threshold=0.8, + expected_track_count=None, server_source=None, **kw): + return (self._album, self._album_conf) + + def get_tracks_by_album(self, album_id): + return self._tracks + + +def _track(path): + return SimpleNamespace(file_path=path) + + +def _album(id=1, title="Ocean Avenue"): + return SimpleNamespace(id=id, title=title) + + +def _mkfile(folder, name): + folder.mkdir(parents=True, exist_ok=True) + f = folder / name + f.write_text("x") + return str(f) + + +def test_reuses_single_folder_under_transfer(tmp_path): + album_dir = tmp_path / "Yellowcard - Ocean Avenue" + f1 = _mkfile(album_dir, "01 - Way Away.mp3") + f2 = _mkfile(album_dir, "02 - Breathing.mp3") + db = _FakeDb(album=_album(), album_conf=0.95, tracks=[_track(f1), _track(f2)]) + out = resolve_existing_album_folder( + db=db, transfer_dir=str(tmp_path), + album_name="Ocean Avenue", album_artist="Yellowcard") + assert out == os.path.normpath(str(album_dir)) + + +def test_no_match_returns_none(tmp_path): + db = _FakeDb(album=None, album_conf=0.0) + assert resolve_existing_album_folder( + db=db, transfer_dir=str(tmp_path), album_name="X", album_artist="Y") is None + + +def test_below_strict_threshold_returns_none(tmp_path): + f = _mkfile(tmp_path / "A", "1.mp3") + # 0.80 < the resolver's 0.85 strict gate -> not reused. + db = _FakeDb(album=_album(), album_conf=0.80, tracks=[_track(f)]) + assert resolve_existing_album_folder( + db=db, transfer_dir=str(tmp_path), album_name="A", album_artist="B") is None + + +def test_multi_folder_defers_to_template(tmp_path): + f1 = _mkfile(tmp_path / "Album" / "Disc 01", "1.mp3") + f2 = _mkfile(tmp_path / "Album" / "Disc 02", "1.mp3") + db = _FakeDb(album=_album(), album_conf=0.95, tracks=[_track(f1), _track(f2)]) + assert resolve_existing_album_folder( + db=db, transfer_dir=str(tmp_path), album_name="A", album_artist="B") is None + + +def test_folder_outside_transfer_returns_none(tmp_path): + f = _mkfile(tmp_path / "outside", "1.mp3") + transfer = tmp_path / "transfer" + transfer.mkdir() + db = _FakeDb(album=_album(), album_conf=0.95, tracks=[_track(f)]) + assert resolve_existing_album_folder( + db=db, transfer_dir=str(transfer), album_name="A", album_artist="B") is None + + +def test_id_first_match_skips_name_lookup(tmp_path): + album_dir = tmp_path / "Album" + f = _mkfile(album_dir, "1.mp3") + # name match would FAIL (album=None); the stored spotify id hits. + db = _FakeDb(album=None, album_conf=0.0, tracks=[_track(f)], by_spotify=_album()) + out = resolve_existing_album_folder( + db=db, transfer_dir=str(tmp_path), spotify_album_id="sp123", + album_name="X", album_artist="Y") + assert out == os.path.normpath(str(album_dir)) + + +def test_missing_transfer_dir_returns_none(tmp_path): + db = _FakeDb(album=_album(), album_conf=0.95, tracks=[]) + assert resolve_existing_album_folder( + db=db, transfer_dir=str(tmp_path / "nope"), album_name="A", album_artist="B") is None + + +def test_album_with_no_files_on_disk_returns_none(tmp_path): + # Album matched but its tracks have no resolvable file -> nothing to reuse. + db = _FakeDb(album=_album(), album_conf=0.95, + tracks=[_track("/gone/1.mp3"), _track(None)]) + assert resolve_existing_album_folder( + db=db, transfer_dir=str(tmp_path), album_name="A", album_artist="B") is None diff --git a/tests/test_jellyfin_album_thumb.py b/tests/test_jellyfin_album_thumb.py new file mode 100644 index 00000000..f04c3d21 --- /dev/null +++ b/tests/test_jellyfin_album_thumb.py @@ -0,0 +1,31 @@ +"""JellyfinAlbum must expose a cover-image thumb so the library scan stores +albums.thumb_url (mirroring JellyfinArtist). Without it the whole library reads +back with empty album thumbs — blank art in the UI + the Cover Art Filler +flagging every album as "missing cover art". +""" + +from __future__ import annotations + +from core.jellyfin_client import JellyfinAlbum, JellyfinArtist + + +class _Client: + pass + + +def test_album_has_primary_image_thumb(): + alb = JellyfinAlbum({'Id': 'abc123', 'Name': 'For You'}, _Client()) + assert alb.thumb == '/Items/abc123/Images/Primary' + + +def test_album_thumb_none_without_id(): + alb = JellyfinAlbum({'Name': 'No Id Album'}, _Client()) + assert alb.thumb is None + + +def test_album_thumb_matches_artist_shape(): + # Same URL shape the artist uses (already proven to display) — just the + # album's own item id. + art = JellyfinArtist({'Id': 'xyz', 'Name': 'A'}, _Client()) + alb = JellyfinAlbum({'Id': 'xyz', 'Name': 'B'}, _Client()) + assert alb.thumb == art.thumb diff --git a/tests/test_launch_lock_gate.py b/tests/test_launch_lock_gate.py new file mode 100644 index 00000000..1e897815 --- /dev/null +++ b/tests/test_launch_lock_gate.py @@ -0,0 +1,115 @@ +"""Server-side launch-PIN gate (#832, Beckid). + +The admin PIN was a client-side overlay only; removing the div gave full API +access. request_is_locked() is the pure allow/deny decision the before_request +gate uses. These pin the exact matrix so the lock can't silently regress to +'advisory'. +""" + +from __future__ import annotations + +import pytest + +from core.security.launch_lock import request_is_locked + + +def L(path, method='GET', require_pin=True, pin_verified=False): + return request_is_locked(path, method, require_pin=require_pin, pin_verified=pin_verified) + + +# ── gate disabled / already verified → never locks ────────────────────────── + +def test_no_lock_when_require_pin_off(): + # The default config: nothing is gated. + assert L('/api/downloads/start', 'POST', require_pin=False) is False + assert L('/api/settings', 'POST', require_pin=False) is False + + +def test_no_lock_when_session_verified(): + assert L('/api/downloads/start', 'POST', pin_verified=True) is False + assert L('/api/v1/api-keys-internal/generate', 'POST', pin_verified=True) is False + + +# ── locked session blocks the app ─────────────────────────────────────────── + +@pytest.mark.parametrize('path,method', [ + ('/api/watchlist/artists', 'GET'), + ('/api/downloads/start', 'POST'), + ('/api/settings', 'POST'), + ('/api/wishlist/tracks', 'GET'), + ('/api/library/artists', 'GET'), + ('/socket.io/', 'GET'), +]) +def test_locked_blocks_data_and_action_endpoints(path, method): + assert L(path, method) is True + + +def test_locked_blocks_profile_mutation(): + # Creating/editing/deleting profiles or setting PINs must NOT be reachable + # pre-auth — else an attacker mints an admin or rewrites the PIN. + assert L('/api/profiles', 'POST') is True # create + assert L('/api/profiles/2', 'PUT') is True # edit + assert L('/api/profiles/2', 'DELETE') is True # delete + assert L('/api/profiles/1/set-pin', 'POST') is True + + +def test_locked_blocks_internal_key_minting(): + # The documented "no auth required" key-management endpoints are the + # secondary bypass: mint a key, walk in via the public API. Must be locked. + assert L('/api/v1/api-keys-internal', 'GET') is True + assert L('/api/v1/api-keys-internal/generate', 'POST') is True + assert L('/api/v1/api-keys-internal/revoke/abc', 'DELETE') is True + + +# ── locked session still allows the unlock flow + shell ───────────────────── + +@pytest.mark.parametrize('path', ['/', '/static/init.js', '/static/dist/app.js', '/favicon.ico']) +def test_locked_allows_page_shell_and_assets(path): + assert L(path) is False + + +def test_locked_allows_unlock_flow(): + assert L('/api/profiles/current', 'GET') is False + assert L('/api/profiles', 'GET') is False # picker list + assert L('/api/profiles/select', 'POST') is False + assert L('/api/profiles/verify-launch-pin', 'POST') is False + assert L('/api/profiles/reset-pin-via-credential', 'POST') is False + assert L('/api/profiles/logout', 'POST') is False + + +def test_locked_allows_keyauthed_public_api(): + # The public REST API carries its own @require_api_key, so a launch-locked + # UI must not break a legitimate headless key holder. + assert L('/api/v1/search', 'GET') is False + assert L('/api/v1/playlists', 'GET') is False + + +def test_method_matters_for_shared_paths(): + # GET /api/profiles is the picker (allowed); POST /api/profiles is create + # (blocked). Same path, opposite verdicts. + assert L('/api/profiles', 'GET') is False + assert L('/api/profiles', 'POST') is True + + +# ── blocked navigations bounce to the lock screen, not JSON (#832 follow-up) ── + +from core.security.launch_lock import is_html_navigation # noqa: E402 + + +def test_browser_navigation_is_detected(): + # Address-bar / link / refresh — gets redirected to the root lock screen. + assert is_html_navigation('GET', 'text/html,application/xhtml+xml,*/*', '') is True + assert is_html_navigation('GET', '*/*', 'navigate') is True + + +def test_programmatic_fetch_is_not_navigation(): + # fetch()/XHR want JSON so the frontend can react to the 401. + assert is_html_navigation('GET', '*/*', 'cors') is False + assert is_html_navigation('GET', 'application/json', 'same-origin') is False + assert is_html_navigation('GET', '', '') is False + + +def test_non_get_is_never_navigation(): + # A programmatic POST/DELETE always gets JSON, never a redirect. + assert is_html_navigation('POST', 'text/html', 'navigate') is False + assert is_html_navigation('DELETE', 'text/html', '') is False diff --git a/tests/test_missing_cover_art.py b/tests/test_missing_cover_art.py index c32ba009..4cbcaf64 100644 --- a/tests/test_missing_cover_art.py +++ b/tests/test_missing_cover_art.py @@ -281,3 +281,151 @@ def test_result_matches_unit(): # no artist on result → require exact title assert m({'title': 'Album'}, 'Album', 'Artist') assert not m({'title': 'Album Deluxe'}, 'Album', 'Artist') + + +# ── disk-art check must run on the RESOLVED path (flags-every-album bug) ── + +def _add_track(conn, path): + conn.execute( + "INSERT INTO tracks (id, album_id, file_path, disc_number, track_number) " + "VALUES (1, 1, ?, 1, 1)", (path,)) + conn.commit() + + +def test_scan_checks_disk_art_on_resolved_path(monkeypatch): + # Album already has a DB thumb (db not missing) and a track whose DB path + # only resolves via mapping. The disk-art check must run on the RESOLVED + # path — checking the raw path would fail on path-mapped setups and flag + # the whole library while the apply (which resolves) finds art present. + conn = _make_db((1, 'Album', 1, 'https://has/thumb', None, None, None, None, None)) + _add_track(conn, '/plex/raw/song.flac') + context = _make_context(conn) + checked = {} + monkeypatch.setattr(mca, 'resolve_library_file_path', + lambda raw, **k: '/resolved/song.flac' if raw == '/plex/raw/song.flac' else None) + monkeypatch.setattr(mca, 'file_has_embedded_art', + lambda p: checked.update(path=p) or True) + monkeypatch.setattr(mca, 'folder_has_cover_sidecar', lambda d: True) # has cover.jpg too + + result = mca.MissingCoverArtJob().scan(context) + + assert checked.get('path') == '/resolved/song.flac' # resolved, not raw + assert result.findings_created == 0 # embedded + cover.jpg → not flagged + + +def test_scan_unresolvable_path_not_flagged_disk_missing(monkeypatch): + # An unreachable file (resolve → None) must NOT be claimed as "missing disk + # art" — we can't know, so don't false-flag. (Album has a thumb already.) + conn = _make_db((1, 'Album', 1, 'https://has/thumb', None, None, None, None, None)) + _add_track(conn, '/gone/song.flac') + context = _make_context(conn) + monkeypatch.setattr(mca, 'resolve_library_file_path', lambda raw, **k: None) + called = [] + monkeypatch.setattr(mca, 'file_has_embedded_art', lambda p: called.append(p) or False) + monkeypatch.setattr(mca, 'folder_has_cover_sidecar', lambda d: called.append(d) or False) + + result = mca.MissingCoverArtJob().scan(context) + + assert result.findings_created == 0 # thumb present, disk unknown → not flagged + assert called == [] # never checked art on a None path + + +def test_local_album_with_embedded_and_sidecar_not_flagged(monkeypatch): + # Has BOTH embedded art AND a cover.jpg — nothing missing, even with an + # empty DB thumb cache. (Boulder: don't flag albums that already have art.) + conn = _make_db((1, 'Album', 1, '', None, None, None, None, None)) # empty thumb + _add_track(conn, '/music/Album/01.flac') + context = _make_context(conn) + monkeypatch.setattr(mca, 'resolve_library_file_path', lambda raw, **k: raw) + monkeypatch.setattr(mca, 'file_has_embedded_art', lambda p: True) + monkeypatch.setattr(mca, 'folder_has_cover_sidecar', lambda d: True) + + result = mca.MissingCoverArtJob().scan(context) + + assert result.findings_created == 0 # has both → not "missing" + assert result.skipped == 1 + + +def test_embedded_art_but_no_cover_jpg_is_flagged(monkeypatch): + # Sokhi: files HAVE embedded art but no cover.jpg sidecar. With cover.jpg + # enabled (default), it's flagged so the filler writes the sidecar — even + # when the API finds NO art (the apply extracts the embedded art). + conn = _make_db((1, 'Album', 1, 'https://has/thumb', None, None, None, None, None)) + _add_track(conn, '/music/Album/01.flac') + context = _make_context(conn) + monkeypatch.setattr(mca, 'resolve_library_file_path', lambda raw, **k: raw) + monkeypatch.setattr(mca, 'file_has_embedded_art', lambda p: True) # embedded present + monkeypatch.setattr(mca, 'folder_has_cover_sidecar', lambda d: False) # but no cover.jpg + monkeypatch.setattr(mca, 'get_primary_source', lambda: 'spotify') + monkeypatch.setattr(mca, 'get_client_for_source', lambda s: _FakeClient()) # API finds nothing + + result = mca.MissingCoverArtJob().scan(context) + assert result.findings_created == 1 # flagged for the missing sidecar + assert context.findings[0]['details']['sidecar_from_embedded'] is True + + +def test_local_album_without_file_art_still_flagged(monkeypatch): + # Local album whose files genuinely lack art → still flagged (real case). + # Give it a source id + findable art so a finding is created when flagged. + conn = _make_db((1, 'Album', 1, '', 'sp-album', None, None, None, None)) + _add_track(conn, '/music/Album/01.flac') + context = _make_context(conn) + monkeypatch.setattr(mca, 'resolve_library_file_path', lambda raw, **k: raw) + monkeypatch.setattr(mca, 'file_has_embedded_art', lambda p: False) # no embedded art + monkeypatch.setattr(mca, 'folder_has_cover_sidecar', lambda d: False) + monkeypatch.setattr(mca, 'get_primary_source', lambda: 'spotify') + monkeypatch.setattr(mca, 'get_client_for_source', lambda s: _FakeClient(album_image='https://img/x')) + + result = mca.MissingCoverArtJob().scan(context) + assert result.findings_created == 1 # files lack art → flagged + + +def test_media_server_only_album_empty_thumb_still_flagged(monkeypatch): + # No local files (media-server-only) + empty thumb → DB thumb is the only + # art, so still flag it. + conn = _make_db((1, 'Album', 1, '', 'sp-album', None, None, None, None)) # no track added + context = _make_context(conn) + monkeypatch.setattr(mca, 'get_primary_source', lambda: 'spotify') + monkeypatch.setattr(mca, 'get_client_for_source', lambda s: _FakeClient(album_image='https://img/x')) + + result = mca.MissingCoverArtJob().scan(context) + assert result.findings_created == 1 # media-server-only + empty thumb → flagged + + +def test_unresolved_path_falls_back_to_raw_when_file_exists(tmp_path, monkeypatch): + # Docker case (Sokhi): the path-mapping layer returns None, but the raw DB + # path is already a real file in the container. The scan must use it as-is — + # like the apply does (`_resolve_file_path(...) or p`) — so the album's + # folder is actually checked instead of the album being skipped. + track = tmp_path / 'Album' / '01.flac' + track.parent.mkdir() + track.write_bytes(b'') + conn = _make_db((1, 'Album', 1, 'https://has/thumb', None, None, None, None, None)) + _add_track(conn, str(track)) + context = _make_context(conn) + monkeypatch.setattr(mca, 'resolve_library_file_path', lambda raw, **k: None) # mapping fails + monkeypatch.setattr(mca, 'file_has_embedded_art', lambda p: True) # files have art + # real folder has no cover.jpg → should flag for the sidecar + monkeypatch.setattr(mca, 'get_primary_source', lambda: 'spotify') + monkeypatch.setattr(mca, 'get_client_for_source', lambda s: _FakeClient()) # API finds nothing + + result = mca.MissingCoverArtJob().scan(context) + + assert result.findings_created == 1 # raw path used → folder checked → flagged + assert context.findings[0]['details']['sidecar_from_embedded'] is True + + +def test_unresolved_path_with_no_real_file_still_skips(tmp_path, monkeypatch): + # Guard: the raw-path fallback must NOT fire for a path that isn't a real + # file (no false "local" on a genuinely media-server-only album). + conn = _make_db((1, 'Album', 1, 'https://has/thumb', None, None, None, None, None)) + _add_track(conn, '/does/not/exist/01.flac') + context = _make_context(conn) + monkeypatch.setattr(mca, 'resolve_library_file_path', lambda raw, **k: None) + called = [] + monkeypatch.setattr(mca, 'file_has_embedded_art', lambda p: called.append(p) or True) + + result = mca.MissingCoverArtJob().scan(context) + + assert result.findings_created == 0 # no real file, db has thumb → skipped + assert called == [] # never treated as local diff --git a/tests/test_path_resolve_confusables.py b/tests/test_path_resolve_confusables.py new file mode 100644 index 00000000..4d012d51 --- /dev/null +++ b/tests/test_path_resolve_confusables.py @@ -0,0 +1,96 @@ +"""Confusable-tolerant path resolution (#833, the-hang-man). + +The library DB stored a track title with a curly apostrophe (U+2019); the file +was written to disk with an ASCII one (U+0027). Delete rebuilt the unlink path +from the DB value, so os.path.exists missed and the file survived. find_on_disk +resolves the real on-disk name despite typographic confusables — with REAL temp +files, not mocks, so it exercises the actual byte-level mismatch. +""" + +from __future__ import annotations + +import os + +from core.library.path_resolve import fold_confusables, find_on_disk + +CURLY = chr(0x2019) # ’ right single quotation mark +ASCII = chr(0x27) # ' ascii apostrophe + + +# ── fold_confusables ──────────────────────────────────────────────────────── + +def test_curly_and_straight_apostrophe_fold_equal(): + assert fold_confusables(f"I{CURLY}m Upset") == fold_confusables(f"I{ASCII}m Upset") + assert fold_confusables(f"I{CURLY}m Upset") == "I'm Upset" + + +def test_other_confusables_fold(): + assert fold_confusables('Rock – Roll') == 'Rock - Roll' # en dash + assert fold_confusables('Rock — Roll') == 'Rock - Roll' # em dash + assert fold_confusables('A “B” C') == 'A "B" C' # smart quotes + + +def test_fold_preserves_case_and_plain_text(): + # Case must NOT be folded — case-sensitive datasets can hold names that + # differ only by case, and folding could pick the wrong file. + assert fold_confusables('Track NAME.mp3') == 'Track NAME.mp3' + assert fold_confusables('') == '' + + +# ── find_on_disk against real files ───────────────────────────────────────── + +def test_finds_ascii_file_from_curly_db_path(tmp_path): + # On disk: ASCII apostrophe. DB/query: curly. This is the exact #833 case. + album = tmp_path / 'Drake' / 'Scorpion' + album.mkdir(parents=True) + real = album / f"01 - I{ASCII}m Upset.mp3" + real.write_text('audio') + + suffix = ['Drake', 'Scorpion', f"01 - I{CURLY}m Upset.mp3"] + found = find_on_disk(str(tmp_path), suffix) + assert found is not None + assert os.path.samefile(found, real) + + +def test_exact_match_still_works(tmp_path): + real = tmp_path / 'Artist' / 'Album' / 'Track.mp3' + real.parent.mkdir(parents=True) + real.write_text('audio') + found = find_on_disk(str(tmp_path), ['Artist', 'Album', 'Track.mp3']) + assert found is not None and os.path.samefile(found, real) + + +def test_confusable_in_folder_component(tmp_path): + # The apostrophe can be in a folder name (album/artist), not just the file. + folder = tmp_path / f"Guns N{ASCII} Roses" + folder.mkdir() + real = folder / 'track.mp3' + real.write_text('audio') + found = find_on_disk(str(tmp_path), [f"Guns N{CURLY} Roses", 'track.mp3']) + assert found is not None and os.path.samefile(found, real) + + +def test_returns_none_for_genuinely_missing_file(tmp_path): + (tmp_path / 'Artist').mkdir() + assert find_on_disk(str(tmp_path), ['Artist', 'Nope.mp3']) is None + + +def test_does_not_match_a_different_track(tmp_path): + # Folding apostrophes must not collapse two genuinely different names. + (tmp_path / 'Some Other Song.mp3').write_text('x') + assert find_on_disk(str(tmp_path), [f"I{CURLY}m Upset.mp3"]) is None + + +def test_exact_wins_over_folded_when_both_present(tmp_path): + # If the byte-exact file exists, it's chosen even when a folded sibling also + # exists — no accidental cross-match. + exact = tmp_path / f"I{CURLY}m Upset.mp3" + other = tmp_path / f"I{ASCII}m Upset.mp3" + exact.write_text('curly') + other.write_text('ascii') + found = find_on_disk(str(tmp_path), [f"I{CURLY}m Upset.mp3"]) + assert found is not None and os.path.samefile(found, exact) + + +def test_bad_base_dir_returns_none(tmp_path): + assert find_on_disk(str(tmp_path / 'does-not-exist'), ['x.mp3']) is None diff --git a/tests/test_playlist_edit.py b/tests/test_playlist_edit.py index 8cc0e10d..7c402992 100644 --- a/tests/test_playlist_edit.py +++ b/tests/test_playlist_edit.py @@ -138,6 +138,15 @@ def test_normalize_keeps_reconcile_from_config(): assert normalize_sync_mode('', 'reconcile') == 'reconcile' +def test_normalize_keeps_append_from_config(): + # #823 — an AUTOMATED sync (mirrored auto-sync / Playlist Pipeline) passes no + # per-request mode, so it must resolve to the user's configured global mode + # instead of hardcoding 'replace' (which recreated the playlist + wiped its + # image/description). Default 'replace' users are unaffected. + assert normalize_sync_mode(None, 'append') == 'append' + assert normalize_sync_mode(None, 'replace') == 'replace' + + def test_normalize_request_overrides_config(): assert normalize_sync_mode('append', 'reconcile') == 'append' assert normalize_sync_mode('reconcile', 'replace') == 'reconcile' @@ -152,3 +161,38 @@ def test_normalize_falls_back_for_unknown(): def test_normalize_all_real_modes_pass_through(): for m in ('replace', 'append', 'reconcile'): assert normalize_sync_mode(m, 'replace') == m + + +# ── plan_playlist_append (#823 round 2) ────────────────────────────────────── +# The Jellyfin/Emby + Navidrome appends deduped on `t.id`, but their track +# wrappers only define `ratingKey` — the existing-ids set was always empty, so +# every sync re-appended the full matched list (every track N times, +# "skipped 0 already present"). The planner is the now-testable dedupe. + +from core.sync.playlist_edit import plan_playlist_append # noqa: E402 + + +def test_append_skips_already_present(): + # carlosjfcasero's case: second sync of an unchanged playlist adds NOTHING. + assert plan_playlist_append(['a', 'b', 'c'], ['a', 'b', 'c']) == [] + + +def test_append_adds_only_new_in_desired_order(): + assert plan_playlist_append(['a', 'c'], ['a', 'b', 'c', 'd']) == ['b', 'd'] + + +def test_append_to_empty_playlist_adds_all(): + assert plan_playlist_append([], ['a', 'b']) == ['a', 'b'] + + +def test_append_dedupes_within_desired(): + assert plan_playlist_append(['x'], ['a', 'a', 'x', 'b', 'a']) == ['a', 'b'] + + +def test_append_stringifies_ids(): + # Emby numeric ids may arrive as ints on one side and strings on the other. + assert plan_playlist_append([16607838], ['16607838', '999']) == ['999'] + + +def test_append_ignores_empty_ids(): + assert plan_playlist_append(['a'], ['', 'b']) == ['b'] diff --git a/tests/test_script_split_integrity.py b/tests/test_script_split_integrity.py index 4390f98a..c299d14f 100644 --- a/tests/test_script_split_integrity.py +++ b/tests/test_script_split_integrity.py @@ -52,7 +52,8 @@ SPLIT_MODULES = [ # Other JS files that exist in static/ but are NOT part of the split NON_SPLIT_JS = {"setup-wizard.js", "docs.js", "helper.js", "particles.js", "worker-orbs.js", - "enrichment-manager.js", "origin-history.js", "blocklist.js"} + "enrichment-manager.js", "origin-history.js", "blocklist.js", + "watchlist-history.js"} # Pre-existing duplicate helper functions that lived in the original monolith. # In a plain + diff --git a/webui/static/api-monitor.js b/webui/static/api-monitor.js index 0d51d85b..07641bd8 100644 --- a/webui/static/api-monitor.js +++ b/webui/static/api-monitor.js @@ -2830,12 +2830,10 @@ async function openWatchlistGlobalSettingsModal() { // Update options visibility based on toggle state toggleGlobalOverrideOptions(); - // Update toggle label border + // Reflect enabled state on the toggle card (styled in CSS) const toggleLabel = document.getElementById('global-override-toggle-label'); if (toggleLabel) { - toggleLabel.style.border = config.global_override_enabled - ? '2px solid rgba(29, 185, 84, 0.5)' - : '2px solid rgba(255, 255, 255, 0.1)'; + toggleLabel.classList.toggle('enabled', !!config.global_override_enabled); } // Show modal @@ -2867,12 +2865,10 @@ function toggleGlobalOverrideOptions() { options.style.pointerEvents = enabled ? 'auto' : 'none'; } - // Update toggle label border + // Reflect enabled state on the toggle card (styled in CSS) const toggleLabel = document.getElementById('global-override-toggle-label'); if (toggleLabel) { - toggleLabel.style.border = enabled - ? '2px solid rgba(29, 185, 84, 0.5)' - : '2px solid rgba(255, 255, 255, 0.1)'; + toggleLabel.classList.toggle('enabled', enabled); } } @@ -3144,7 +3140,7 @@ async function startWatchlistScan() { try { const button = document.getElementById('scan-watchlist-btn'); button.disabled = true; - button.textContent = 'Starting scan...'; + _wlSetChipLabel(button, 'Starting scan...'); button.classList.add('btn-processing'); const response = await fetch('/api/watchlist/scan', { @@ -3157,7 +3153,7 @@ async function startWatchlistScan() { throw new Error(data.error || 'Failed to start scan'); } - button.textContent = 'Scanning...'; + _wlSetChipLabel(button, 'Scanning...'); // Show cancel button const cancelBtn = document.getElementById('cancel-watchlist-scan-btn'); @@ -3180,7 +3176,7 @@ async function startWatchlistScan() { console.error('Error starting watchlist scan:', error); const button = document.getElementById('scan-watchlist-btn'); button.disabled = false; - button.textContent = 'Scan for New Releases'; + _wlSetChipLabel(button, 'Scan for New Releases'); button.classList.remove('btn-processing'); alert(`Error starting scan: ${error.message}`); } @@ -3189,6 +3185,91 @@ async function startWatchlistScan() { /** * Poll watchlist scan status */ +// #831 (Tacobell444): the scan summary said "New tracks: 19 • Added to +// wishlist: 10" with no way to see WHICH tracks. The scan now ships a per-run +// ledger (scan_track_events: track/artist/album/thumb + added|skipped) and +// this renders it as an expandable list under the completion summary. +function renderWatchlistScanTrackLedger(events) { + if (!Array.isArray(events) || events.length === 0) return ''; + const added = events.filter(e => e.status === 'added'); + const skipped = events.filter(e => e.status !== 'added'); + + const row = (e) => ` +
+ +
+
${escapeHtml(e.track_name || '')}
+
${escapeHtml(e.artist_name || '')}${e.album_name ? ' — ' + escapeHtml(e.album_name) : ''}
+
+ ${e.status === 'added' + ? 'added' + : 'skipped'} +
`; + + const section = (label, list) => list.length + ? `
${label} (${list.length})
${list.map(row).join('')}` + : ''; + + return ` + + `; +} + +// Human-readable phase line for the scan deck ("checking_album_2_of_5" → +// "Checking album 2 of 5"). +// Set a wl-chip button's label without destroying its icon/shimmer children +// (textContent wipes them; these buttons carry an svg + shimmer span). +function _wlSetChipLabel(btn, text) { + if (!btn) return; + const svg = btn.querySelector('svg'); + const shimmer = btn.querySelector('.wl-chip-shimmer'); + btn.textContent = ''; + if (svg) btn.appendChild(svg); + btn.appendChild(document.createTextNode(' ' + text)); + if (shimmer) btn.appendChild(shimmer); +} + +function _wlPrettyPhase(data) { + const phase = data.current_phase || ''; + if (!phase) return 'Working…'; + const m = phase.match(/^checking_album_(\d+)_of_(\d+)$/); + if (m) return `Checking album ${m[1]} of ${m[2]}`; + const map = { + starting: 'Starting…', + fetching_discography: 'Fetching releases…', + populating_discovery_pool: 'Populating discovery…', + updating_listenbrainz: 'Updating ListenBrainz…', + }; + return map[phase] || phase.replace(/_/g, ' '); +} + +// Update a live counter and replay its pop animation when the value changes. +function _wlBumpCounter(id, value) { + const el = document.getElementById(id); + if (!el) return; + const text = String(value); + if (el.textContent !== text) { + el.textContent = text; + el.classList.remove('pop'); + void el.offsetWidth; // restart the CSS animation + el.classList.add('pop'); + } +} + +function toggleWatchlistScanTracks(btn) { + const list = btn.parentElement.querySelector('.watchlist-scan-tracks'); + if (!list) return; + const open = list.style.display !== 'none'; + list.style.display = open ? 'none' : 'block'; + btn.innerHTML = (open ? 'Show tracks ' : 'Hide tracks ') + + `${open ? '▾' : '▴'}`; +} + function handleWatchlistScanData(data) { const button = document.getElementById('scan-watchlist-btn'); const liveActivity = document.getElementById('watchlist-live-activity'); @@ -3201,17 +3282,23 @@ function handleWatchlistScanData(data) { // Update live visual activity display if (liveActivity && data.status === 'scanning') { - liveActivity.style.display = 'flex'; + liveActivity.style.display = liveActivity.classList.contains('wl-scan-deck') ? 'block' : 'flex'; - // Update artist image and name + // Update artist image and name (hide the img when THIS artist has no + // photo, so the previous artist's portrait doesn't linger and the + // glyph placeholder shows instead) const artistImg = document.getElementById('watchlist-artist-img'); const artistName = document.getElementById('watchlist-artist-name'); - if (artistImg && data.current_artist_image_url) { - artistImg.src = data.current_artist_image_url; - artistImg.style.display = 'block'; + if (artistImg) { + if (data.current_artist_image_url) { + artistImg.src = data.current_artist_image_url; + artistImg.style.display = 'block'; + } else { + artistImg.style.display = 'none'; + } } if (artistName) { - artistName.textContent = data.current_artist_name || 'Processing...'; + artistName.textContent = data.current_artist_name || 'Starting…'; } // Update album image and name @@ -3224,30 +3311,54 @@ function handleWatchlistScanData(data) { albumImg.style.display = 'none'; } if (albumName) { - albumName.textContent = data.current_album || (data.current_phase === 'fetching_discography' ? 'Fetching releases...' : 'Processing...'); + albumName.textContent = data.current_album + || (data.current_phase === 'fetching_discography' ? 'Fetching releases…' : 'Looking for new releases…'); } - // Update current track + // Update current track ('' keeps the slot without echoing the album line) const trackName = document.getElementById('watchlist-track-name'); if (trackName) { - trackName.textContent = data.current_track_name || (data.current_phase === 'fetching_discography' ? 'Fetching releases...' : 'Processing...'); + trackName.textContent = data.current_track_name || '—'; } - // Update wishlist additions feed + // #831 round 2: scan-deck extras — artist progress bar, live counters, + // readable phase. All optional elements, so the legacy modal markup + // (which lacks them) keeps working untouched. + const total = data.total_artists || 0; + const idx = total ? Math.min((data.current_artist_index || 0) + 1, total) : 0; + const progText = document.getElementById('wl-scan-progress-text'); + if (progText) progText.textContent = total ? `${idx} / ${total} artists` : ''; + const progBar = document.getElementById('wl-scan-progress-bar'); + if (progBar && total) progBar.style.width = `${Math.round((100 * idx) / total)}%`; + const phaseEl = document.getElementById('wl-scan-phase'); + if (phaseEl) phaseEl.textContent = _wlPrettyPhase(data); + _wlBumpCounter('wl-scan-found', data.tracks_found_this_scan || 0); + _wlBumpCounter('wl-scan-added', data.tracks_added_this_scan || 0); + + // Update wishlist additions feed — only re-render when it actually + // changed, so the slide-in animation plays once per new track instead + // of replaying on every poll. const additionsFeed = document.getElementById('watchlist-additions-feed'); if (additionsFeed) { - if (data.recent_wishlist_additions && data.recent_wishlist_additions.length > 0) { - additionsFeed.innerHTML = data.recent_wishlist_additions.map(item => ` -
- -
-
${item.track_name}
-
${item.artist_name}
+ const additions = data.recent_wishlist_additions || []; + const feedKey = additions.map(a => a.track_name).join(''); + if (additionsFeed.dataset.feedKey !== feedKey) { + const grew = additions.length > 0 && additionsFeed.dataset.feedKey !== undefined + && feedKey.length > (additionsFeed.dataset.feedKey || '').length; + additionsFeed.dataset.feedKey = feedKey; + if (additions.length > 0) { + additionsFeed.innerHTML = additions.map((item, i) => ` +
+ +
+
${escapeHtml(item.track_name || '')}
+
${escapeHtml(item.artist_name || '')}
+
-
- `).join(''); - } else { - additionsFeed.innerHTML = '
No tracks added yet...
'; + `).join(''); + } else { + additionsFeed.innerHTML = '
No tracks added yet…
'; + } } } } else if (liveActivity && data.status !== 'scanning') { @@ -3257,7 +3368,7 @@ function handleWatchlistScanData(data) { if (data.status === 'completed') { if (button) { button.disabled = false; - button.textContent = 'Scan for New Releases'; + _wlSetChipLabel(button, 'Scan for New Releases'); button.classList.remove('btn-processing'); } @@ -3295,6 +3406,7 @@ function handleWatchlistScanData(data) { • Added to wishlist: ${addedTracks}
+ ${renderWatchlistScanTrackLedger(data.scan_track_events)} `; } @@ -3307,7 +3419,7 @@ function handleWatchlistScanData(data) { } else if (data.status === 'cancelled') { if (button) { button.disabled = false; - button.textContent = 'Scan for New Releases'; + _wlSetChipLabel(button, 'Scan for New Releases'); button.classList.remove('btn-processing'); } @@ -3341,6 +3453,7 @@ function handleWatchlistScanData(data) { • Added to wishlist: ${addedTracks} + ${renderWatchlistScanTrackLedger(data.scan_track_events)} `; } @@ -3354,7 +3467,7 @@ function handleWatchlistScanData(data) { } else if (data.status === 'error') { if (button) { button.disabled = false; - button.textContent = 'Scan for New Releases'; + _wlSetChipLabel(button, 'Scan for New Releases'); button.classList.remove('btn-processing'); } @@ -3405,7 +3518,7 @@ async function updateSimilarArtists() { const scanButton = document.getElementById('scan-watchlist-btn'); button.disabled = true; - button.textContent = 'Updating...'; + _wlSetChipLabel(button, 'Updating...'); button.classList.add('btn-processing'); if (scanButton) scanButton.disabled = true; @@ -3430,7 +3543,7 @@ async function updateSimilarArtists() { const scanButton = document.getElementById('scan-watchlist-btn'); button.disabled = false; - button.textContent = 'Update Similar Artists'; + _wlSetChipLabel(button, 'Update Similar Artists'); button.classList.remove('btn-processing'); if (scanButton) scanButton.disabled = false; @@ -3453,7 +3566,7 @@ async function pollSimilarArtistsUpdate() { if (data.status === 'completed') { if (button) { button.disabled = false; - button.textContent = 'Update Similar Artists'; + _wlSetChipLabel(button, 'Update Similar Artists'); button.classList.remove('btn-processing'); } if (scanButton) scanButton.disabled = false; @@ -3464,7 +3577,7 @@ async function pollSimilarArtistsUpdate() { } else if (data.status === 'error') { if (button) { button.disabled = false; - button.textContent = 'Update Similar Artists'; + _wlSetChipLabel(button, 'Update Similar Artists'); button.classList.remove('btn-processing'); } if (scanButton) scanButton.disabled = false; @@ -3491,7 +3604,7 @@ async function pollSimilarArtistsUpdate() { if (button) { button.disabled = false; - button.textContent = 'Update Similar Artists'; + _wlSetChipLabel(button, 'Update Similar Artists'); button.classList.remove('btn-processing'); } if (scanButton) scanButton.disabled = false; diff --git a/webui/static/downloads.js b/webui/static/downloads.js index 5d5ad688..aeca1bd1 100644 --- a/webui/static/downloads.js +++ b/webui/static/downloads.js @@ -3127,8 +3127,8 @@ function _renderCandidatesModal(data) { + placeholder="Search, or paste a Tidal / Qobuz track link..." + maxlength="300" /> ${sourceControl} +
${entries.map(entryRow).join('')}
+ `).join(''); _updateOriginDeleteButton(); } +function toggleOriginGroup(btn) { + const bodyEl = btn.parentElement.querySelector('.origin-group-body'); + const caret = btn.querySelector('.origin-group-caret'); + if (!bodyEl) return; + const open = bodyEl.style.display !== 'none'; + bodyEl.style.display = open ? 'none' : ''; + if (caret) caret.textContent = open ? '▸' : '▾'; +} + function toggleOriginEntry(id, on) { if (on) _originSelected.add(id); else _originSelected.delete(id); _updateOriginDeleteButton(); diff --git a/webui/static/search.js b/webui/static/search.js index 54110af1..721df256 100644 --- a/webui/static/search.js +++ b/webui/static/search.js @@ -750,6 +750,9 @@ function initializeSearchModeToggle() { // Enrich each track with full album object (needed for wishlist functionality) const enrichedTracks = albumData.tracks.map(track => ({ ...track, + // Carry the metadata source for source-specific import logic + // (Deezer contributors upgrade for multi-artist tags). + source: track.source || album.source || null, album: { name: albumData.name, id: albumData.id, @@ -902,7 +905,16 @@ function initializeSearchModeToggle() { const enrichedTrack = { id: track.id, name: track.name, - artists: [track.artist], // Convert string to array for modal compatibility + // Carry the metadata source so the import pipeline can run + // source-specific logic (Deezer contributors upgrade for + // multi-artist tags) on the downloaded file. + source: track.source || null, + // Prefer the real artist list (Spotify/Tidal collabs) over the + // joined "A, B" display string, so downloads get proper + // multi-artist tags instead of one combined artist. + artists: (Array.isArray(track.artists) && track.artists.length) + ? track.artists + : [track.artist], album: { name: track.album, id: null, diff --git a/webui/static/settings.js b/webui/static/settings.js index 380a0b09..533d66b8 100644 --- a/webui/static/settings.js +++ b/webui/static/settings.js @@ -45,6 +45,11 @@ function debouncedAutoSaveSettings() { // fields on load — those aren't user edits and must not trigger a full // save (which re-initializes every backend service client). if (window._suppressSettingsAutoSave) return; + // #827: the Logs tab has no savable settings — its live-viewer controls + // (source picker, filters, auto-scroll) were tripping the auto-save and + // flooding app.log with "Settings saved" lines, drowning out the logs the + // user is trying to read. Never auto-save while the Logs tab is active. + if (document.querySelector('.stg-tab.active')?.dataset.tab === 'logs') return; if (settingsAutoSaveTimer) clearTimeout(settingsAutoSaveTimer); settingsAutoSaveTimer = setTimeout(() => saveSettings(true), 2000); } @@ -1056,6 +1061,10 @@ async function loadSettingsData() { const _metaSel = (_fbSrc === 'spotify' && settings.metadata?.spotify_free === true) ? 'spotify_free' : _fbSrc; document.getElementById('metadata-fallback-source').value = _metaSel; + const _efEl = document.getElementById('metadata-spotify-free-enrichment'); + // Default ON: unset (undefined) reads as enabled, matching the worker's + // config default (metadata.spotify_free_enrichment defaults True). + if (_efEl) _efEl.checked = settings.metadata?.spotify_free_enrichment !== false; // Populate Hydrabase settings const hbConfig = settings.hydrabase || {}; @@ -1391,12 +1400,49 @@ async function loadSettingsData() { console.error('Error checking dev mode:', error); } + // Secret fields now arrive masked as REDACTED_SECRET_SENTINEL (#832 + // follow-up) — wire them so editing replaces the mask instead of typing + // on top of it, and an untouched field re-masks on blur (round-trips the + // sentinel, which the server treats as "keep existing"). + _wireRedactedSecrets(); + } catch (error) { console.error('Error loading settings:', error); showToast('Failed to load settings', 'error'); } } +// Mirrors ConfigManager.REDACTED_SENTINEL — secrets are never sent to the +// browser; configured ones come back as this placeholder (rendered as dots in +// the password inputs). +const REDACTED_SECRET_SENTINEL = '__redacted_unchanged__'; + +function _wireRedactedSecrets() { + document.querySelectorAll('input[type="password"]').forEach(el => { + if (el.dataset.redactWired === '1') return; + el.dataset.redactWired = '1'; + // Clear the mask on focus so the user types a fresh value, not on top + // of the sentinel. + el.addEventListener('focus', () => { + if (el.value === REDACTED_SECRET_SENTINEL) { + el.value = ''; + el.dataset.wasRedacted = '1'; + } + }); + // Untouched (focused but not edited, left empty) → restore the mask so + // save round-trips the sentinel and the real secret is kept. + el.addEventListener('blur', () => { + if (el.dataset.wasRedacted === '1' && el.value === '') { + el.value = REDACTED_SECRET_SENTINEL; + el.dataset.wasRedacted = ''; + } + }); + // Real typing means a genuine change/clear — drop the redacted mark so + // blur won't re-mask (an emptied field then saves as a real clear). + el.addEventListener('input', () => { el.dataset.wasRedacted = ''; }); + }); +} + async function changeLogLevel() { const selector = document.getElementById('log-level-select'); const level = selector.value; @@ -2881,7 +2927,10 @@ async function saveSettings(quiet = false) { // 'Spotify Free' is stored as the spotify source + a flag, so all // downstream 'spotify' routing is unchanged. fallback_source: metadataSource === 'spotify_free' ? 'spotify' : metadataSource, - spotify_free: metadataSource === 'spotify_free' + spotify_free: metadataSource === 'spotify_free', + // Independent opt-in: run the enrichment worker on Spotify Free even + // when an official account is connected (spares the official quota). + spotify_free_enrichment: document.getElementById('metadata-spotify-free-enrichment')?.checked || false }, hydrabase: { url: document.getElementById('hydrabase-url').value, diff --git a/webui/static/stats-automations.js b/webui/static/stats-automations.js index 01aab02a..68e1adea 100644 --- a/webui/static/stats-automations.js +++ b/webui/static/stats-automations.js @@ -1678,6 +1678,12 @@ async function retryFailedMirroredDiscovery(urlHash) { state.phase = 'discovering'; state.status = 'discovering'; state.discovery_progress = 0; + // #815: stamp a baseline so the completion toast can report how many + // of these retried tracks were newly found (not just overall matched). + state._retryDiscovery = { + matchesBefore: state.spotify_matches || 0, + retryCount: data.retry_count, + }; } // Update modal buttons to show discovering state diff --git a/webui/static/style.css b/webui/static/style.css index f6a2452e..bdf6f023 100644 --- a/webui/static/style.css +++ b/webui/static/style.css @@ -2673,7 +2673,10 @@ body.helper-mode-active #dashboard-activity-feed:hover { .helper-first-launch-tip { position: fixed; bottom: 34px; - right: 84px; + /* Clear the ? float button (right:24 + 48 wide → left edge ~72px from the + right) AND its 8px pulse ring, with margin, so the tip never covers the + button (#817). Was 84px — only ~4px off the pulse ring, so they touched. */ + right: 96px; padding: 8px 16px; background: rgba(16, 16, 16, 0.95); border: 1px solid rgba(var(--accent-rgb), 0.3); @@ -19888,6 +19891,651 @@ body.helper-mode-active #dashboard-activity-feed:hover { margin-bottom: 10px; } +/* #831: expandable per-run track ledger under the scan summary */ +.watchlist-scan-tracks-toggle { + margin-top: 10px; + padding: 5px 14px; + background: rgba(255, 255, 255, 0.06); + border: 1px solid rgba(255, 255, 255, 0.12); + border-radius: 14px; + color: #ccc; + font-size: 12px; + cursor: pointer; + transition: background 0.15s ease; +} + +.watchlist-scan-tracks-toggle:hover { + background: rgba(255, 255, 255, 0.12); + color: #fff; +} + +.watchlist-scan-tracks { + margin-top: 10px; + max-height: 280px; + overflow-y: auto; + text-align: left; +} + +.watchlist-scan-tracks-section { + font-size: 11px; + font-weight: 600; + text-transform: uppercase; + letter-spacing: 0.4px; + color: #888; + margin: 10px 0 4px; +} + +.watchlist-scan-tracks .watchlist-live-addition-item { + position: relative; +} + +.watchlist-scan-track-badge { + margin-left: auto; + flex-shrink: 0; + padding: 2px 8px; + border-radius: 10px; + font-size: 10px; + font-weight: 600; + text-transform: uppercase; + letter-spacing: 0.3px; +} + +.watchlist-scan-track-badge.added { + background: rgba(80, 200, 120, 0.15); + color: #6fd99a; +} + +.watchlist-scan-track-badge.skipped { + background: rgba(255, 255, 255, 0.08); + color: #999; +} + +/* ── #831 round 2: full-width live scan deck ───────────────────────────── */ + +.wl-scan-deck { + width: 100%; + margin-top: 15px; + padding: 18px 20px 16px; + background: linear-gradient(180deg, rgba(var(--accent-rgb), 0.07), rgba(255, 255, 255, 0.02) 55%); + border: 1px solid rgba(var(--accent-rgb), 0.22); + border-radius: 14px; + backdrop-filter: blur(8px); + text-align: left; +} + +.wl-scan-deck-head { + display: flex; + align-items: center; + gap: 10px; +} + +.wl-scan-live-dot { + width: 9px; + height: 9px; + border-radius: 50%; + background: rgb(var(--accent-rgb)); + box-shadow: 0 0 0 0 rgba(var(--accent-rgb), 0.55); + animation: wl-live-pulse 1.6s ease-out infinite; +} + +@keyframes wl-live-pulse { + 0% { box-shadow: 0 0 0 0 rgba(var(--accent-rgb), 0.55); } + 70% { box-shadow: 0 0 0 9px rgba(var(--accent-rgb), 0); } + 100% { box-shadow: 0 0 0 0 rgba(var(--accent-rgb), 0); } +} + +.wl-scan-live-label { + font-size: 12px; + font-weight: 700; + text-transform: uppercase; + letter-spacing: 1.2px; + color: rgb(var(--accent-light-rgb)); +} + +.wl-scan-progress-text { + font-size: 12px; + color: rgba(255, 255, 255, 0.55); +} + +.wl-scan-counters { + margin-left: auto; + display: flex; + gap: 8px; +} + +.wl-scan-counter { + display: flex; + align-items: baseline; + gap: 5px; + padding: 4px 12px; + border-radius: 999px; + background: rgba(255, 255, 255, 0.05); + border: 1px solid rgba(255, 255, 255, 0.1); +} + +.wl-scan-counter.added { + background: rgba(80, 200, 120, 0.1); + border-color: rgba(80, 200, 120, 0.25); +} + +.wl-scan-counter-num { + font-size: 16px; + font-weight: 700; + color: #fff; + display: inline-block; +} + +.wl-scan-counter.added .wl-scan-counter-num { + color: #6fd99a; +} + +.wl-scan-counter-num.pop { + animation: wl-counter-pop 0.35s ease; +} + +@keyframes wl-counter-pop { + 0% { transform: scale(1); } + 40% { transform: scale(1.35); } + 100% { transform: scale(1); } +} + +.wl-scan-counter-label { + font-size: 10px; + text-transform: uppercase; + letter-spacing: 0.5px; + color: rgba(255, 255, 255, 0.45); +} + +.wl-scan-progress { + margin: 12px 0 14px; + height: 5px; + border-radius: 3px; + background: rgba(255, 255, 255, 0.07); + overflow: hidden; +} + +.wl-scan-progress-bar { + height: 100%; + border-radius: 3px; + background: linear-gradient(90deg, rgb(var(--accent-rgb)), rgb(var(--accent-light-rgb))); + position: relative; + transition: width 0.6s ease; +} + +.wl-scan-progress-bar::after { + content: ''; + position: absolute; + inset: 0; + background: linear-gradient(90deg, transparent, rgba(255, 255, 255, 0.35), transparent); + animation: wl-progress-shimmer 1.8s linear infinite; +} + +@keyframes wl-progress-shimmer { + 0% { transform: translateX(-100%); } + 100% { transform: translateX(100%); } +} + +.wl-scan-deck-body { + display: grid; + grid-template-columns: minmax(320px, 1.1fr) minmax(300px, 1fr); + gap: 24px; + align-items: stretch; +} + +@media (max-width: 760px) { + .wl-scan-deck-body { grid-template-columns: 1fr; } +} + +/* Hero — big square portrait (artist-page language) with the current album + stamped as an overlay badge, so the layout NEVER shifts when album art is + missing: the badge keeps its slot and shows a glyph placeholder instead. */ +.wl-scan-hero { + display: flex; + align-items: center; + gap: 20px; + min-width: 0; +} + +.wl-scan-portrait { + position: relative; + width: 148px; + height: 148px; + border-radius: 16px; + background: #181818; + box-shadow: 0 8px 28px rgba(0, 0, 0, 0.45), 0 0 0 1px rgba(var(--accent-rgb), 0.25); + flex-shrink: 0; +} + +/* glyph placeholder behind the (possibly hidden) artist image */ +.wl-scan-portrait::before { + content: '♪'; + position: absolute; + inset: 0; + display: flex; + align-items: center; + justify-content: center; + font-size: 42px; + color: rgba(255, 255, 255, 0.12); +} + +.wl-scan-portrait-img { + position: absolute; + inset: 0; + width: 100%; + height: 100%; + border-radius: 16px; + object-fit: cover; +} + +.wl-scan-album-thumb { + position: absolute; + right: -12px; + bottom: -12px; + width: 62px; + height: 62px; + border-radius: 10px; + background: #181818; + border: 3px solid #121212; + box-shadow: 0 4px 14px rgba(0, 0, 0, 0.55); + overflow: hidden; +} + +.wl-scan-album-thumb::before { + content: '♪'; + position: absolute; + inset: 0; + display: flex; + align-items: center; + justify-content: center; + font-size: 18px; + color: rgba(255, 255, 255, 0.15); +} + +.wl-scan-album-thumb img { + position: absolute; + inset: 0; + width: 100%; + height: 100%; + object-fit: cover; +} + +.wl-scan-hero-text { + min-width: 0; + display: flex; + flex-direction: column; + justify-content: center; + gap: 4px; +} + +.wl-scan-artist-name { + font-size: 24px; + font-weight: 800; + letter-spacing: -0.3px; + color: #fff; + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; +} + +.wl-scan-phase { + font-size: 12px; + font-weight: 600; + text-transform: uppercase; + letter-spacing: 0.6px; + color: rgba(var(--accent-light-rgb), 0.9); +} + +/* Fixed-height "now checking" block — placeholders keep the slot when the + scan hasn't reached an album/track yet, so nothing jumps. */ +.wl-scan-now { + margin-top: 8px; + padding: 8px 12px; + border-left: 2px solid rgba(var(--accent-rgb), 0.5); + background: rgba(255, 255, 255, 0.03); + border-radius: 0 8px 8px 0; + min-height: 44px; +} + +.wl-scan-album-name { + font-size: 14px; + font-weight: 600; + color: rgba(255, 255, 255, 0.9); + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; +} + +.wl-scan-track-name { + font-size: 12px; + color: rgba(255, 255, 255, 0.5); + margin-top: 2px; + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; +} + +/* Feed — inset panel (artist-page sidebar language) with a FIXED height so + the deck is the same size whether 0 or 10 tracks have been added. */ +.wl-scan-feed { + min-width: 0; + display: flex; + flex-direction: column; + height: 172px; + padding: 12px 14px; + background: rgba(0, 0, 0, 0.3); + border: 1px solid rgba(255, 255, 255, 0.07); + border-radius: 12px; +} + +.wl-scan-feed-label { + font-size: 11px; + font-weight: 700; + text-transform: uppercase; + letter-spacing: 0.6px; + color: #6fd99a; + margin-bottom: 8px; + flex-shrink: 0; +} + +.wl-scan-feed-list { + flex: 1; + min-height: 0; + overflow-y: auto; + display: flex; + flex-direction: column; + gap: 5px; +} + +.wl-scan-feed-list .watchlist-live-addition-empty { + flex: 1; + display: flex; + align-items: center; + justify-content: center; + color: rgba(255, 255, 255, 0.3); + font-size: 12px; +} + +.wl-scan-feed-list .watchlist-live-addition-item { + padding: 5px 8px; + border-radius: 6px; + background: rgba(255, 255, 255, 0.04); +} + +.wl-scan-feed-list .watchlist-live-addition-item img { + width: 34px; + height: 34px; + border-radius: 5px; +} + +.wl-scan-feed-list .watchlist-live-addition-item.is-new { + animation: wl-feed-slide-in 0.45s cubic-bezier(0.2, 0.9, 0.3, 1.2); +} + +@keyframes wl-feed-slide-in { + 0% { opacity: 0; transform: translateY(-10px) scale(0.96); } + 100% { opacity: 1; transform: translateY(0) scale(1); } +} + +/* ── #831 round 2: scan history modal run cards ────────────────────────── */ + +.wlh-run { + margin-bottom: 6px; +} + +.wlh-run-header { + display: flex; + align-items: center; + gap: 10px; + width: 100%; + padding: 10px 14px; + background: rgba(255, 255, 255, 0.04); + border: 1px solid rgba(255, 255, 255, 0.08); + border-radius: 10px; + cursor: pointer; + text-align: left; + transition: background 0.15s ease; +} + +.wlh-run-header:hover { + background: rgba(255, 255, 255, 0.08); +} + +.wlh-run-when { + font-size: 13px; + font-weight: 600; + color: #fff; +} + +.wlh-run-status.cancelled { + padding: 2px 8px; + border-radius: 10px; + font-size: 10px; + font-weight: 600; + text-transform: uppercase; + background: rgba(248, 113, 113, 0.12); + color: #f87171; +} + +.wlh-run-stats { + margin-left: auto; + display: flex; + gap: 14px; +} + +.wlh-run-stat { + display: flex; + align-items: baseline; + gap: 4px; + font-size: 14px; + font-weight: 700; + color: rgba(255, 255, 255, 0.85); +} + +.wlh-run-stat.added { + color: #6fd99a; +} + +.wlh-run-stat i { + font-style: normal; + font-size: 10px; + font-weight: 500; + text-transform: uppercase; + letter-spacing: 0.4px; + color: rgba(255, 255, 255, 0.4); +} + +.wlh-run-body { + padding: 8px 6px 4px 14px; +} + +.wlh-run-body .origin-modal-loading, +.wlh-empty { + padding: 16px; +} + +.wlh-run-body .watchlist-live-addition-item { + margin-bottom: 4px; +} + +/* ── #831 round 3: watchlist action chips (artist-page button language) ── */ + +.wl-chip { + --chip-rgb: 148, 163, 184; + position: relative; + display: inline-flex; + align-items: center; + justify-content: center; + gap: 8px; + padding: 8px 16px; + font-size: 12px; + font-weight: 700; + letter-spacing: 0.02em; + font-family: inherit; + color: rgba(var(--chip-rgb), 1); + background: linear-gradient(135deg, rgba(var(--chip-rgb), 0.15) 0%, rgba(var(--chip-rgb), 0.05) 100%); + border: 1px solid rgba(var(--chip-rgb), 0.3); + border-radius: 8px; + cursor: pointer; + transition: all 0.25s ease; + outline: none; + overflow: hidden; +} + +.wl-chip:hover { + background: linear-gradient(135deg, rgba(var(--chip-rgb), 0.25) 0%, rgba(var(--chip-rgb), 0.12) 100%); + border-color: rgba(var(--chip-rgb), 0.5); + transform: translateY(-1px); + box-shadow: 0 4px 16px rgba(var(--chip-rgb), 0.25); +} + +.wl-chip:active { + transform: translateY(0); + box-shadow: none; +} + +.wl-chip svg { + transition: transform 0.25s ease; + flex-shrink: 0; +} + +.wl-chip:hover svg { + transform: scale(1.12); +} + +.wl-chip--blue { --chip-rgb: 96, 165, 250; } +.wl-chip--green { --chip-rgb: 111, 217, 154; } +.wl-chip--amber { --chip-rgb: 251, 191, 36; } +.wl-chip--red { --chip-rgb: 248, 113, 113; } +.wl-chip--slate { --chip-rgb: 165, 180, 203; } + +/* Primary CTA — solid accent gradient + shimmer sweep */ +.wl-chip--cta { + color: #fff; + background: linear-gradient(135deg, rgb(var(--accent-rgb)), rgb(var(--accent-light-rgb))); + border: 1px solid rgba(var(--accent-light-rgb), 0.6); + box-shadow: 0 4px 18px rgba(var(--accent-rgb), 0.35); +} + +.wl-chip--cta:hover { + background: linear-gradient(135deg, rgb(var(--accent-rgb)), rgb(var(--accent-light-rgb))); + border-color: rgba(var(--accent-light-rgb), 0.9); + transform: translateY(-1px); + box-shadow: 0 6px 24px rgba(var(--accent-rgb), 0.5); +} + +.wl-chip-shimmer { + position: absolute; + inset: 0; + background: linear-gradient(105deg, transparent 40%, rgba(255, 255, 255, 0.25) 50%, transparent 60%); + transform: translateX(-100%); + animation: wl-chip-shimmer 3.2s ease-in-out infinite; + pointer-events: none; +} + +@keyframes wl-chip-shimmer { + 0% { transform: translateX(-100%); } + 55% { transform: translateX(100%); } + 100% { transform: translateX(100%); } +} + +/* Header meta chips */ +.wl-meta-chip { + display: inline-flex; + align-items: center; + gap: 6px; + padding: 4px 12px; + border-radius: 999px; + font-size: 12px; + font-weight: 600; + color: rgba(255, 255, 255, 0.7); + background: rgba(255, 255, 255, 0.05); + border: 1px solid rgba(255, 255, 255, 0.1); +} + +.wl-meta-chip--accent { + color: rgb(var(--accent-light-rgb)); + background: rgba(var(--accent-rgb), 0.1); + border-color: rgba(var(--accent-rgb), 0.3); +} + +/* ── #831 round 3: Global Settings modal reskin ───────────────────────── */ + +.wl-global-modal-head { + display: flex; + align-items: flex-start; + justify-content: space-between; + gap: 16px; + padding: 22px 28px 18px; + border-bottom: 1px solid rgba(255, 255, 255, 0.07); +} + +.wl-global-modal-title { + margin: 0; + font-size: 20px; + font-weight: 800; + letter-spacing: -0.2px; + color: #fff; +} + +.wl-global-modal-sub { + margin: 4px 0 0; + font-size: 13px; + color: rgba(255, 255, 255, 0.5); +} + +.wl-global-modal-close { + flex-shrink: 0; + width: 32px; + height: 32px; + display: flex; + align-items: center; + justify-content: center; + border-radius: 8px; + border: 1px solid rgba(255, 255, 255, 0.12); + background: rgba(255, 255, 255, 0.05); + color: rgba(255, 255, 255, 0.6); + font-size: 14px; + cursor: pointer; + transition: all 0.15s ease; +} + +.wl-global-modal-close:hover { + background: rgba(255, 255, 255, 0.12); + color: #fff; +} + +/* Option cards: live checked-state feedback (applies to the per-artist config + modal too — same components, same standard). */ +.config-option:has(input[type="checkbox"]:checked) { + border-color: rgba(var(--accent-rgb), 0.45); + background: rgba(var(--accent-rgb), 0.07); +} + +.config-option:has(input[type="checkbox"]:checked) .config-option-icon { + filter: none; + opacity: 1; +} + +.config-option:not(:has(input[type="checkbox"]:checked)) .config-option-icon { + filter: grayscale(0.7); + opacity: 0.55; +} + +/* The global-override master toggle gets a stronger enabled treatment */ +.global-override-toggle.enabled { + border-color: rgba(var(--accent-rgb), 0.6) !important; + background: linear-gradient(135deg, rgba(var(--accent-rgb), 0.12), rgba(var(--accent-rgb), 0.04)) !important; + box-shadow: 0 0 22px rgba(var(--accent-rgb), 0.12); +} + +.wl-global-modal .config-section-title { + font-size: 12px; + font-weight: 700; + text-transform: uppercase; + letter-spacing: 0.8px; + color: rgb(var(--accent-light-rgb)); +} + /* Watchlist Search */ .watchlist-search-input { @@ -59389,7 +60037,11 @@ body.reduce-effects #page-particles-canvas { } .adl-batch-panel.collapsed .adl-batch-active, -.adl-batch-panel.collapsed .adl-batch-history-section { +.adl-batch-panel.collapsed .adl-batch-history-section, +/* The active-batches summary line ("N batches · M downloading · …") is shown + via JS and was NOT hidden on collapse, so it overflowed the 44px rail as + clipped text (#814). Hide it with the rest of the panel body. */ +.adl-batch-panel.collapsed .adl-batch-summary { display: none; } @@ -61931,7 +62583,10 @@ body.reduce-effects .dash-card::after { } .dash-card:hover { border-color: rgba(var(--accent-rgb), 0.35); - transform: translateY(-3px); + /* No translateY lift: moving the card up on hover pulls its bottom edge off + the cursor when you hover that edge, which un-hovers → drops → re-hovers + in a rapid flicker loop (#816). The stronger shadow + glow below already + reads as "raised" without moving the hit box. */ box-shadow: 0 12px 40px rgba(0, 0, 0, 0.40), 0 4px 14px rgba(0, 0, 0, 0.22), @@ -62133,7 +62788,9 @@ body.reduce-effects .dash-card::after { .qa-tile:hover, .qa-tile:focus-visible { - transform: translateY(-2px); + /* No translateY lift — see .dash-card:hover (#816 hover-flicker). overflow + is hidden here so a pseudo-element gap-buffer can't help; the glow/shadow + below carries the hover state. */ border-color: rgba(var(--accent-rgb), 0.32); box-shadow: 0 16px 40px rgba(0, 0, 0, 0.5), @@ -62306,7 +62963,11 @@ body.reduce-effects .dash-card::after { align-items: center; justify-content: space-between; gap: clamp(4px, 0.5cqw, 8px); - opacity: 0.45; + /* The flow sits in the bottom row, directly behind the green "Open →" CTA — + at 0.45 the accent nodes/line competed with the CTA and read as clutter + (#816 "automations looks a bit strange"). Toned to a faint background + texture; it still brightens on hover. */ + opacity: 0.22; transition: opacity 0.35s ease; } @@ -66692,6 +67353,55 @@ body.em-scroll-lock { overflow: hidden; } color: #f87171 !important; } +/* #831: origins grouped by what triggered them (artist / playlist) */ +.origin-group { + margin-bottom: 6px; +} + +.origin-group-header { + display: flex; + align-items: center; + gap: 8px; + width: 100%; + padding: 8px 12px; + background: rgba(255, 255, 255, 0.04); + border: 1px solid rgba(255, 255, 255, 0.08); + border-radius: 8px; + cursor: pointer; + text-align: left; + transition: background 0.15s ease; +} + +.origin-group-header:hover { + background: rgba(255, 255, 255, 0.08); +} + +.origin-group-caret { + color: rgba(255, 255, 255, 0.45); + font-size: 11px; + width: 12px; +} + +.origin-group-name { + flex: 1; + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; + font-size: 13px; + font-weight: 600; + color: rgb(var(--accent-light-rgb)); +} + +.origin-group-count { + flex: 0 0 auto; + font-size: 11px; + color: rgba(255, 255, 255, 0.45); +} + +.origin-group-body { + padding: 4px 0 2px 6px; +} + /* ── Blocklist modal (artist/album/track bans) ── */ .blocklist-modal { position: relative; diff --git a/webui/static/sync-services.js b/webui/static/sync-services.js index 69dd0cef..f375bdd3 100644 --- a/webui/static/sync-services.js +++ b/webui/static/sync-services.js @@ -9248,6 +9248,24 @@ async function startYouTubeDiscovery(urlHash) { } } +// Discovery-complete toast. For a "Retry Failed" run (#815) it reports how many +// of the retried tracks were newly found this attempt, instead of the generic +// message — the baseline is stamped on the state by retryFailedMirroredDiscovery. +function _discoveryCompleteToast(urlHash) { + const st = youtubePlaylistStates[urlHash]; + const retry = st && st._retryDiscovery; + if (retry) { + const found = Math.max(0, (st.spotify_matches || 0) - (retry.matchesBefore || 0)); + const stillFailed = Math.max(0, (retry.retryCount || 0) - found); + delete st._retryDiscovery; + let msg = `Retry complete: ${found} of ${retry.retryCount} newly found`; + if (stillFailed > 0) msg += `, ${stillFailed} still not found`; + showToast(msg, found > 0 ? 'success' : 'info'); + return; + } + showToast('Discovery complete!', 'success'); +} + function startYouTubeDiscoveryPolling(urlHash) { // Stop any existing polling if (activeYouTubePollers[urlHash]) { @@ -9274,7 +9292,7 @@ function startYouTubeDiscoveryPolling(urlHash) { if (st) st.phase = 'discovered'; updateYouTubeCardPhase(urlHash, 'discovered'); updateYouTubeModalButtons(urlHash, 'discovered'); - showToast('Discovery complete!', 'success'); + _discoveryCompleteToast(urlHash); } }; } @@ -9322,7 +9340,7 @@ function startYouTubeDiscoveryPolling(urlHash) { updateYouTubeModalButtons(urlHash, 'discovered'); console.log('✅ Discovery complete:', urlHash); - showToast('Discovery complete!', 'success'); + _discoveryCompleteToast(urlHash); } } catch (error) { diff --git a/webui/static/watchlist-history.js b/webui/static/watchlist-history.js new file mode 100644 index 00000000..6f12bafb --- /dev/null +++ b/webui/static/watchlist-history.js @@ -0,0 +1,149 @@ +// Watchlist Scan History modal (#831 round 2). +// +// Every scan run is persisted server-side (watchlist_scan_runs) with its full +// track ledger — the tracks the run ADDED to the wishlist plus the found-but- +// skipped ones. This modal lists past runs (newest first) and expands each +// into its ledger. Note: this is what the watchlist PUT IN THE WISHLIST — the +// watchlist never downloads; downloaded tracks live in Download Origins. + +let _wlhModalEl = null; +let _wlhRuns = []; +const _wlhEventsCache = new Map(); + +function openWatchlistHistoryModal() { + if (!_wlhModalEl) { + _wlhModalEl = document.createElement('div'); + _wlhModalEl.className = 'modal-overlay origin-modal-overlay'; + _wlhModalEl.innerHTML = ` +
+
+
+

Scan History

+

Every watchlist scan and the tracks it added to your wishlist.

+
+ +
+
+
`; + _wlhModalEl.addEventListener('click', (e) => { + if (e.target === _wlhModalEl) closeWatchlistHistoryModal(); + }); + document.body.appendChild(_wlhModalEl); + } + _wlhModalEl.classList.remove('hidden'); + _wlhLoadRuns(); +} + +function closeWatchlistHistoryModal() { + if (_wlhModalEl) _wlhModalEl.classList.add('hidden'); +} + +async function _wlhLoadRuns() { + const body = document.getElementById('wlh-modal-body'); + body.innerHTML = '
Loading…
'; + try { + const resp = await fetch('/api/watchlist/scan/history?limit=50'); + const data = await resp.json(); + if (!data.success) throw new Error(data.error || 'Failed to load'); + _wlhRuns = data.runs || []; + _wlhRenderRuns(); + } catch (err) { + body.innerHTML = `
Couldn't load: ${escapeHtml(err.message)}
`; + } +} + +function _wlhRenderRuns() { + const body = document.getElementById('wlh-modal-body'); + if (!_wlhRuns.length) { + body.innerHTML = '
No scans recorded yet. Run a watchlist scan and it will appear here.
'; + return; + } + body.innerHTML = _wlhRuns.map(r => { + const when = _wlhFormatDate(r.completed_at || r.started_at); + const cancelled = r.status === 'cancelled'; + return `
+ + +
`; + }).join(''); +} + +async function toggleWatchlistHistoryRun(runId, btn) { + const runEl = btn.closest('.wlh-run'); + const bodyEl = runEl.querySelector('.wlh-run-body'); + const caret = btn.querySelector('.origin-group-caret'); + const open = bodyEl.style.display !== 'none'; + if (open) { + bodyEl.style.display = 'none'; + if (caret) caret.textContent = '▸'; + return; + } + bodyEl.style.display = ''; + if (caret) caret.textContent = '▾'; + + if (!_wlhEventsCache.has(runId)) { + bodyEl.innerHTML = '
Loading…
'; + try { + const resp = await fetch(`/api/watchlist/scan/history/${encodeURIComponent(runId)}/tracks`); + const data = await resp.json(); + _wlhEventsCache.set(runId, data.success ? (data.events || []) : []); + } catch (e) { + _wlhEventsCache.set(runId, []); + } + } + bodyEl.innerHTML = _wlhRenderEvents(_wlhEventsCache.get(runId)); +} + +function _wlhRenderEvents(events) { + if (!events.length) { + return '
No new tracks were found by this scan.
'; + } + const added = events.filter(e => e.status === 'added'); + const skipped = events.filter(e => e.status !== 'added'); + + const row = (e) => ` +
+ +
+
${escapeHtml(e.track_name || '')}
+
${escapeHtml(e.artist_name || '')}${e.album_name ? ' — ' + escapeHtml(e.album_name) : ''}
+
+ ${e.status === 'added' + ? 'added' + : 'skipped'} +
`; + + const section = (label, list) => list.length + ? `
${label} (${list.length})
${list.map(row).join('')}` + : ''; + + return section('Added to wishlist', added) + + section('Found but skipped — already queued or blocklisted', skipped); +} + +function _wlhFormatDate(ts) { + if (!ts) return 'Unknown time'; + try { + const d = new Date(ts); + if (isNaN(d.getTime())) return ts; + return d.toLocaleString(undefined, { + month: 'short', day: 'numeric', + hour: 'numeric', minute: '2-digit', + }); + } catch (e) { + return ts; + } +} + +window.openWatchlistHistoryModal = openWatchlistHistoryModal; +window.closeWatchlistHistoryModal = closeWatchlistHistoryModal; +window.toggleWatchlistHistoryRun = toggleWatchlistHistoryRun; diff --git a/webui/static/wishlist-tools.js b/webui/static/wishlist-tools.js index cbc23c52..1842e72a 100644 --- a/webui/static/wishlist-tools.js +++ b/webui/static/wishlist-tools.js @@ -953,6 +953,7 @@ async function handleAddToWishlist() { let successCount = 0; let errorCount = 0; + let skippedCount = 0; // already-in-library tracks the backend declined to add (#825) // Add each track to wishlist individually for (const track of tracks) { @@ -1026,7 +1027,10 @@ async function handleAddToWishlist() { const result = await response.json(); - if (result.success) { + if (result.success && result.skipped) { + skippedCount++; // already in library — not added (#825) + console.log(`⏭️ "${track.name}" already in library — skipped`); + } else if (result.success) { successCount++; console.log(`✅ Added "${track.name}" to wishlist`); } else { @@ -1040,12 +1044,15 @@ async function handleAddToWishlist() { } } - // Show completion message - if (successCount > 0) { - const message = errorCount > 0 - ? `Added ${successCount}/${tracks.length} tracks to wishlist (${errorCount} failed)` - : `Added ${successCount} tracks to wishlist`; - showToast(message, successCount === tracks.length ? 'success' : 'warning'); + // Show completion message (#825: report already-owned tracks honestly + // instead of counting them as "added"). + if (successCount === 0 && skippedCount > 0 && errorCount === 0) { + showToast(`All ${skippedCount} track${skippedCount !== 1 ? 's' : ''} already in your library`, 'success'); + } else if (successCount > 0) { + let message = `Added ${successCount} track${successCount !== 1 ? 's' : ''} to wishlist`; + if (skippedCount > 0) message += ` (${skippedCount} already owned)`; + if (errorCount > 0) message += ` — ${errorCount} failed`; + showToast(message, errorCount > 0 ? 'warning' : 'success'); } else { showToast('Failed to add any tracks to wishlist', 'error'); } @@ -1365,6 +1372,7 @@ async function addModalTracksToWishlist(playlistId) { try { let successCount = 0; let errorCount = 0; + let skippedCount = 0; // already-in-library tracks the backend declined to add (#825) // Add each track to wishlist individually let wingItSkipped = 0; @@ -1479,11 +1487,14 @@ async function addModalTracksToWishlist(playlistId) { } } - // Show result toast - if (successCount > 0) { - let message = errorCount > 0 - ? `Added ${successCount}/${tracks.length} tracks to wishlist (${errorCount} failed)` - : `Added ${successCount} tracks to wishlist`; + // Show result toast (#825: report already-owned skips honestly). + if (successCount === 0 && skippedCount > 0 && errorCount === 0 && wingItSkipped === 0) { + showToast(`All ${skippedCount} track${skippedCount !== 1 ? 's' : ''} already in your library`, 'success'); + await closeDownloadMissingModal(playlistId); + } else if (successCount > 0) { + let message = `Added ${successCount} track${successCount !== 1 ? 's' : ''} to wishlist`; + if (skippedCount > 0) message += ` (${skippedCount} already owned)`; + if (errorCount > 0) message += ` — ${errorCount} failed`; if (wingItSkipped > 0) message += ` (${wingItSkipped} wing-it skipped)`; showToast(message, 'success');