From f2f4f8cceea3240592066a90f956686e1562154b Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 23 Jun 2026 11:03:24 -0700 Subject: [PATCH 01/20] #910: add the per-track 'year' column the Full Refresh insert needs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Full Refresh INSERTs a per-track year (from file tags) into tracks.year, but that column was only ever in the live INSERT — never in CREATE TABLE and never in a migration. So on EVERY db (old and current — verified the shipped music_library.db lacks it too) every Full Refresh track insert hard-failed with 'table tracks has no column named year', importing 0 tracks while artists/albums succeeded. Fix (additive + nullable, nothing reads it but the writer): - add year INTEGER to the tracks CREATE TABLE (new DBs) - ALTER it onto existing tracks tables in _ensure_core_media_schema_columns (the repair backstop that already runs every init), right beside the file_size repair Tests (tests/test_tracks_year_migration.py): fresh-DB has it, nullable, idempotent, ALTERs onto an old year-less table, and a regression that the exact Full Refresh insert fails before the repair and succeeds after. --- database/music_database.py | 9 +++ tests/test_tracks_year_migration.py | 104 ++++++++++++++++++++++++++++ 2 files changed, 113 insertions(+) create mode 100644 tests/test_tracks_year_migration.py diff --git a/database/music_database.py b/database/music_database.py index 1b140874..2eb1ed80 100644 --- a/database/music_database.py +++ b/database/music_database.py @@ -301,6 +301,7 @@ class MusicDatabase: file_path TEXT, bitrate INTEGER, file_size INTEGER, -- bytes; populated by deep scan from media-server API + year INTEGER, -- per-track release year from file tags (albums.year is canonical) created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, updated_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, FOREIGN KEY (album_id) REFERENCES albums (id) ON DELETE CASCADE, @@ -1156,6 +1157,14 @@ class MusicDatabase: if track_cols and 'file_size' not in track_cols: cursor.execute("ALTER TABLE tracks ADD COLUMN file_size INTEGER") logger.info("Repaired missing file_size column on tracks table") + # #910 — Full Refresh writes a per-track `year` (from file tags), but the column + # was only ever in the live INSERT, never in CREATE TABLE or a migration. On any + # DB that predates this fix, every Full Refresh track insert hard-fails with + # "table tracks has no column named year". Additive + nullable; nothing reads it + # except the writer, so this is safe to backfill on every existing DB. + if track_cols and 'year' not in track_cols: + cursor.execute("ALTER TABLE tracks ADD COLUMN year INTEGER") + logger.info("Repaired missing year column on tracks table (#910)") cursor.execute("PRAGMA table_info(albums)") album_cols = {c[1] for c in cursor.fetchall()} diff --git a/tests/test_tracks_year_migration.py b/tests/test_tracks_year_migration.py new file mode 100644 index 00000000..8aedff45 --- /dev/null +++ b/tests/test_tracks_year_migration.py @@ -0,0 +1,104 @@ +"""Migration + regression test for the per-track ``year`` column (#910). + +Full Refresh INSERTs a per-track ``year`` (read from file tags), but the column was +only ever present in that live INSERT — never in CREATE TABLE and never in a +migration. So on every DB (old *and* current) the Full Refresh track insert +hard-failed with ``table tracks has no column named year``, importing 0 tracks +while artists/albums imported fine. + +The repair backstop (``_ensure_core_media_schema_columns``) must ALTER ``year`` +onto any tracks table that lacks it. Additive + nullable; nothing reads it except +the writer, so backfilling it on every existing DB is safe. +""" + +from __future__ import annotations + +import sqlite3 + +import pytest + +from database.music_database import MusicDatabase + + +def _track_cols(cur): + cur.execute("PRAGMA table_info(tracks)") + return {c[1] for c in cur.fetchall()} + + +# An upgraded tracks table that has every Full Refresh insert column EXCEPT year +# (mirrors the #910 reporter exactly: "64 columns, only year absent"). Text ids +# match the live schema after the id->TEXT migration. +_OLD_TRACKS = ( + "CREATE TABLE tracks (id TEXT PRIMARY KEY, album_id TEXT, artist_id TEXT, " + "title TEXT, track_number INTEGER, disc_number INTEGER, duration INTEGER, " + "file_path TEXT, bitrate INTEGER, server_source TEXT, " + "created_at TIMESTAMP, updated_at TIMESTAMP)" +) + +# The exact Full Refresh insert from web_server.py (the statement that failed). +_FULL_REFRESH_INSERT = ( + "INSERT OR IGNORE INTO tracks (id, album_id, artist_id, title, track_number, disc_number, " + "duration, file_path, bitrate, year, server_source, created_at, updated_at) " + "VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, 'soulsync', CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)" +) +_ROW = ('t1::soulsync', 'a1', 'ar1', 'Song', 1, 1, 200000, '/music/song.flac', 1000, 2009) + + +def test_fresh_db_has_year_column(tmp_path): + db = MusicDatabase(str(tmp_path / "m.db")) + cur = db._get_connection().cursor() + assert 'year' in _track_cols(cur) + + +def test_year_column_is_nullable(tmp_path): + db = MusicDatabase(str(tmp_path / "m.db")) + cur = db._get_connection().cursor() + cur.execute("PRAGMA table_info(tracks)") + info = {c[1]: c for c in cur.fetchall()} # name -> (cid, name, type, notnull, dflt, pk) + assert info['year'][3] == 0 # nullable -> safe to add to a populated table + + +def test_migration_is_idempotent(tmp_path): + db = MusicDatabase(str(tmp_path / "m.db")) + cur = db._get_connection().cursor() + before = _track_cols(cur) + # Re-running must not raise (the PRAGMA guard skips the existing column). + db._ensure_core_media_schema_columns(cur) + db._ensure_core_media_schema_columns(cur) + assert _track_cols(cur) == before + assert 'year' in _track_cols(cur) + + +def test_migration_adds_year_to_old_tracks_table(tmp_path): + conn = sqlite3.connect(str(tmp_path / "old.db")) + conn.execute(_OLD_TRACKS) + conn.commit() + cur = conn.cursor() + assert 'year' not in _track_cols(cur) + + db = MusicDatabase(str(tmp_path / "scratch.db")) + db._ensure_core_media_schema_columns(cur) + conn.commit() + + assert 'year' in _track_cols(cur) + + +def test_full_refresh_insert_fails_before_repair_and_succeeds_after(tmp_path): + """Regression for #910: the real Full Refresh track insert hard-fails on a + year-less tracks table, then succeeds once the repair has added the column.""" + conn = sqlite3.connect(str(tmp_path / "old.db")) + conn.execute(_OLD_TRACKS) + conn.commit() + cur = conn.cursor() + + # Before the fix: the live insert blows up exactly as the issue reports. + with pytest.raises(sqlite3.OperationalError, match="no column named year"): + cur.execute(_FULL_REFRESH_INSERT, _ROW) + + # Apply the repair backstop, then the same insert must succeed and persist year. + db = MusicDatabase(str(tmp_path / "scratch.db")) + db._ensure_core_media_schema_columns(cur) + cur.execute(_FULL_REFRESH_INSERT, _ROW) + conn.commit() + cur.execute("SELECT year FROM tracks WHERE id = 't1::soulsync'") + assert cur.fetchone()[0] == 2009 From 69cb51cc137b7a139ef19f8affec2376826d29b3 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 23 Jun 2026 11:12:29 -0700 Subject: [PATCH 02/20] #911: album Redownload uses the stored match id, not a fresh search MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Enhanced-view album Redownload only honoured album.spotify_album_id. For an iTunes-matched album (no spotify id) it fell through to a fresh /api/enhanced-search and grabbed the FIRST hit — which can be a different edition than the one you have (issue: matched the 66-track 'Original Soundtrack Collection', got the 19-track 'Volume 1'). Now it prefers the album row's stored source id (spotify, then iTunes — the iTunes endpoint already returns a Spotify-shaped payload) and only searches when neither exists. Also fixed the search fallback to fetch from the MATCHING source endpoint instead of always hitting Spotify (latent bug for iTunes search hits). Frontend-only orchestration fix; no JS test runner in the repo, the album endpoints are unchanged. --- webui/static/library.js | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/webui/static/library.js b/webui/static/library.js index c654d211..6be1e029 100644 --- a/webui/static/library.js +++ b/webui/static/library.js @@ -5416,20 +5416,34 @@ function _pollRedownloadProgress(taskId, overlay) { async function redownloadLibraryAlbum(album, artistName, btn) { const albumName = album.title || ''; const spotifyAlbumId = album.spotify_album_id || ''; + const itunesAlbumId = album.itunes_album_id || ''; - if (!spotifyAlbumId && !albumName) { + if (!spotifyAlbumId && !itunesAlbumId && !albumName) { showToast('No album ID or name available for redownload', 'warning'); return; } + // Fetch a specific album edition by its source id. The iTunes endpoint returns a + // Spotify-shaped payload, so all the downstream handling is identical. + const fetchAlbumBySource = (source, id, name, artist) => { + const params = new URLSearchParams({ name: name || albumName, artist: artist || artistName || '' }); + const base = source === 'itunes' ? '/api/itunes/album/' : '/api/spotify/album/'; + return fetch(`${base}${encodeURIComponent(id)}?${params}`); + }; + const origText = btn ? btn.innerHTML : ''; try { if (btn) { btn.disabled = true; btn.textContent = 'Loading...'; } + // #911 — redownload the edition the user ACTUALLY matched, using the album row's + // stored source id. A fresh search can resolve to a different edition (e.g. a + // single-volume release instead of the matched full collection), so use the + // spotify/iTunes id when present and only fall back to a search if neither exists. let response; if (spotifyAlbumId) { - const params = new URLSearchParams({ name: albumName, artist: artistName || '' }); - response = await fetch(`/api/spotify/album/${encodeURIComponent(spotifyAlbumId)}?${params}`); + response = await fetchAlbumBySource('spotify', spotifyAlbumId); + } else if (itunesAlbumId) { + response = await fetchAlbumBySource('itunes', itunesAlbumId); } if (!response || !response.ok) { @@ -5441,13 +5455,15 @@ async function redownloadLibraryAlbum(album, artistName, btn) { }); if (!searchResp.ok) throw new Error('Album search failed'); const searchData = await searchResp.json(); - const found = searchData.spotify_albums?.[0] || searchData.itunes_albums?.[0]; + const spotHit = searchData.spotify_albums?.[0]; + const found = spotHit || searchData.itunes_albums?.[0]; if (!found || !found.id) { showToast(`Could not find "${albumName}" by ${artistName || 'unknown'}`, 'warning'); return; } - const params = new URLSearchParams({ name: found.name || albumName, artist: found.artist || artistName || '' }); - response = await fetch(`/api/spotify/album/${encodeURIComponent(found.id)}?${params}`); + // Fetch from the MATCHING source endpoint — the old fallback always hit the + // Spotify endpoint, which is wrong for an iTunes search hit. + response = await fetchAlbumBySource(spotHit ? 'spotify' : 'itunes', found.id, found.name, found.artist); } if (!response.ok) throw new Error(`Failed to load album: ${response.status}`); From 65f73fae9258c8639c1ccf4047878679e7ab1936 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 23 Jun 2026 11:20:03 -0700 Subject: [PATCH 03/20] #911: redownload via the album's CANONICAL source (covers the 67% with multiple ids) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first pass only checked spotify then itunes. But ~67% of a real library (46k/69k albums) carry BOTH a spotify and itunes id, and the canonical priority is spotify>deezer>itunes>mb>…, so a spotify-first guess diverges from what the Enhanced view actually tags/displays the album as (e.g. a deezer-canonical album with an itunes id too). Now redownload reuses _getEnhancedAlbumCanonicalSource (the view's single source of truth) and fetches via the same /api/album//tracks?source= endpoint the view uses for its canonical tracklist — so a redownload is always the exact edition on screen, across every source. The stored spotify/iTunes id + a last-resort search remain as fallbacks. Frontend-only; the album endpoints and canonical resolver are unchanged. --- webui/static/library.js | 85 +++++++++++++++++++++++++---------------- 1 file changed, 53 insertions(+), 32 deletions(-) diff --git a/webui/static/library.js b/webui/static/library.js index 6be1e029..080b8fc4 100644 --- a/webui/static/library.js +++ b/webui/static/library.js @@ -5417,14 +5417,20 @@ async function redownloadLibraryAlbum(album, artistName, btn) { const albumName = album.title || ''; const spotifyAlbumId = album.spotify_album_id || ''; const itunesAlbumId = album.itunes_album_id || ''; + // #911 — the album's CANONICAL source (the same one the Enhanced view tags + displays it as) + // wins. Redownload must pull THAT exact edition, not a fresh search that can resolve to a + // different one (issue: matched the 66-track 'Original Soundtrack Collection', a search got + // the 19-track 'Volume 1'). _getEnhancedAlbumCanonicalSource is the single source of truth + // for which source identifies this album, across spotify/deezer/itunes/musicbrainz/… + const canonical = _getEnhancedAlbumCanonicalSource(album); - if (!spotifyAlbumId && !itunesAlbumId && !albumName) { + if (!canonical && !spotifyAlbumId && !itunesAlbumId && !albumName) { showToast('No album ID or name available for redownload', 'warning'); return; } - // Fetch a specific album edition by its source id. The iTunes endpoint returns a - // Spotify-shaped payload, so all the downstream handling is identical. + // Fetch a specific album edition by its source id (the Spotify/iTunes endpoints both return + // a Spotify-shaped payload, so downstream handling is identical). const fetchAlbumBySource = (source, id, name, artist) => { const params = new URLSearchParams({ name: name || albumName, artist: artist || artistName || '' }); const base = source === 'itunes' ? '/api/itunes/album/' : '/api/spotify/album/'; @@ -5435,40 +5441,55 @@ async function redownloadLibraryAlbum(album, artistName, btn) { try { if (btn) { btn.disabled = true; btn.textContent = 'Loading...'; } - // #911 — redownload the edition the user ACTUALLY matched, using the album row's - // stored source id. A fresh search can resolve to a different edition (e.g. a - // single-volume release instead of the matched full collection), so use the - // spotify/iTunes id when present and only fall back to a search if neither exists. - let response; - if (spotifyAlbumId) { - response = await fetchAlbumBySource('spotify', spotifyAlbumId); - } else if (itunesAlbumId) { - response = await fetchAlbumBySource('itunes', itunesAlbumId); - } + let albumData = null; - if (!response || !response.ok) { - const query = `${artistName || ''} ${albumName}`.trim(); - const searchResp = await fetch('/api/enhanced-search', { - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ query }) - }); - if (!searchResp.ok) throw new Error('Album search failed'); - const searchData = await searchResp.json(); - const spotHit = searchData.spotify_albums?.[0]; - const found = spotHit || searchData.itunes_albums?.[0]; - if (!found || !found.id) { - showToast(`Could not find "${albumName}" by ${artistName || 'unknown'}`, 'warning'); - return; + // 1) Primary: the canonical tagged source (any source), via the SAME + // /api/album//tracks endpoint the Enhanced view uses for its canonical tracklist — + // so a redownload is always the album the user is actually looking at. + if (canonical) { + const params = new URLSearchParams({ name: albumName, artist: artistName || '', source: canonical.source }); + const r = await fetch(`/api/album/${encodeURIComponent(canonical.id)}/tracks?${params}`); + if (r.ok) { + const data = await r.json(); + if (data && data.success && Array.isArray(data.tracks) && data.tracks.length) { + albumData = { ...data.album, tracks: data.tracks }; // normalize to {…, tracks:[]} + } } - // Fetch from the MATCHING source endpoint — the old fallback always hit the - // Spotify endpoint, which is wrong for an iTunes search hit. - response = await fetchAlbumBySource(spotHit ? 'spotify' : 'itunes', found.id, found.name, found.artist); } - if (!response.ok) throw new Error(`Failed to load album: ${response.status}`); + // 2) Fallback: the stored spotify/iTunes id, then a last-resort search. + if (!albumData) { + let response; + if (spotifyAlbumId) { + response = await fetchAlbumBySource('spotify', spotifyAlbumId); + } else if (itunesAlbumId) { + response = await fetchAlbumBySource('itunes', itunesAlbumId); + } + + if (!response || !response.ok) { + const query = `${artistName || ''} ${albumName}`.trim(); + const searchResp = await fetch('/api/enhanced-search', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ query }) + }); + if (!searchResp.ok) throw new Error('Album search failed'); + const searchData = await searchResp.json(); + const spotHit = searchData.spotify_albums?.[0]; + const found = spotHit || searchData.itunes_albums?.[0]; + if (!found || !found.id) { + showToast(`Could not find "${albumName}" by ${artistName || 'unknown'}`, 'warning'); + return; + } + // Fetch from the MATCHING source endpoint — the old fallback always hit Spotify, + // which is wrong for an iTunes search hit. + response = await fetchAlbumBySource(spotHit ? 'spotify' : 'itunes', found.id, found.name, found.artist); + } + + if (!response.ok) throw new Error(`Failed to load album: ${response.status}`); + albumData = await response.json(); + } - const albumData = await response.json(); if (!albumData || !albumData.tracks || albumData.tracks.length === 0) { showToast(`No tracks found for "${albumName}"`, 'warning'); return; From e301877e647a1666eb08d5b1aba808642ce996d8 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 23 Jun 2026 11:26:00 -0700 Subject: [PATCH 04/20] #912: Empty Folder Cleaner reads its opt-in from the right config key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The #891 'also remove image/sidecar-only folders' toggle never worked. Job settings are persisted as a nested dict under repair.jobs..settings (RepairWorker.set_job_settings), but the scan read flat keys — repair.jobs.empty_folder_cleaner.remove_residual_files — which never matched, so it always fell back to the False default and skipped every image/.lrc-only folder. (remove_junk_files had the same mismatch but its default is True, which is why only the truly-empty 'deleted' folder kept showing up.) Now reads from .settings like get_job_config / lossy_converter do. The pure dir_is_removable logic was already correct + tested; the bug was purely the config read in scan(), which had no test. Added two scan-level regression tests driving the real JobContext + a config that stores the toggle the way the UI does. --- core/repair_jobs/empty_folder_cleaner.py | 20 +++++---- tests/test_empty_folder_cleaner.py | 53 +++++++++++++++++++++++- 2 files changed, 65 insertions(+), 8 deletions(-) diff --git a/core/repair_jobs/empty_folder_cleaner.py b/core/repair_jobs/empty_folder_cleaner.py index fe1f5d02..e660a52c 100644 --- a/core/repair_jobs/empty_folder_cleaner.py +++ b/core/repair_jobs/empty_folder_cleaner.py @@ -91,13 +91,19 @@ class EmptyFolderCleanerJob(RepairJob): ignore_disposable = False try: if context.config_manager: - ignore_junk = bool(context.config_manager.get( - 'repair.jobs.empty_folder_cleaner.remove_junk_files', True)) - # #891: also clear folders left holding only images / .lrc / sidecars - # (what a reorganize leaves behind). Opt-in — default off. - ignore_disposable = bool(context.config_manager.get( - 'repair.jobs.empty_folder_cleaner.remove_residual_files', False)) - except Exception: # noqa: S110 — setting read is best-effort; defaults to True + # #912: job settings are persisted as a nested dict under + # `repair.jobs..settings` (see RepairWorker.set_job_settings / get_job_config). + # The old flat-key reads ('repair.jobs.empty_folder_cleaner.remove_residual_files') + # never matched what the UI saves, so the #891 opt-in toggle silently did nothing — + # the scan always fell back to the False default and skipped every image/.lrc folder. + job_settings = context.config_manager.get( + 'repair.jobs.empty_folder_cleaner.settings', {}) or {} + if isinstance(job_settings, dict): + ignore_junk = bool(job_settings.get('remove_junk_files', True)) + # #891: also clear folders left holding only images / .lrc / sidecars + # (what a reorganize leaves behind). Opt-in — default off. + ignore_disposable = bool(job_settings.get('remove_residual_files', False)) + except Exception: # noqa: S110 — setting read is best-effort; defaults to junk-only pass flagged = set() # dir paths we'd remove → a parent sees them as "gone" diff --git a/tests/test_empty_folder_cleaner.py b/tests/test_empty_folder_cleaner.py index 776a8e5a..3070efcf 100644 --- a/tests/test_empty_folder_cleaner.py +++ b/tests/test_empty_folder_cleaner.py @@ -6,7 +6,10 @@ from __future__ import annotations import os -from core.repair_jobs.empty_folder_cleaner import dir_is_removable, remove_empty_folder, is_junk +from core.repair_jobs.base import JobContext +from core.repair_jobs.empty_folder_cleaner import ( + EmptyFolderCleanerJob, dir_is_removable, is_junk, remove_empty_folder, +) # ── pure decision ─────────────────────────────────────────────────────────── @@ -120,3 +123,51 @@ def test_apply_residual_opt_still_refuses_real_content(tmp_path): remove_disposable=True, root=str(root), **_fx()) assert res['removed'] is False and d.exists() assert (d / 'booklet.pdf').exists() and (d / 'cover.jpg').exists() # nothing deleted + + +# ── #912: scan() must read the opt-in from where the UI SAVES it ───────────── +class _Cfg: + """Mimics ConfigManager: job settings live as a nested dict under + `repair.jobs..settings` (RepairWorker.set_job_settings writes there).""" + + def __init__(self, settings): + self._settings = settings + + def get(self, key, default=None): + if key == 'repair.jobs.empty_folder_cleaner.settings': + return self._settings + return default + + +def _run_scan(tmp_path, settings): + root = tmp_path / 'lib'; root.mkdir() + res_dir = root / 'Artist' / 'Old Album'; res_dir.mkdir(parents=True) + (res_dir / 'cover.jpg').write_text('img') # image + lyric only — the #912 case + (res_dir / 'lyrics.lrc').write_text('la') + keep = root / 'Artist2' / 'Real Album'; keep.mkdir(parents=True) + (keep / 'song.flac').write_text('audio') # has audio — must never be flagged + + flagged = [] + ctx = JobContext( + db=None, transfer_folder=str(root), config_manager=_Cfg(settings), + create_finding=lambda **kw: (flagged.append(kw.get('file_path')), True)[1], + ) + EmptyFolderCleanerJob().scan(ctx) + return str(res_dir), str(keep), set(flagged) + + +def test_scan_flags_residual_folder_when_opt_in_saved_under_settings(tmp_path): + # The toggle is stored at repair.jobs..settings.remove_residual_files. The scan must + # read it from THERE — the old flat-key read missed it, so the option did nothing (#912). + res_dir, keep, flagged = _run_scan( + tmp_path, {'remove_junk_files': True, 'remove_residual_files': True}) + assert res_dir in flagged # the cover.jpg + .lrc folder is now found + assert keep not in flagged # the audio folder is never touched + + +def test_scan_keeps_residual_folder_when_opt_off(tmp_path): + # Opt-off preserves the conservative default: an image/.lrc folder is left alone. + res_dir, keep, flagged = _run_scan( + tmp_path, {'remove_junk_files': True, 'remove_residual_files': False}) + assert res_dir not in flagged + assert keep not in flagged From 9f5bc0de89972a484343cfe104de2d8cfc61bd7c Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 23 Jun 2026 11:54:55 -0700 Subject: [PATCH 05/20] #909: backfill the YT-artist column from a confident match instead of 'Unknown Artist' MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit YouTube's flat playlist extraction returns ONLY the title (verified: no artist/channel/uploader field at all), so a track starts as 'Unknown Artist' and only gains a name if per-video recovery succeeds. When recovery comes up empty (no cookies / age-gated / bot-checked) but the track still matched confidently, the worker threw the match's artist away and left the column 'Unknown Artist' — the #909 symptom. Now the displayed yt_artist falls back to the matched artist when it's still Unknown. Display-only: the match itself, track['artists'], cache, and download flow are untouched, so a real recovered name always wins and an unmatched/error row honestly stays Unknown. Extracted resolve_display_artist as a pure, tested seam; applied in the cache-hit and fresh-match result paths (the error path has no match to draw from). --- core/discovery/youtube.py | 33 +++++++++++++++++++---- tests/discovery/test_discovery_youtube.py | 28 +++++++++++++++++++ 2 files changed, 56 insertions(+), 5 deletions(-) diff --git a/core/discovery/youtube.py b/core/discovery/youtube.py index 97cf7fd4..7c4d05fe 100644 --- a/core/discovery/youtube.py +++ b/core/discovery/youtube.py @@ -32,6 +32,26 @@ from typing import Any, Callable logger = logging.getLogger(__name__) +_UNKNOWN_ARTIST = 'Unknown Artist' + + +def resolve_display_artist(yt_artist: str, matched_artist: str) -> str: + """The artist to show in the 'YT Artist' column (#909). + + YouTube's flat playlist data carries no artist, so a track starts as + "Unknown Artist" and only gains a real name if per-video recovery succeeds. + When recovery comes up empty but the track still matched confidently, show + the matched artist instead of a misleading "Unknown Artist". Returns the + original ``yt_artist`` whenever it's already a real name (recovery worked) or + when there's no matched artist to fall back to — purely a display choice, the + match itself is unaffected. + """ + current = (yt_artist or '').strip() + if current and current != _UNKNOWN_ARTIST: + return current # recovery already gave a real name — keep it + fallback = (matched_artist or '').strip() + return fallback or _UNKNOWN_ARTIST # backfill from the match, else honest Unknown + @dataclass class YoutubeDiscoveryDeps: @@ -131,14 +151,15 @@ def run_youtube_discovery_worker(url_hash, deps: YoutubeDiscoveryDeps): cached_match = cache_db.get_discovery_cache_match(cache_key[0], cache_key[1], discovery_source) if cached_match and deps.validate_discovery_cache_artist(cleaned_artist, cached_match): logger.debug(f"CACHE HIT [{i+1}/{len(tracks)}]: {cleaned_artist} - {cleaned_title}") + _match_artist = deps.extract_artist_name(cached_match.get('artists', [''])[0]) if cached_match.get('artists') else '' result = { 'index': i, 'yt_track': cleaned_title, - 'yt_artist': cleaned_artist, + 'yt_artist': resolve_display_artist(cleaned_artist, _match_artist), 'status': 'Found', 'status_class': 'found', 'spotify_track': cached_match.get('name', ''), - 'spotify_artist': deps.extract_artist_name(cached_match.get('artists', [''])[0]) if cached_match.get('artists') else '', + 'spotify_artist': _match_artist, 'spotify_album': cached_match.get('album', {}).get('name', '') if isinstance(cached_match.get('album'), dict) else cached_match.get('album', ''), 'duration': f"{int(track['duration_ms']) // 60000}:{(int(track['duration_ms']) % 60000) // 1000:02d}" if track['duration_ms'] else '0:00', 'discovery_source': discovery_source, @@ -265,15 +286,17 @@ def run_youtube_discovery_worker(url_hash, deps: YoutubeDiscoveryDeps): best_confidence = confidence logger.info(f"Strategy 4 YouTube match (extended): {match.artists[0]} - {match.name} (confidence: {confidence:.3f})") - # Create result entry + # Create result entry. yt_artist falls back to the matched artist when + # YouTube/recovery left it "Unknown Artist" but we matched confidently (#909). + _match_artist = deps.extract_artist_name(matched_track.artists[0]) if matched_track else '' result = { 'index': i, 'yt_track': cleaned_title, - 'yt_artist': cleaned_artist, + 'yt_artist': resolve_display_artist(cleaned_artist, _match_artist), 'status': 'Found' if matched_track else 'Not Found', 'status_class': 'found' if matched_track else 'not-found', 'spotify_track': matched_track.name if matched_track else '', - 'spotify_artist': deps.extract_artist_name(matched_track.artists[0]) if matched_track else '', + 'spotify_artist': _match_artist, 'spotify_album': matched_track.album if matched_track else '', 'duration': f"{int(track['duration_ms']) // 60000}:{(int(track['duration_ms']) % 60000) // 1000:02d}" if track['duration_ms'] else '0:00', 'discovery_source': discovery_source, diff --git a/tests/discovery/test_discovery_youtube.py b/tests/discovery/test_discovery_youtube.py index 5114f159..e7f816a5 100644 --- a/tests/discovery/test_discovery_youtube.py +++ b/tests/discovery/test_discovery_youtube.py @@ -406,3 +406,31 @@ def test_results_sorted_by_index(): indices = [r['index'] for r in states['h11']['discovery_results']] assert indices == sorted(indices) + + +# --------------------------------------------------------------------------- +# resolve_display_artist (#909) — YT-artist column backfill +# --------------------------------------------------------------------------- + +def test_display_artist_keeps_recovered_name(): + # Recovery already produced a real artist → never overwritten by the match. + assert dy.resolve_display_artist('Goo Goo Dolls', 'Spotify Goo') == 'Goo Goo Dolls' + + +def test_display_artist_backfills_from_match_when_unknown(): + # YouTube/recovery gave nothing, but we matched → show the matched artist. + assert dy.resolve_display_artist('Unknown Artist', 'Don McLean') == 'Don McLean' + assert dy.resolve_display_artist('', 'Don McLean') == 'Don McLean' + assert dy.resolve_display_artist(None, 'Don McLean') == 'Don McLean' + + +def test_display_artist_stays_unknown_with_no_match(): + # No recovery AND no match → honest "Unknown Artist" (an unmatched/error row). + assert dy.resolve_display_artist('Unknown Artist', '') == 'Unknown Artist' + assert dy.resolve_display_artist('Unknown Artist', None) == 'Unknown Artist' + assert dy.resolve_display_artist('', '') == 'Unknown Artist' + + +def test_display_artist_trims_whitespace(): + assert dy.resolve_display_artist(' ', 'Matched') == 'Matched' + assert dy.resolve_display_artist('Real Artist ', '') == 'Real Artist' From fc2c38ad979e78ef8a0174646f310db00e7c5882 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 23 Jun 2026 13:35:04 -0700 Subject: [PATCH 06/20] #908: get YouTube playlists past the ~100-track cap (yt-dlp #16943 workaround) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit YouTube Music 'Liked Music' (and any large playlist) only returned ~104 tracks. Diagnosed it to a YouTube/yt-dlp regression (upstream #16943): the webpage-based playlist path stops at the first ~100-item continuation page. Not a SoulSync cap (no limit in the parse path) and not the user's cookies/IP — reproduced on a fully-served public playlist. Fix: pass extractor_args youtubetab:skip=webpage so yt-dlp pages via the InnerTube API instead. Verified live on the reporter's setup: 100 -> 200 entries on a large playlist (the workaround is itself partial upstream, but a major improvement until yt-dlp PR #16948 lands). Single touch point — parse_youtube_playlist is the only place that lists a YouTube playlist. --- web_server.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/web_server.py b/web_server.py index eaa6bb47..51bfc4c5 100644 --- a/web_server.py +++ b/web_server.py @@ -14993,7 +14993,12 @@ def parse_youtube_playlist(url): 'no_warnings': True, 'extract_flat': 'in_playlist', # Only extract basic info, no individual video metadata 'skip_download': True, # Don't download, just extract IDs and basic info - 'lazy_playlist': False, # Force full playlist resolution (prevents ~100 entry cap) + 'lazy_playlist': False, # Force full playlist resolution + # #908 / yt-dlp #16943: a YouTube-side regression caps the webpage-based playlist + # path at the first ~100-item page (Liked Music came back as only 104). Skipping the + # webpage and paging via the InnerTube API directly gets far more — verified live + # 100 -> 200+ on a large playlist. Remove once the upstream fix (PR #16948) ships. + 'extractor_args': {'youtubetab': {'skip': ['webpage']}}, } ydl_opts.update(_youtube_cookie_opts()) From 9d16abf9521a04028a8f02ccbcf7bf59ccfddc87 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 23 Jun 2026 14:27:31 -0700 Subject: [PATCH 07/20] #914: Reorganize matches bare local titles to iTunes '(feat. X)' tracks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit iTunes appends featured-artist credits to track titles ('The Chase (feat. Y)') while the user's file is often just 'The Chase'. _normalize_title only stripped the parens, keeping 'feat y' as words, so the title-match ratio fell below the 0.6 substring floor — and with no track-number rescue the track was reported 'no matching track in the iTunes tracklist' even though it was the right song. Strip feat/ft/featuring credits (parenthesised anywhere, or a bare trailing 'feat. X') before normalizing, so both sides reduce to the same title and match exactly. Guarded so 'The Feat', 'Defeat', 'Lift' aren't touched, and version differentiators (Remix) still hard-reject. Tests: 8 new (strip variants + the exact no-tn failure + cross-match/remix regressions); 63 existing reorganize tests still green. --- core/library_reorganize.py | 22 ++++++++- tests/test_reorganize_feat_matching.py | 66 ++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 1 deletion(-) create mode 100644 tests/test_reorganize_feat_matching.py diff --git a/core/library_reorganize.py b/core/library_reorganize.py index a5c9e4cd..574d5ee7 100644 --- a/core/library_reorganize.py +++ b/core/library_reorganize.py @@ -27,6 +27,7 @@ entirely. """ import os +import re import shutil import threading import time @@ -407,6 +408,18 @@ def _differentiators_in(norm_title: str) -> frozenset: return frozenset(t for t in norm_title.split() if t in _VERSION_DIFFERENTIATORS) +# Featured-artist credit: "(feat. X)" / "[ft X]" / a trailing "feat. X". The +# parenthesised form is stripped wherever it appears; the bare form only when +# something follows it (so a song literally named "The Feat" is left alone, and +# "Defeat"/"Lift" never trip the word-boundary). Case-insensitive. +_FEAT_RE = re.compile( + r"""\s*[\(\[]\s*(?:feat|ft|featuring)\b\.?[^)\]]*[\)\]] # (feat. X) / [ft. X] + | \s+(?:feat|ft|featuring)\b\.?\s+\S.*$ # trailing feat. X ... + """, + re.IGNORECASE | re.VERBOSE, +) + + def _normalize_title(value) -> str: """Lowercase + strip cosmetic punctuation and treat brackets / dashes / slashes as word separators so the same track named slightly @@ -418,10 +431,17 @@ def _normalize_title(value) -> str: - ``Don't Stop Believin'`` ↔ ``Don’t Stop Believin’`` - ``Swimming Pools (Drank) - Extended Version`` ↔ ``Swimming Pools (Drank) (Extended Version)`` + - ``The Chase (feat. Big Artist)`` ↔ ``The Chase`` (#914) """ if value is None: return '' - out = str(value).strip().lower() + out = str(value).strip() + # #914: drop featured-artist credits FIRST (while the parens are still here to + # bound the group). iTunes appends "(feat. X)" to track titles while a user's + # file is often just "The Chase" — the credit is metadata, not the song's + # identity, and leaving it in dropped the match ratio below the threshold so + # correctly-identified tracks reported as "not in the tracklist". + out = _FEAT_RE.sub('', out).lower() # Strip characters that don't carry meaning across providers. for ch in ('"', "'", '‘', '’', '“', '”', '.', ',', '!', '?', '(', ')', '[', ']', '{', '}'): diff --git a/tests/test_reorganize_feat_matching.py b/tests/test_reorganize_feat_matching.py new file mode 100644 index 00000000..8d784bbe --- /dev/null +++ b/tests/test_reorganize_feat_matching.py @@ -0,0 +1,66 @@ +"""Reorganize title matcher: featured-artist credits must not block a match (#914). + +iTunes appends "(feat. X)" to track titles while a user's file is often just the +bare title. Before the fix that extra credit dropped the substring ratio below the +match threshold, so a correctly-identified track was reported as "no matching track +in the iTunes tracklist". The credit is metadata, so it's stripped before scoring. +""" + +from __future__ import annotations + +from core.library_reorganize import _find_api_track, _normalize_title + + +# ── normalization ──────────────────────────────────────────────────────────── +def test_feat_paren_stripped_equals_bare(): + assert _normalize_title('The Chase (feat. Big Artist)') == _normalize_title('The Chase') + assert _normalize_title('The Chase (feat. Big Artist)') == 'the chase' + + +def test_feat_variants_all_stripped(): + for v in ('Song (feat. A)', 'Song (ft. A)', 'Song [ft A]', + 'Song (featuring A & B)', 'Song feat. A', 'Song ft. A & B'): + assert _normalize_title(v) == 'song', v + + +def test_feat_strip_preserves_version_differentiator(): + # The remix tag must survive so the hard-reject still distinguishes recordings. + assert _normalize_title('Song (feat. A) - Remix') == 'song remix' + + +def test_bare_feat_word_not_overstripped(): + # "The Feat" (nothing after) and words containing the letters are left alone. + assert _normalize_title('The Feat') == 'the feat' + assert _normalize_title('Defeat') == 'defeat' + assert _normalize_title('Lift Off') == 'lift off' + + +# ── matcher (the #914 failure) ─────────────────────────────────────────────── +def _api(name, tn): + return {'name': name, 'track_number': tn} + + +def test_bare_local_matches_feat_titled_api_track_without_tn(): + # The exact bug: long featured-artist name pushed the ratio below threshold and + # there was no track-number rescue. After stripping feat it's an EXACT match. + api = [_api('The Chase (feat. Somebody Very Famous)', 9)] + assert _find_api_track(api, 'The Chase', None) is api[0] + + +def test_bare_local_matches_feat_titled_api_track_with_tn(): + api = [_api('Money Trees (feat. Jay Rock)', 6), _api('Poetic Justice (feat. Drake)', 7)] + assert _find_api_track(api, 'Money Trees', 6) is api[0] + assert _find_api_track(api, 'Poetic Justice', 7) is api[1] + + +def test_feat_strip_does_not_cross_match_different_songs(): + # Stripping feat must not collapse two genuinely different titles together. + api = [_api('The Chase (feat. X)', 1), _api('The Race (feat. Y)', 2)] + assert _find_api_track(api, 'The Race', None) is api[1] + assert _find_api_track(api, 'Nonexistent Song', None) is None + + +def test_remix_still_hard_rejected_even_with_feat(): + # A bare "Song" must NOT match an API "Song (feat. X) [Remix]" — different recording. + api = [_api('Song (feat. X) - Remix', 1)] + assert _find_api_track(api, 'Song', 1) is None From 10a1e1337fbb0113533b08ac0f31ed75ca74e6e2 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 23 Jun 2026 14:39:29 -0700 Subject: [PATCH 08/20] #905 (part 1): push the deduped track list to the media server, not the raw matched list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit sync_playlist computed deduped_tracks but the dispatch (append/reconcile/replace) sent the raw valid_tracks — so a library track matched by more than one source entry was pushed multiple times each sync. Extracted _dedupe_by_rating_key (tested) and routed all three modes through it. This fixes the WITHIN-sync duplication. The cross-sync growth reporters describe (Navidrome playlist doubling every resync) is a separate server-push issue still under diagnosis. --- services/sync_service.py | 42 +++++++++++++++++++++++++-------------- tests/test_sync_dedupe.py | 40 +++++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 15 deletions(-) create mode 100644 tests/test_sync_dedupe.py diff --git a/services/sync_service.py b/services/sync_service.py index c1213f1a..077af59a 100644 --- a/services/sync_service.py +++ b/services/sync_service.py @@ -37,6 +37,23 @@ def _plex_track_file(plex_track) -> str: return '' +def _dedupe_by_rating_key(tracks: list) -> list: + """Drop repeated media-server tracks (same ratingKey), preserving first-seen + order. The same library track can match more than one source entry (or appear + twice in the source), and pushing the dupes made reconcile/replace re-add the + track every sync — part of the #905 doubling. The sync dispatch MUST send this + deduped list, not the raw matched list.""" + seen = set() + out = [] + for t in tracks: + key = getattr(t, 'ratingKey', None) + if key is None or key in seen: + continue + seen.add(key) + out.append(t) + return out + + def reresolve_manual_match_live_plex(cache_db, media_client, m, *, profile_id, source_track_id, server_source): """Re-resolve a manual match whose stored Plex ratingKey went stale. @@ -438,21 +455,16 @@ class PlaylistSyncService: f"{server_type.title()} objects with ratingKeys" ) - # Deduplicate by ratingKey — media servers silently drop duplicates - seen_keys = set() - deduped_tracks = [] - for t in valid_tracks: - if t.ratingKey not in seen_keys: - seen_keys.add(t.ratingKey) - deduped_tracks.append(t) - if len(deduped_tracks) < len(valid_tracks): + # Deduplicate by ratingKey — media servers silently drop duplicates, + # and pushing dupes made every sync re-add the same track (#905). The + # dispatch below sends THIS deduped list, never the raw `valid_tracks`. + plex_tracks = _dedupe_by_rating_key(valid_tracks) + if len(plex_tracks) < len(valid_tracks): logger.info( - f"Deduplicated {len(valid_tracks) - len(deduped_tracks)} duplicate ratingKeys " - f"({len(valid_tracks)} → {len(deduped_tracks)} tracks)" + f"Deduplicated {len(valid_tracks) - len(plex_tracks)} duplicate ratingKeys " + f"({len(valid_tracks)} → {len(plex_tracks)} tracks)" ) - plex_tracks = deduped_tracks - if not media_client: logger.error("No active media client available for playlist sync") sync_success = False @@ -462,11 +474,11 @@ class PlaylistSyncService: f"(mode: {sync_mode})" ) if sync_mode == 'append': - sync_success = media_client.append_to_playlist(playlist.name, valid_tracks) + sync_success = media_client.append_to_playlist(playlist.name, plex_tracks) elif sync_mode == 'reconcile': - sync_success = self._reconcile_or_replace(media_client, playlist.name, valid_tracks) + sync_success = self._reconcile_or_replace(media_client, playlist.name, plex_tracks) else: - sync_success = media_client.update_playlist(playlist.name, valid_tracks) + sync_success = media_client.update_playlist(playlist.name, plex_tracks) synced_tracks = len(plex_tracks) if sync_success else 0 # Not in library (for wishlist), not "total minus playlist size". diff --git a/tests/test_sync_dedupe.py b/tests/test_sync_dedupe.py new file mode 100644 index 00000000..386322ea --- /dev/null +++ b/tests/test_sync_dedupe.py @@ -0,0 +1,40 @@ +"""Sync must push a ratingKey-deduped track list (#905). + +The dedup was computed but the dispatch sent the raw matched list, so a track +matched by more than one source entry got pushed multiple times — and on +reconcile/replace that re-added it every sync (playlists doubling). This guards +the pure dedup helper the dispatch now uses. +""" + +from __future__ import annotations + +from services.sync_service import _dedupe_by_rating_key + + +class _T: + def __init__(self, rk): + self.ratingKey = rk + + +def test_removes_duplicate_rating_keys_preserving_order(): + a, b, c = _T('1'), _T('2'), _T('3') + dup_b = _T('2') + out = _dedupe_by_rating_key([a, b, c, dup_b, a]) + assert [t.ratingKey for t in out] == ['1', '2', '3'] + assert out[1] is b # first-seen object kept, not the later duplicate + + +def test_no_duplicates_is_identity(): + items = [_T('1'), _T('2'), _T('3')] + assert _dedupe_by_rating_key(items) == items + + +def test_drops_tracks_without_rating_key(): + class _NoKey: + pass + out = _dedupe_by_rating_key([_T('1'), _NoKey(), _T('2')]) + assert [t.ratingKey for t in out] == ['1', '2'] + + +def test_empty(): + assert _dedupe_by_rating_key([]) == [] From 15fa64248c8f457dccef28f0ba7556f02d34d19c Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 23 Jun 2026 15:19:47 -0700 Subject: [PATCH 09/20] #905 (root cause): Navidrome reconcile read current tracks via missing t.id -> playlists doubled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit reconcile_playlist read the existing playlist's track ids with str(t.id), but NavidromeTrack exposes the Subsonic song id as .ratingKey and has NO .id attribute (append_to_playlist already reads ratingKey — reconcile was the straggler). So current_ids came back EMPTY every time: plan_playlist_reconcile saw an 'empty' playlist, re-added the entire matched set, and removed nothing. Result: the playlist grew by the full track count on every sync (warl0ck: 5 songs, one removed -> 9), in reconcile mode and whenever reconcile is the active mode. Fix: read current ids via ratingKey, matching append_to_playlist. Verified: tests/test_navidrome_reconcile.py drives the real reconcile_playlist with a stubbed server — reverting the one-char change flips 3 tests red (they show it re-adding all 4 tracks), the fix flips them green. Covers no-op resync, a removed track (remove, don't re-add all), and an added track (append once). --- core/navidrome_client.py | 6 ++- tests/test_navidrome_reconcile.py | 74 +++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 1 deletion(-) create mode 100644 tests/test_navidrome_reconcile.py diff --git a/core/navidrome_client.py b/core/navidrome_client.py index edfc7ffc..e042788a 100644 --- a/core/navidrome_client.py +++ b/core/navidrome_client.py @@ -1192,7 +1192,11 @@ class NavidromeClient(MediaServerClient): primary = existing_playlists[0] existing_tracks = self.get_playlist_tracks(primary.id) - current_ids = [str(t.id) for t in existing_tracks if getattr(t, 'id', None)] + # #905: NavidromeTrack exposes the Subsonic song id as `ratingKey` (NOT `.id`, + # which doesn't exist) — same as append_to_playlist reads it. Reading `t.id` here + # made current_ids ALWAYS empty, so reconcile thought the playlist was empty and + # re-added every track each sync (playlists doubling) while removing nothing. + current_ids = [str(t.ratingKey) for t in existing_tracks if getattr(t, 'ratingKey', None)] desired_ids = [] for t in tracks: tid = (str(t.ratingKey) if hasattr(t, 'ratingKey') diff --git a/tests/test_navidrome_reconcile.py b/tests/test_navidrome_reconcile.py new file mode 100644 index 00000000..f1abb842 --- /dev/null +++ b/tests/test_navidrome_reconcile.py @@ -0,0 +1,74 @@ +"""Navidrome reconcile must read the CURRENT playlist via ratingKey (#905). + +NavidromeTrack exposes the Subsonic song id as ``ratingKey`` — it has no ``.id``. +reconcile_playlist used ``t.id``, so the current-track list came back empty, the +add/remove plan saw "nothing is here", and every sync re-added the whole matched +set (playlists doubling). These tests drive the real reconcile_playlist with a +stubbed client and assert the Subsonic params reflect the true delta. +""" + +from __future__ import annotations + +from types import SimpleNamespace + +from core.navidrome_client import NavidromeClient, NavidromeTrack + + +def _track(song_id): + """A NavidromeTrack as get_playlist_tracks / the matcher produce them.""" + return NavidromeTrack({'id': song_id, 'title': f'Song {song_id}'}, client=None) + + +def test_navidrome_track_exposes_ratingkey_not_id(): + # Root cause: the attribute is ratingKey, NOT id. + t = _track('42') + assert t.ratingKey == '42' + assert not hasattr(t, 'id') + + +def _client_with_existing(existing_ids): + """A NavidromeClient whose server playlist already holds `existing_ids`, + capturing the Subsonic params reconcile sends.""" + c = NavidromeClient.__new__(NavidromeClient) + c.ensure_connection = lambda: True + c.get_playlists_by_name = lambda name: [SimpleNamespace(id='PL1')] + c.get_playlist_tracks = lambda pid: [_track(i) for i in existing_ids] + captured = {} + + def _req(method, params): + captured['method'] = method + captured['params'] = params + return {'status': 'ok'} + + c._make_request = _req + return c, captured + + +def test_no_change_resync_is_a_noop(): + # Playlist already == desired → reconcile must add/remove NOTHING. + c, captured = _client_with_existing(['1', '2', '3', '4']) + desired = [_track('1'), _track('2'), _track('3'), _track('4')] + assert c.reconcile_playlist('My Playlist', desired) is True + # plan empty → early return, updatePlaylist never called (no re-add). + assert 'params' not in captured + + +def test_removed_source_track_is_removed_not_everything_readded(): + # warl0ck's exact case: server has 1..5, source now has 1..4 (5 removed). + c, captured = _client_with_existing(['1', '2', '3', '4', '5']) + desired = [_track('1'), _track('2'), _track('3'), _track('4')] + assert c.reconcile_playlist('My Playlist', desired) is True + params = captured['params'] + assert params['playlistId'] == 'PL1' + assert 'songIdToAdd' not in params # the bug: would re-add 1..4 → doubling + assert params['songIndexToRemove'] == [4] # only song '5' (index 4) is removed + + +def test_added_source_track_is_appended_once(): + # Server has 1..3, source now has 1..4 → add only '4'. + c, captured = _client_with_existing(['1', '2', '3']) + desired = [_track('1'), _track('2'), _track('3'), _track('4')] + assert c.reconcile_playlist('My Playlist', desired) is True + params = captured['params'] + assert params['songIdToAdd'] == ['4'] + assert 'songIndexToRemove' not in params From 16cb29c9eec54b2e426061503e834032af34c2a5 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 23 Jun 2026 15:32:26 -0700 Subject: [PATCH 10/20] #916: enhanced view stops flagging multi-disc tracks as missing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Multi-disc albums store disc_number=1 for EVERY track in the library (verified across the live DB — even a 175-track OST shows disc[1..1]; the scanner doesn't split discs). The enhanced view matched owned<->canonical tracks strictly by disc:track_number slot, so every canonical disc-2+ track (slot 2:N) found no owned counterpart and was flagged missing ('62/72 · 36 missing'). _deriveEnhancedMissingTracks now matches each canonical track by slot first, then falls back to title against any UNUSED owned track (consuming each owned track once, so genuine missings and duplicate titles still count right). Display-only — no scanner/data change. Verified by simulation (62 owned all disc-1, 72 canonical across 2 discs): old logic flags 36 missing, new flags 10 (the genuinely-absent tracks). Frontend-only; repo has no JS test runner. --- webui/static/library.js | 53 ++++++++++++++++++++++++++++++++--------- 1 file changed, 42 insertions(+), 11 deletions(-) diff --git a/webui/static/library.js b/webui/static/library.js index 080b8fc4..502ea93e 100644 --- a/webui/static/library.js +++ b/webui/static/library.js @@ -3964,23 +3964,54 @@ async function ensureEnhancedAlbumCanonicalTracks(album) { } } +// Loose title key for owned<->canonical matching. Lowercase, drop bracketed +// annotations ((feat. X), [Explicit]) and punctuation so editions line up. +function _normTitleForMatch(value) { + return String(value || '') + .toLowerCase() + .replace(/\([^)]*\)|\[[^\]]*\]/g, ' ') + .replace(/[^a-z0-9]+/g, ' ') + .trim(); +} + function _deriveEnhancedMissingTracks(album, canonicalTracks) { - const occupiedSlots = new Set(); - (album.tracks || []).forEach(track => { + // #916: multi-disc albums store disc_number = 1 for EVERY track in the library + // (the scanner doesn't split discs), so a strict disc:track slot match flags every + // canonical disc-2+ track as missing. Match each canonical track to an owned track + // by slot FIRST, then fall back to title — consuming each owned track once so genuine + // missings and duplicate titles still count correctly. + const owned = (album.tracks || []).map(t => ({ + slot: _trackSlotKey(t), + title: _normTitleForMatch(t.title || t.name), + used: false, + })); + const slotIndex = new Map(); + owned.forEach((o, i) => { if (o.slot !== '1:0' && !slotIndex.has(o.slot)) slotIndex.set(o.slot, i); }); + + const missing = []; + (canonicalTracks || []).forEach(track => { const key = _trackSlotKey(track); - if (key !== '1:0') occupiedSlots.add(key); - }); - return (canonicalTracks || []) - .map(track => ({ + const normalized = _normalizeExpectedMissingTrack(track, album); + if (key === '1:0' || !normalized._hasActionableContext) return; + + // 1) exact disc:track slot + const si = slotIndex.get(key); + if (si != null && !owned[si].used) { owned[si].used = true; return; } + + // 2) fallback: title vs any UNUSED owned track (handles the disc_number=1 collapse) + const nt = _normTitleForMatch(track.name || track.title); + if (nt) { + const m = owned.find(o => !o.used && o.title === nt); + if (m) { m.used = true; return; } + } + + missing.push({ ...track, name: track.name || track.title, duration_ms: track.duration_ms || track.duration || 0, - })) - .filter(track => { - const key = _trackSlotKey(track); - const normalized = _normalizeExpectedMissingTrack(track, album); - return key !== '1:0' && normalized._hasActionableContext && !occupiedSlots.has(key); }); + }); + return missing; } function _getEnhancedAlbumTrackRows(album) { From 293490387498093b38ae5889154d82e04e718c21 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 23 Jun 2026 15:58:36 -0700 Subject: [PATCH 11/20] #916: align missing-track title match with the Reorganize matcher MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reporter's image 3 shows Reorganize maps all 62 multi-disc tracks correctly ('62 unchanged') — it matches by title, proving the titles DO align on this album. My first normalizer DELETED bracket content, so 'X - Main Theme' (file) vs 'X (Main Theme)' (canonical) would mismatch. Reorganize treats brackets as separators (keeps the words); now _normTitleForMatch does the same — drop only the (feat. Y) credit, turn every other separator into whitespace. Verified: dash<->bracket, curly<->straight apostrophe, special<->regular hyphen, and feat all normalize equal; distinct titles stay distinct. --- webui/static/library.js | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/webui/static/library.js b/webui/static/library.js index 502ea93e..ca9cfa35 100644 --- a/webui/static/library.js +++ b/webui/static/library.js @@ -3964,13 +3964,19 @@ async function ensureEnhancedAlbumCanonicalTracks(album) { } } -// Loose title key for owned<->canonical matching. Lowercase, drop bracketed -// annotations ((feat. X), [Explicit]) and punctuation so editions line up. +// Loose title key for owned<->canonical matching. Mirrors the Reorganize +// matcher (core.library_reorganize._normalize_title), which already maps these +// same multi-disc tracks correctly: drop only the featured-artist credit, then +// treat every other separator (brackets, dashes, slashes, punctuation) as +// whitespace — so "X (Main Theme)" and "X - Main Theme" collapse to the same key +// while "(feat. Y)" is removed. Keeping bracket CONTENT (not deleting it) is what +// makes editions line up. function _normTitleForMatch(value) { return String(value || '') .toLowerCase() - .replace(/\([^)]*\)|\[[^\]]*\]/g, ' ') - .replace(/[^a-z0-9]+/g, ' ') + .replace(/[([]\s*(?:feat|ft|featuring)\b[^)\]]*[)\]]/g, ' ') // (feat. Y) / [ft Y] + .replace(/\s+(?:feat|ft|featuring)\b\.?\s.*$/g, ' ') // trailing feat. Y … + .replace(/[^a-z0-9]+/g, ' ') // all other separators -> space (KEEP content) .trim(); } From 600a744f7f24c39235a9587723188310efb28ea1 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 23 Jun 2026 16:18:00 -0700 Subject: [PATCH 12/20] #917: 'I have this' reuses the album's existing folder year instead of dropping it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The import rebuilds the destination path from album metadata. When the albums row has no year, release_date is empty, the path template drops $year, and the copied file lands in a NEW yearless directory instead of the album's existing 'Album (YYYY)' folder. (The code logically forces this: the year only drops when album.year is empty.) Fix: when album.year is empty, recover it from a sibling track — its own year column, else a (YYYY)/[YYYY] in the album folder name — so the rebuilt path matches the existing directory. No-op when album.year is already set. Tests: _existing_album_year_from_sibling covers year-column, paren folder, bracket folder, no-signal, and target-slot exclusion. --- core/library/missing_track_import.py | 59 ++++++++++++++++++++++ tests/library/test_missing_track_import.py | 44 ++++++++++++++++ 2 files changed, 103 insertions(+) diff --git a/core/library/missing_track_import.py b/core/library/missing_track_import.py index 7b322be4..ad59b5ae 100644 --- a/core/library/missing_track_import.py +++ b/core/library/missing_track_import.py @@ -10,6 +10,7 @@ from __future__ import annotations from dataclasses import dataclass import logging import os +import re import shutil import uuid from typing import Any, Callable, Dict, Optional @@ -121,6 +122,20 @@ def import_existing_track_for_album_slot(album_id: str, payload: dict, deps: Mis if album_data.get("server_source") and source_track.get("server_source") and album_data["server_source"] != source_track["server_source"]: raise MissingTrackImportError("Selected track belongs to a different library source", 400) + # #917: "I have this" rebuilds the destination path from album metadata. When the album row + # has no year, the rebuilt path drops the $year and the copied file lands in a NEW, yearless + # directory instead of the album's existing folder. Recover the year from a sibling track so + # the import reuses the same directory. + if not album_data.get("year"): + recovered_year = _existing_album_year_from_sibling( + database, album_id, deps.resolve_library_file_path_fn, + int(expected.get("disc_number") or 1), int(expected.get("track_number") or 1), + ) + if recovered_year: + album_data["year"] = recovered_year + logger.info("[I Have This] recovered album year %s from existing folder for album %s", + recovered_year, album_id) + source_path = deps.resolve_library_file_path_fn(source_track.get("file_path")) if not source_path: raise MissingTrackImportError(_file_not_found_message(source_track.get("file_path")), 404) @@ -426,6 +441,50 @@ def _sync_imported_track(deps: MissingTrackImportDeps, track_id, expected_title: logger.debug("Existing-track import server sync skipped/failed: %s", sync_err) +def _existing_album_year_from_sibling( + database, + album_id: str, + resolve_library_file_path_fn: Callable[[Optional[str]], Optional[str]], + target_disc: int, + target_track: int, +) -> Optional[str]: + """Find the release year already baked into this album's on-disk folder (#917). + + Read from a sibling track — its own ``year`` column first, else a ``(YYYY)`` / + ``[YYYY]`` in the album folder name — so an "I have this" import reuses the album's + existing directory instead of rebuilding a yearless one. Returns the 4-digit year + string, or None when no signal exists. + """ + try: + with database._get_connection() as conn: + cursor = conn.cursor() + cursor.execute( + """ + SELECT file_path, year FROM tracks + WHERE album_id = ? + AND file_path IS NOT NULL AND file_path != '' + AND NOT (COALESCE(disc_number, 1) = ? AND track_number = ?) + ORDER BY COALESCE(disc_number, 1), track_number + LIMIT 12 + """, + (album_id, target_disc, target_track), + ) + rows = cursor.fetchall() + for row in rows: + year = row["year"] + if year is not None and str(year).strip()[:4].isdigit(): + return str(year).strip()[:4] + resolved = resolve_library_file_path_fn(row["file_path"]) + if resolved: + folder = os.path.basename(os.path.dirname(resolved)) + match = re.search(r"[(\[](\d{4})[)\]]", folder) # "Album (2019)" / "Album [2019]" + if match: + return match.group(1) + except Exception as exc: + logger.debug("Could not recover album year from sibling for %s: %s", album_id, exc) + return None + + def copy_album_identity_from_target_sibling( database, album_id: str, diff --git a/tests/library/test_missing_track_import.py b/tests/library/test_missing_track_import.py index 077810be..4850c381 100644 --- a/tests/library/test_missing_track_import.py +++ b/tests/library/test_missing_track_import.py @@ -258,3 +258,47 @@ def test_import_rejects_missing_expected_track_context(tmp_path): assert exc.value.status_code == 400 assert "expected_track" in str(exc.value) + + +# ── #917: recover album year from existing folder so "I have this" reuses the dir ── +def _year_db(rows): + conn = sqlite3.connect(":memory:") + conn.row_factory = sqlite3.Row + conn.execute("CREATE TABLE tracks (album_id TEXT, disc_number INTEGER, track_number INTEGER, file_path TEXT, year INTEGER)") + conn.executemany( + "INSERT INTO tracks (album_id, disc_number, track_number, file_path, year) VALUES (?,?,?,?,?)", + [(r.get("album_id", "A1"), r.get("disc_number", 1), r.get("track_number", 1), + r.get("file_path"), r.get("year")) for r in rows], + ) + conn.commit() + return _FakeDB(conn) + + +def _ident(p): + return p + + +def test_year_recovered_from_sibling_year_column(): + db = _year_db([{"track_number": 3, "file_path": "/m/Artist/Album/03.flac", "year": 2024}]) + assert mti._existing_album_year_from_sibling(db, "A1", _ident, 1, 5) == "2024" + + +def test_year_recovered_from_paren_folder_name(): + db = _year_db([{"track_number": 3, "file_path": "/music/Artist/Album (2019)/03 - Song.flac", "year": None}]) + assert mti._existing_album_year_from_sibling(db, "A1", _ident, 1, 5) == "2019" + + +def test_year_recovered_from_bracket_folder_name(): + db = _year_db([{"track_number": 3, "file_path": "/music/Artist/Album [2008]/03.flac", "year": None}]) + assert mti._existing_album_year_from_sibling(db, "A1", _ident, 1, 5) == "2008" + + +def test_year_none_when_no_signal(): + db = _year_db([{"track_number": 3, "file_path": "/music/Artist/Album/03.flac", "year": None}]) + assert mti._existing_album_year_from_sibling(db, "A1", _ident, 1, 5) is None + + +def test_year_ignores_the_target_slot_itself(): + # The only sibling row IS the slot being imported -> excluded -> no year. + db = _year_db([{"disc_number": 1, "track_number": 5, "file_path": "/m/Album (2030)/05.flac", "year": 2030}]) + assert mti._existing_album_year_from_sibling(db, "A1", _ident, 1, 5) is None From cce7df4f3d9c6ef18cb1baade9f88014d59bf67f Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 23 Jun 2026 17:58:17 -0700 Subject: [PATCH 13/20] #918: iTunes album fetch no longer truncates albums >50 tracks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit iTunes get_album_tracks called _lookup(id, entity='song') with no limit. The iTunes Lookup API returns only 50 related entities unless limit is passed (max 200), so albums over 50 tracks showed only the first 50 in the download window. Pass limit=200 on the main lookup AND the fallback- storefront request. Proven against the live iTunes API on the reporter's exact album (Frieren OST, id 1739445636, 70 tracks): no limit -> 50 songs, limit=200 -> 70 songs. Spotify already paginates; Deezer uses limit=500 — iTunes was the only truncating source. Regression test asserts limit=200 is requested. --- core/itunes_client.py | 6 ++-- tests/test_itunes_album_tracks_limit.py | 46 +++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 2 deletions(-) create mode 100644 tests/test_itunes_album_tracks_limit.py diff --git a/core/itunes_client.py b/core/itunes_client.py index 25f9c73a..0d34b6db 100644 --- a/core/itunes_client.py +++ b/core/itunes_client.py @@ -741,7 +741,9 @@ class iTunesClient: if cached and cached.get('items'): return cached - results = self._lookup(id=album_id, entity='song') + # #918: the iTunes Lookup API returns only 50 related entities unless `limit` is + # passed (max 200), so albums >50 tracks were truncated in the download window. + results = self._lookup(id=album_id, entity='song', limit=200) if not results: return None @@ -763,7 +765,7 @@ class iTunesClient: try: fb_results = self.session.get( self.LOOKUP_URL, - params={'id': album_id, 'entity': 'song', 'country': fallback}, + params={'id': album_id, 'entity': 'song', 'country': fallback, 'limit': 200}, # #918 timeout=15 ) if fb_results.status_code == 200: diff --git a/tests/test_itunes_album_tracks_limit.py b/tests/test_itunes_album_tracks_limit.py new file mode 100644 index 00000000..1fa36e40 --- /dev/null +++ b/tests/test_itunes_album_tracks_limit.py @@ -0,0 +1,46 @@ +"""iTunes album-track fetch must request the full album, not the 50-entity default (#918). + +The iTunes Lookup API returns only 50 related entities unless `limit` is passed (max 200), +so albums >50 tracks were truncated to the first 50 in the download window. get_album_tracks +must pass limit=200. +""" + +from __future__ import annotations + +from core import itunes_client as ic + + +class _Cache: + """Cache stub: always a miss; any write method is a no-op.""" + def get_entity(self, *a, **k): + return None + + def __getattr__(self, _name): + return lambda *a, **k: None + + +_RESULTS = [ + {'wrapperType': 'collection', 'collectionId': 123, 'collectionName': 'Big OST', + 'artistName': 'Composer', 'trackCount': 70, 'artworkUrl100': 'http://x/100x100bb.jpg'}, + {'wrapperType': 'track', 'kind': 'song', 'trackId': 1, 'trackName': 'Track 1', + 'trackNumber': 1, 'discNumber': 1, 'artistName': 'Composer', 'trackTimeMillis': 1000}, +] + + +def test_get_album_tracks_requests_limit_200(monkeypatch): + client = ic.iTunesClient(country='US') + captured = {} + + def fake_lookup(**params): + captured.update(params) + return _RESULTS + + monkeypatch.setattr(client, '_lookup', fake_lookup) + monkeypatch.setattr(ic, 'get_metadata_cache', lambda: _Cache()) + + result = client.get_album_tracks('123') + + assert captured.get('limit') == 200 # #918: full album, not the iTunes 50 default + assert captured.get('entity') == 'song' + assert captured.get('id') == '123' + assert result is not None From 2b17ed8451800fdcb830b647e9e10828c3ba4ba0 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 23 Jun 2026 18:46:35 -0700 Subject: [PATCH 14/20] #915: post-processing hydrates lean album context from the PRIMARY source (parity with Reorganize) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause: the only album-context backfill in the download path (hydrate_download_metadata) goes through spotify_client.get_track_details — Spotify-only. An iTunes/Deezer-primary user's download kept a lean context (no release_date), so the path dropped $year and the date defaulted to YYYY-01-01 — until they ran a Reorganize, which reads the full album from the PRIMARY source. That asymmetry IS the bug. Fix: when the context is lean and the primary source isn't Spotify, hydrate it from that source via get_album_for_source — the exact path Reorganize/Enrich use. Verified the primary source returns the real data (live iTunes get_album for the reporter's album: release_date 2024-04-17, not 2024-01-01). backfill_album_context_from_source is a pure, injected-fn seam: 6 tests (hydrate, no-op when complete / spotify-primary / sentinel-id, stays-lean on None, swallows source errors). 552 downloads tests green. --- core/downloads/candidates.py | 15 ++++ core/downloads/track_metadata_backfill.py | 51 ++++++++++++++ .../downloads/test_track_metadata_backfill.py | 68 +++++++++++++++++++ 3 files changed, 134 insertions(+) diff --git a/core/downloads/candidates.py b/core/downloads/candidates.py index d9f20777..1edca30d 100644 --- a/core/downloads/candidates.py +++ b/core/downloads/candidates.py @@ -205,6 +205,21 @@ def attempt_download_with_candidates(task_id, candidates, track, batch_id=None, 'artists': _fallback_album_artists } + # #915: parity with Reorganize / manual Enrich. If the album context is lean + # (no release_date) and the user's PRIMARY metadata source isn't Spotify, hydrate + # it from that source — the same place a reorganize reads — so the download's + # $year folder, release_date and album_type match instead of dropping the year / + # defaulting to YYYY-01-01 and forcing a manual reorganize afterwards. + try: + from core.downloads.track_metadata_backfill import backfill_album_context_from_source + from core.metadata import registry as _meta_registry + from core.metadata.album_tracks import get_album_for_source as _get_album_for_source + backfill_album_context_from_source( + spotify_album_context, _meta_registry.get_primary_source(), _get_album_for_source, + ) + except Exception as _bf_err: # noqa: BLE001 — never let backfill break a download + logger.debug("[Context] primary-source album backfill skipped: %s", _bf_err) + download_payload = candidate.__dict__ username = download_payload.get('username') diff --git a/core/downloads/track_metadata_backfill.py b/core/downloads/track_metadata_backfill.py index da7fac0e..82474d65 100644 --- a/core/downloads/track_metadata_backfill.py +++ b/core/downloads/track_metadata_backfill.py @@ -93,6 +93,56 @@ def _backfill_album_context( album_context['image_url'] = first['url'] +# Placeholder album ids used when no real source album id is known — never queryable. +_SENTINEL_ALBUM_IDS = {'explicit_album', 'from_sync_modal', ''} + + +def backfill_album_context_from_source( + album_context: Dict[str, Any], + primary_source: Optional[str], + get_album_for_source_fn: Any, +) -> bool: + """Hydrate a lean album context from the user's PRIMARY metadata source (#915). + + Post-processing's only album backfill (:func:`hydrate_download_metadata`) goes through + ``spotify_client.get_track_details`` — Spotify-only. An iTunes/Deezer-primary user's + download therefore kept a lean context (no ``release_date``), so the path dropped the + ``$year`` and the date defaulted to ``YYYY-01-01`` — until they ran a Reorganize, which + reads the full album from the PRIMARY source. This closes that gap by doing the same: + fetch the full album from the primary source and backfill, so a download's pathing/tags + match what a later reorganize would produce. + + ``get_album_for_source_fn(source, album_id)`` is injected (the real one is + ``core.metadata.album_tracks.get_album_for_source``) so this stays pure + testable. + No-op when: the context is already complete; the primary source is spotify (the existing + track-details path covers it); or no real source album id is present. Returns True when + it filled anything. Never raises — a backfill failure must not break a download. + """ + if not isinstance(album_context, dict) or not _album_is_lean(album_context): + return False + if not primary_source or primary_source == 'spotify': + return False + album_id = album_context.get('id') + if not album_id or str(album_id) in _SENTINEL_ALBUM_IDS: + return False + try: + album = get_album_for_source_fn(primary_source, str(album_id)) + except Exception as e: # noqa: BLE001 — defensive: never let backfill break a download + logger.warning("[Context] primary-source (%s) album backfill failed: %s", primary_source, e) + return False + if not isinstance(album, dict): + return False + before = album_context.get('release_date') + _backfill_album_context(album_context, {'album': album}) + if album_context.get('release_date') and album_context.get('release_date') != before: + logger.info( + "[Context] Hydrated lean album context from primary source %s " + "(release_date=%r, total_tracks=%r)", + primary_source, album_context.get('release_date'), album_context.get('total_tracks'), + ) + return True + + def hydrate_download_metadata( track: Any, track_info: Any, @@ -172,4 +222,5 @@ def hydrate_download_metadata( __all__ = [ 'ResolvedTrackMetadata', 'hydrate_download_metadata', + 'backfill_album_context_from_source', ] diff --git a/tests/downloads/test_track_metadata_backfill.py b/tests/downloads/test_track_metadata_backfill.py index 99f77351..6eb74ed5 100644 --- a/tests/downloads/test_track_metadata_backfill.py +++ b/tests/downloads/test_track_metadata_backfill.py @@ -507,5 +507,73 @@ def test_api_called_at_most_once_per_invocation(): assert client.get_track_details.call_count == 1 +# ── #915: source-aware album-context backfill (parity with Reorganize/Enrich) ── +from core.downloads.track_metadata_backfill import backfill_album_context_from_source # noqa: E402 + + +def _lean_ctx(album_id="itunes-123"): + return {"id": album_id, "name": "Big OST", "release_date": "", "total_tracks": 0, "album_type": "album"} + + +def _itunes_album(**over): + base = {"id": "itunes-123", "name": "Big OST", "release_date": "2024-04-17", + "total_tracks": 70, "album_type": "album"} + base.update(over) + return base + + +def test_backfill_hydrates_lean_context_from_primary_source(): + ctx = _lean_ctx() + calls = [] + + def get_album(source, album_id): + calls.append((source, album_id)) + return _itunes_album() + + assert backfill_album_context_from_source(ctx, "itunes", get_album) is True + assert ctx["release_date"] == "2024-04-17" # real date, not YYYY-01-01 + assert ctx["total_tracks"] == 70 + assert calls == [("itunes", "itunes-123")] # queried the PRIMARY source with the album id + + +def test_backfill_noop_when_context_already_complete(): + ctx = {"id": "itunes-123", "release_date": "2024-04-17", "total_tracks": 70, "album_type": "album"} + called = [] + backfill_album_context_from_source(ctx, "itunes", lambda *a: called.append(a)) + assert called == [] # complete -> no fetch + assert ctx["release_date"] == "2024-04-17" + + +def test_backfill_noop_for_spotify_primary(): + # Spotify is covered by hydrate_download_metadata's get_track_details path. + called = [] + assert backfill_album_context_from_source(_lean_ctx(), "spotify", lambda *a: called.append(a)) is False + assert called == [] + + +def test_backfill_noop_for_sentinel_album_id(): + called = [] + for sentinel in ("explicit_album", "from_sync_modal", ""): + backfill_album_context_from_source(_lean_ctx(sentinel), "itunes", lambda *a: called.append(a)) + assert called == [] # no real id -> never queries + + +def test_backfill_leaves_context_lean_when_source_returns_nothing(): + ctx = _lean_ctx() + backfill_album_context_from_source(ctx, "itunes", lambda *a: None) + assert ctx["release_date"] == "" # still lean, but no crash + + +def test_backfill_swallows_source_errors(): + ctx = _lean_ctx() + + def boom(*_a): + raise RuntimeError("itunes down") + + # Must not raise — a backfill failure cannot break a download. + assert backfill_album_context_from_source(ctx, "itunes", boom) is False + assert ctx["release_date"] == "" + + if __name__ == '__main__': pytest.main([__file__, '-v']) From d4e80fdaa0413ea05f30dace8cc37f6e0b1fce07 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 23 Jun 2026 19:21:50 -0700 Subject: [PATCH 15/20] #915: redownload pulls full album_data from the primary source for iTunes/Deezer too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second leak of the same class: redownload_start built full album_data (release_date/album_type/ total_tracks) only in the Spotify branch. The iTunes and Deezer branches set just track/disc number and left album_data lean ({'name': ...}), so single-track redownloads on those sources dropped the $year — same symptom as #915 in the add/download path. Fix: both branches now fetch the album via get_album_for_source (cached, source-aware) and build album_data through the shared _album_data_from_source helper, mirroring the Spotify branch. Falls back to the lean default if the fetch returns nothing (no regression). get_album is cached on both iTunes and Deezer, so no extra API cost. Tests: _album_data_from_source (full build, image-url fallback, defaults). 694 library+downloads tests green. --- core/library/redownload.py | 48 ++++++++++++++++++--- tests/library/test_redownload_album_data.py | 38 ++++++++++++++++ 2 files changed, 80 insertions(+), 6 deletions(-) create mode 100644 tests/library/test_redownload_album_data.py diff --git a/core/library/redownload.py b/core/library/redownload.py index 08d5cd70..a2bc895f 100644 --- a/core/library/redownload.py +++ b/core/library/redownload.py @@ -16,6 +16,7 @@ from core.runtime_state import ( download_tasks, tasks_lock, ) +from core.metadata.album_tracks import get_album_for_source from core.metadata.registry import ( get_deezer_client, get_itunes_client, @@ -26,6 +27,27 @@ from database.music_database import get_database logger = logging.getLogger(__name__) +def _album_data_from_source(full: dict, album_id: str, fallback_name: str) -> dict: + """Build the redownload `album_data` from a primary-source get_album result (#915). + + Mirrors the Spotify branch's album_data shape so iTunes/Deezer redownloads carry the + real release_date / album_type / total_tracks — instead of a lean {'name': ...} that + drops the $year and forces a manual reorganize afterwards.""" + images = full.get('images') or [] + image_url = full.get('image_url') or '' + if not image_url and images and isinstance(images[0], dict): + image_url = images[0].get('url', '') + return { + 'id': str(full.get('id') or album_id), + 'name': full.get('name') or fallback_name, + 'release_date': full.get('release_date', ''), + 'album_type': full.get('album_type', 'album'), + 'total_tracks': full.get('total_tracks', 0), + 'images': images, + 'image_url': image_url, + } + + def _get_itunes_client(): """Mirror of web_server._get_itunes_client — delegates to registry.""" return get_itunes_client() @@ -141,12 +163,26 @@ def redownload_start(track_id): 'images': album_images, 'image_url': album_images[0]['url'] if album_images else '', } - elif meta_source == 'itunes': - track_number = full_track_details.get('trackNumber') - disc_number = full_track_details.get('discNumber', 1) - elif meta_source == 'deezer': - track_number = full_track_details.get('track_position') - disc_number = full_track_details.get('disk_number', 1) + elif meta_source in ('itunes', 'deezer'): + # #915: parity with the Spotify branch + Reorganize — pull the full album from + # the primary source so album_data carries release_date/album_type/total_tracks + # (was lean {'name': ...}, which dropped the $year on iTunes/Deezer redownloads). + if meta_source == 'itunes': + track_number = full_track_details.get('trackNumber') + disc_number = full_track_details.get('discNumber', 1) + _alb_id = full_track_details.get('collectionId') + else: + track_number = full_track_details.get('track_position') + disc_number = full_track_details.get('disk_number', 1) + _alb_id = (full_track_details.get('album') or {}).get('id') + if _alb_id: + try: + _full_album = get_album_for_source(meta_source, str(_alb_id)) + except Exception as _alb_err: # noqa: BLE001 — never let metadata break redownload + logger.debug("[Redownload] %s album fetch failed: %s", meta_source, _alb_err) + _full_album = None + if isinstance(_full_album, dict): + album_data = _album_data_from_source(_full_album, str(_alb_id), metadata.get('album', '')) track_data = { 'id': meta_id, diff --git a/tests/library/test_redownload_album_data.py b/tests/library/test_redownload_album_data.py new file mode 100644 index 00000000..8a648951 --- /dev/null +++ b/tests/library/test_redownload_album_data.py @@ -0,0 +1,38 @@ +"""Redownload builds full album_data from the primary source (#915). + +iTunes/Deezer single-track redownloads used to carry a lean album_data ({'name': ...}), +dropping the $year folder. _album_data_from_source mirrors the Spotify branch so the real +release_date / album_type / total_tracks come through. +""" + +from __future__ import annotations + +from core.library.redownload import _album_data_from_source + + +def test_builds_full_album_data_from_source(): + full = {'id': 'it-1', 'name': 'Big OST', 'release_date': '2024-04-17', + 'album_type': 'album', 'total_tracks': 70, 'image_url': 'http://x/cover.jpg'} + out = _album_data_from_source(full, 'it-1', 'fallback') + assert out['release_date'] == '2024-04-17' # real date, not YYYY-01-01 + assert out['album_type'] == 'album' + assert out['total_tracks'] == 70 + assert out['id'] == 'it-1' + assert out['name'] == 'Big OST' + assert out['image_url'] == 'http://x/cover.jpg' + + +def test_image_url_falls_back_to_images_array(): + full = {'name': 'A', 'release_date': '2020-01-01', 'images': [{'url': 'http://x/img.jpg'}]} + out = _album_data_from_source(full, 'a1', 'fb') + assert out['image_url'] == 'http://x/img.jpg' + + +def test_defaults_when_fields_missing(): + out = _album_data_from_source({}, 'a1', 'Fallback Album') + assert out['id'] == 'a1' # falls back to the queried id + assert out['name'] == 'Fallback Album' + assert out['album_type'] == 'album' # default + assert out['total_tracks'] == 0 + assert out['release_date'] == '' + assert out['image_url'] == '' From 9ad518861094487b311167ce50c963a424f9bdbc Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 23 Jun 2026 22:47:43 -0700 Subject: [PATCH 16/20] #913: listening-driven recommendation core (pure, tested) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New, fully-additive module — the heart of the 'expand Because You Listen To into a real listening-driven block' plan. Two pure functions, no DB/network/config: - rank_recommended_artists(seeds, similars_by_seed, owned): consensus-ranked artists you'd love but don't own. Score = Σ over endorsing seeds of (play_weight × similarity) — rewards consensus, play weight and similarity strength in one sum. Excludes owned + seeds; min_seed_count is the adventurousness dial's lever; exposes seed_count + which seeds ('because you like A, B, C'). - aggregate_candidate_tracks(recs, top_tracks_by_artist, owned): per-artist-capped, deduped, rank-ordered candidate list for the generated playlist; exclude_owned toggles discovery vs replay. 11 tests (consensus vs single, play-weight, similarity, owned/seed exclusion, min_seed_count, case-insensitive dedup, per-artist cap, owned exclusion, total limit, empty-artist skip). Nothing existing touched — wiring into the watchlist scan + playlist sync comes next. --- core/discovery/listening_recommendations.py | 166 ++++++++++++++++++ .../test_listening_recommendations.py | 117 ++++++++++++ 2 files changed, 283 insertions(+) create mode 100644 core/discovery/listening_recommendations.py create mode 100644 tests/discovery/test_listening_recommendations.py diff --git a/core/discovery/listening_recommendations.py b/core/discovery/listening_recommendations.py new file mode 100644 index 00000000..e8683dae --- /dev/null +++ b/core/discovery/listening_recommendations.py @@ -0,0 +1,166 @@ +"""Listening-driven recommendation core (#913). + +PURE, side-effect-free ranking that turns "the artists you listen to most" plus +"who's similar to each" into: + + 1. a consensus-ranked list of artists you'd probably love but don't own, and + 2. an aggregated candidate-track list for a generated playlist. + +No DB / network / config here. The caller (the watchlist scanner) supplies the +seeds (top-played artists), the ``similar_artists`` rows per seed, and the +owned-artist set, then fetches top tracks for the winners. Keeping the decision +logic in one pure place makes it fully unit-testable without the live stack and +keeps the scan wiring thin — and additive, so it can't disturb existing flows. + +Scoring rationale (the "best in class" bit): a recommended artist's score is +``Σ over the seeds that recommend it of (seed_weight × similarity)``. That single +sum rewards all three signals at once — **consensus** (an artist endorsed by many +of your seeds accumulates more terms), your **play weight** (heavier seeds push +harder), and **similarity strength** — instead of a flat "appears in N lists". +``seed_count`` is exposed separately for display ("because you like A, B, C") and +as the adventurousness dial's lever (``min_seed_count``). +""" + +from __future__ import annotations + +from dataclasses import dataclass, field +from typing import Dict, List, Optional, Sequence, Set + + +def _norm(name: object) -> str: + return str(name or "").strip().lower() + + +def _positive_float(value: object, default: float = 1.0) -> float: + try: + f = float(value) # type: ignore[arg-type] + except (TypeError, ValueError): + return default + return f if f > 0 else default + + +@dataclass +class RecommendedArtist: + """One artist recommended from your listening, with the why.""" + name: str # display name (first-seen casing) + score: float # Σ seed_weight × similarity + seed_count: int # distinct seeds endorsing it (consensus) + seeds: List[str] = field(default_factory=list) # display names of those seeds + + +def rank_recommended_artists( + seeds: Sequence[dict], + similars_by_seed: Dict[str, Sequence[dict]], + owned_artist_names: Optional[Set[str]] = None, + *, + limit: int = 30, + min_seed_count: int = 1, +) -> List[RecommendedArtist]: + """Rank artists similar to your most-played by consensus + play weight + similarity. + + Args: + seeds: ``[{'name': str, 'weight': float}]`` — your top-played artists. + ``weight`` (play count or any positive number) defaults to 1.0. + similars_by_seed: ``{seed_name_lower: [{'name': str, 'score': float}]}`` — the + similar-artist rows for each seed. ``score`` is optional (defaults 1.0). + owned_artist_names: lowercased names already in the library — excluded so the + result is artists you DON'T have. The seeds themselves are always excluded. + limit: max results. + min_seed_count: drop recommendations endorsed by fewer than N seeds — the + adventurousness dial's "Safer" end raises this for higher-confidence picks. + + Returns up to ``limit`` :class:`RecommendedArtist`, highest score first. + """ + owned = {_norm(a) for a in (owned_artist_names or set())} + seed_norms = {_norm(s.get("name")) for s in seeds} + seed_norms.discard("") + exclude = owned | seed_norms + + acc: Dict[str, dict] = {} + for seed in seeds: + s_name = _norm(seed.get("name")) + if not s_name: + continue + s_display = str(seed.get("name") or "").strip() + weight = _positive_float(seed.get("weight", 1.0)) + for sim in similars_by_seed.get(s_name, ()) or (): + a_norm = _norm(sim.get("name")) + if not a_norm or a_norm in exclude: + continue + sim_score = _positive_float(sim.get("score", 1.0)) + row = acc.setdefault( + a_norm, {"name": str(sim.get("name") or "").strip(), "score": 0.0, "seeds": {}} + ) + row["score"] += weight * sim_score + row["seeds"].setdefault(s_name, s_display) # one seed counts once + + out: List[RecommendedArtist] = [] + floor = max(1, int(min_seed_count)) + for row in acc.values(): + seed_count = len(row["seeds"]) + if seed_count < floor: + continue + out.append(RecommendedArtist( + name=row["name"], + score=round(row["score"], 6), + seed_count=seed_count, + seeds=list(row["seeds"].values()), + )) + out.sort(key=lambda r: (-r.score, -r.seed_count, r.name.lower())) + return out[:limit] + + +def aggregate_candidate_tracks( + recommended_artists: Sequence[RecommendedArtist], + top_tracks_by_artist: Dict[str, Sequence[dict]], + owned_track_keys: Optional[Set] = None, + *, + per_artist: int = 3, + limit: int = 50, + exclude_owned: bool = True, +) -> List[dict]: + """Build the candidate track list for the generated playlist. + + Takes the top ``per_artist`` tracks from each recommended artist **in artist-rank + order**, dedups by ``(artist, title)``, optionally drops owned tracks (the + "discovery" flavor) and caps at ``limit``. Each returned track dict is the source + track plus ``_seed_artist`` (which recommended artist it came from). + + Args: + recommended_artists: ranked output of :func:`rank_recommended_artists`. + top_tracks_by_artist: ``{artist_name_lower: [track_dict, ...]}`` — fetched by + the caller (Last.fm / source top tracks), NOT limited to a curated pool. + owned_track_keys: set of ``(artist_lower, title_lower)`` already in the library. + exclude_owned: drop tracks in ``owned_track_keys`` (discovery flavor). Set False + for a "replay" playlist of tracks you already own. + """ + owned = owned_track_keys or set() + seen: Set = set() + out: List[dict] = [] + for art in recommended_artists: + tracks = top_tracks_by_artist.get(_norm(art.name), ()) or () + taken = 0 + for t in tracks: + if taken >= per_artist: + break + title = str(t.get("name") or t.get("title") or "").strip() + if not title: + continue + key = (_norm(art.name), _norm(title)) + if key in seen: + continue + if exclude_owned and key in owned: + continue + seen.add(key) + out.append({**t, "_seed_artist": art.name}) + taken += 1 + if len(out) >= limit: + break + return out[:limit] + + +__all__ = [ + "RecommendedArtist", + "rank_recommended_artists", + "aggregate_candidate_tracks", +] diff --git a/tests/discovery/test_listening_recommendations.py b/tests/discovery/test_listening_recommendations.py new file mode 100644 index 00000000..ce562ca6 --- /dev/null +++ b/tests/discovery/test_listening_recommendations.py @@ -0,0 +1,117 @@ +"""Listening-driven recommendation core (#913) — pure ranking + candidate aggregation.""" + +from __future__ import annotations + +from core.discovery.listening_recommendations import ( + aggregate_candidate_tracks, + rank_recommended_artists, +) + + +def _seed(name, weight=1.0): + return {"name": name, "weight": weight} + + +# ── rank_recommended_artists ───────────────────────────────────────────────── +def test_consensus_outranks_single_endorsement(): + # 'Common' is similar to BOTH seeds; 'Solo' to one. Equal weights/scores. + seeds = [_seed("A"), _seed("B")] + sims = { + "a": [{"name": "Common"}, {"name": "Solo"}], + "b": [{"name": "Common"}], + } + out = rank_recommended_artists(seeds, sims) + assert [r.name for r in out] == ["Common", "Solo"] + assert out[0].seed_count == 2 + assert sorted(out[0].seeds) == ["A", "B"] + assert out[1].seed_count == 1 + + +def test_play_weight_boosts_a_seeds_similars(): + seeds = [_seed("Fav", weight=100), _seed("Minor", weight=1)] + sims = {"fav": [{"name": "FromFav"}], "minor": [{"name": "FromMinor"}]} + out = rank_recommended_artists(seeds, sims) + assert out[0].name == "FromFav" # heavier seed's similar wins + + +def test_similarity_score_weights_within_a_seed(): + seeds = [_seed("A")] + sims = {"a": [{"name": "Close", "score": 0.9}, {"name": "Far", "score": 0.1}]} + out = rank_recommended_artists(seeds, sims) + assert [r.name for r in out] == ["Close", "Far"] + + +def test_owned_and_seed_artists_are_excluded(): + seeds = [_seed("A"), _seed("B")] + sims = {"a": [{"name": "Owned"}, {"name": "B"}, {"name": "New"}]} # 'B' is a seed + out = rank_recommended_artists(seeds, sims, owned_artist_names={"owned"}) + assert [r.name for r in out] == ["New"] # Owned dropped, seed B dropped + + +def test_min_seed_count_filters_low_consensus(): + seeds = [_seed("A"), _seed("B")] + sims = {"a": [{"name": "Common"}, {"name": "Solo"}], "b": [{"name": "Common"}]} + out = rank_recommended_artists(seeds, sims, min_seed_count=2) + assert [r.name for r in out] == ["Common"] # 'Solo' (1 seed) dropped + + +def test_case_insensitive_dedup_and_matching(): + seeds = [_seed("Radiohead")] + sims = {"radiohead": [{"name": "Muse"}, {"name": "MUSE"}]} # same artist twice + out = rank_recommended_artists(seeds, sims) + assert len(out) == 1 and out[0].name in ("Muse", "MUSE") + assert out[0].score == 2.0 # accumulated (still one seed) + assert out[0].seed_count == 1 + + +def test_empty_and_limit(): + assert rank_recommended_artists([], {}) == [] + seeds = [_seed("A")] + sims = {"a": [{"name": f"S{i}"} for i in range(10)]} + assert len(rank_recommended_artists(seeds, sims, limit=3)) == 3 + + +# ── aggregate_candidate_tracks ─────────────────────────────────────────────── +def _recs(*names): + return rank_recommended_artists( + [_seed(n) for n in names], + {n.lower(): [{"name": f"sim-{n}"}] for n in names}, + ) + + +def test_aggregate_caps_per_artist_and_total_in_rank_order(): + recs = _recs("A", "B") # -> recommended sim-A, sim-B + tracks = { + "sim-a": [{"name": "a1"}, {"name": "a2"}, {"name": "a3"}], + "sim-b": [{"name": "b1"}, {"name": "b2"}], + } + out = aggregate_candidate_tracks(recs, tracks, per_artist=2, limit=10) + names = [t["name"] for t in out] + assert names == ["a1", "a2", "b1", "b2"] # per_artist=2, rank order + assert all(t["_seed_artist"].startswith("sim-") for t in out) + + +def test_aggregate_excludes_owned_when_requested(): + recs = _recs("A") + tracks = {"sim-a": [{"name": "Owned Song"}, {"name": "New Song"}]} + owned = {("sim-a", "owned song")} + out = aggregate_candidate_tracks(recs, tracks, owned, per_artist=5, exclude_owned=True) + assert [t["name"] for t in out] == ["New Song"] + # replay flavor keeps owned + keep = aggregate_candidate_tracks(recs, tracks, owned, per_artist=5, exclude_owned=False) + assert [t["name"] for t in keep] == ["Owned Song", "New Song"] + + +def test_aggregate_dedups_and_respects_total_limit(): + recs = _recs("A", "B") + tracks = {"sim-a": [{"name": "dup"}], "sim-b": [{"name": "dup"}, {"name": "x"}]} + # 'dup' under sim-a and sim-b are different (artist,title) keys -> both kept; + # within an artist a repeat would dedup. Here check the total limit instead. + out = aggregate_candidate_tracks(recs, tracks, per_artist=5, limit=2) + assert len(out) == 2 + + +def test_aggregate_skips_artist_with_no_tracks(): + recs = _recs("A", "B") + out = aggregate_candidate_tracks(recs, {"sim-a": [{"name": "only"}]}, per_artist=5) + assert [t["name"] for t in out] == ["only"] # sim-b had no tracks -> skipped From c21031b9bcc0495ff38ef84ee42bb912270ebad2 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 23 Jun 2026 22:53:57 -0700 Subject: [PATCH 17/20] #913: add group_similars_by_seed assembly helper (pure, tested) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The stored similar_artists rows key the similar artist by the SEED's source/db id, not its name, so rank_recommended_artists can't consume them directly. group_similars_by_seed resolves each row's source id to a seed name via a caller-supplied id_to_name map and reshapes to the {seed_name: [{'name': similar}]} the ranker wants — the fragile id->name join, now pure + tested (dataclass + dict rows, unknown-id drop, non-seed drop, group->rank end-to-end). 15 tests total. --- core/discovery/listening_recommendations.py | 39 ++++++++++++++ .../test_listening_recommendations.py | 52 +++++++++++++++++++ 2 files changed, 91 insertions(+) diff --git a/core/discovery/listening_recommendations.py b/core/discovery/listening_recommendations.py index e8683dae..9af7b417 100644 --- a/core/discovery/listening_recommendations.py +++ b/core/discovery/listening_recommendations.py @@ -39,6 +39,44 @@ def _positive_float(value: object, default: float = 1.0) -> float: return f if f > 0 else default +def _get(row: object, attr: str): + """Read a field from a dataclass row or a dict row.""" + if isinstance(row, dict): + return row.get(attr) + return getattr(row, attr, None) + + +def group_similars_by_seed( + seeds: Sequence[dict], + similar_rows: Sequence, + id_to_name: Dict[str, str], + *, + source_id_attr: str = "source_artist_id", + similar_name_attr: str = "similar_artist_name", +) -> Dict[str, List[dict]]: + """Reshape flat ``similar_artists`` rows into ``{seed_name_lower: [{'name': similar}]}``. + + The stored rows key the similar artist by the SEED's source id (``source_artist_id``), + not its name, so :func:`rank_recommended_artists` can't consume them directly. This + resolves each row's source id to a name via ``id_to_name`` (``{source_artist_id: + artist_name}`` for the library, built by the caller) and keeps only rows that resolve + to one of the ``seeds``. Rows may be dataclass objects or dicts. Pure — no I/O. + """ + seed_names = {_norm(s.get("name")) for s in seeds} + seed_names.discard("") + id_to_norm = {str(k): _norm(v) for k, v in (id_to_name or {}).items()} + + out: Dict[str, List[dict]] = {} + for row in similar_rows or (): + seed_name = id_to_norm.get(str(_get(row, source_id_attr) or ""), "") + if not seed_name or seed_name not in seed_names: + continue + sim_name = str(_get(row, similar_name_attr) or "").strip() + if sim_name: + out.setdefault(seed_name, []).append({"name": sim_name}) + return out + + @dataclass class RecommendedArtist: """One artist recommended from your listening, with the why.""" @@ -161,6 +199,7 @@ def aggregate_candidate_tracks( __all__ = [ "RecommendedArtist", + "group_similars_by_seed", "rank_recommended_artists", "aggregate_candidate_tracks", ] diff --git a/tests/discovery/test_listening_recommendations.py b/tests/discovery/test_listening_recommendations.py index ce562ca6..a4610c80 100644 --- a/tests/discovery/test_listening_recommendations.py +++ b/tests/discovery/test_listening_recommendations.py @@ -115,3 +115,55 @@ def test_aggregate_skips_artist_with_no_tracks(): recs = _recs("A", "B") out = aggregate_candidate_tracks(recs, {"sim-a": [{"name": "only"}]}, per_artist=5) assert [t["name"] for t in out] == ["only"] # sim-b had no tracks -> skipped + + +# ── group_similars_by_seed (id->name join) ─────────────────────────────────── +from dataclasses import dataclass as _dc # noqa: E402 + +from core.discovery.listening_recommendations import group_similars_by_seed # noqa: E402 + + +@_dc +class _Row: + source_artist_id: str + similar_artist_name: str + + +def test_group_resolves_source_id_to_seed_name(): + seeds = [_seed("Radiohead"), _seed("Bjork")] + rows = [ + _Row("id-rh", "Muse"), + _Row("id-rh", "Coldplay"), + _Row("id-bj", "Portishead"), + _Row("id-unknown", "Nobody"), # id not in map -> dropped + ] + id_to_name = {"id-rh": "Radiohead", "id-bj": "Bjork"} + out = group_similars_by_seed(seeds, rows, id_to_name) + assert {n["name"] for n in out["radiohead"]} == {"Muse", "Coldplay"} + assert [n["name"] for n in out["bjork"]] == ["Portishead"] + assert "id-unknown" not in out and "Nobody" not in str(out) + + +def test_group_keeps_only_rows_for_actual_seeds(): + # id resolves to a name, but that name isn't a seed -> dropped. + seeds = [_seed("A")] + rows = [_Row("id-a", "SimA"), _Row("id-x", "SimX")] + out = group_similars_by_seed(seeds, rows, {"id-a": "A", "id-x": "X"}) + assert list(out.keys()) == ["a"] + + +def test_group_accepts_dict_rows(): + seeds = [_seed("A")] + rows = [{"source_artist_id": "id-a", "similar_artist_name": "SimA"}] + out = group_similars_by_seed(seeds, rows, {"id-a": "A"}) + assert out["a"] == [{"name": "SimA"}] + + +def test_group_then_rank_end_to_end(): + # The two-step the scanner will run: group rows, then rank. + seeds = [_seed("A", weight=2), _seed("B", weight=1)] + rows = [_Row("ia", "Common"), _Row("ia", "Solo"), _Row("ib", "Common")] + grouped = group_similars_by_seed(seeds, rows, {"ia": "A", "ib": "B"}) + ranked = rank_recommended_artists(seeds, grouped, owned_artist_names={"solo"}) + assert ranked[0].name == "Common" and ranked[0].seed_count == 2 + assert all(r.name != "Solo" for r in ranked) # owned excluded From 03926bd6e20c010332eef0448f290fc8ffd4ad85 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 23 Jun 2026 22:58:04 -0700 Subject: [PATCH 18/20] #913 phase 1: generate listening recommendations during the watchlist scan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wires the pure recommendation core into curate_discovery_playlists via a new self-contained, double-guarded method _build_listening_recommendations: gathers inputs already in the DB (top 30 played artists as seeds + play_count weight, the similar_artists graph, a library id->name map + owned set, discovery-pool tracks) — NO new network — runs group_similars_by_seed -> rank -> aggregate, and stores results under NEW keys (metadata 'listening_recs_artists', curated playlist 'listening_recs_tracks'). Additive: touches no existing BYLT/curation logic, writes no existing key, and both the call site and the method body are try/except-wrapped so it can never disturb the scan. Phase-1 candidate tracks come from the discovery pool (like BYLT); a later phase swaps in a direct top-tracks fetch for pool-independent coverage. py_compile + ruff clean; 51 watchlist tests green. --- core/watchlist_scanner.py | 82 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/core/watchlist_scanner.py b/core/watchlist_scanner.py index d4584519..33a74cf4 100644 --- a/core/watchlist_scanner.py +++ b/core/watchlist_scanner.py @@ -3971,6 +3971,14 @@ class WatchlistScanner: except Exception as e: logger.debug(f"Error building BYLT for {played_artist.get('name', '?')}: {e}") + # #913: listening-driven recommendations — consensus-ranked artists you'd love but + # don't own + candidate playlist tracks. Self-contained + double-guarded so it can + # never affect the scan. + try: + self._build_listening_recommendations(profile_id, sources_to_process) + except Exception as _lr_err: # noqa: BLE001 — never let recs break the scan + logger.debug("[Listening Recs] skipped: %s", _lr_err) + # Also save without suffix for backward compatibility (use first active source). active_source = sources_to_process[0] release_radar_key = f'release_radar_{active_source}' @@ -3989,6 +3997,80 @@ class WatchlistScanner: import traceback traceback.print_exc() + def _build_listening_recommendations(self, profile_id, sources_to_process): + """#913: consensus-ranked artists you'd love but don't own, plus candidate playlist + tracks, derived from your most-played artists + the similar_artists graph. + + The ranking lives in core.discovery.listening_recommendations (pure + tested); this only + gathers inputs — all already in the DB, NO new network — and stores the result under NEW + metadata/curated keys. Fully self-contained and guarded: any failure logs and returns, so + it can never disturb the scan. Phase-1 candidate tracks come from the discovery pool (like + BYLT); a later phase swaps in a direct top-tracks fetch for richer coverage. + """ + try: + import json as _json + from core.discovery.listening_recommendations import ( + aggregate_candidate_tracks, + group_similars_by_seed, + rank_recommended_artists, + ) + + seeds = [{'name': s['name'], 'weight': s.get('play_count', 1)} + for s in (self.database.get_top_artists('all', 30) or []) if s.get('name')] + if not seeds: + return + + # id -> name + owned-artist set for the WHOLE library (similar_artists rows key the + # similar artist by the seed artist's id). + id_to_name, owned = {}, set() + with self.database._get_connection() as conn: + cur = conn.cursor() + cur.execute("SELECT id, name FROM artists WHERE name IS NOT NULL AND name != ''") + for row in cur.fetchall(): + id_to_name[str(row[0])] = row[1] + owned.add((row[1] or '').lower()) + + similar_rows = self.database.get_top_similar_artists(limit=1000, profile_id=profile_id) + similars_by_seed = group_similars_by_seed(seeds, similar_rows, id_to_name) + recs = rank_recommended_artists(seeds, similars_by_seed, owned, limit=40) + if not recs: + logger.info("[Listening Recs] no recommendations yet (no similar-artist coverage)") + return + + self.database.set_metadata('listening_recs_artists', _json.dumps([ + {'name': r.name, 'seed_count': r.seed_count, 'seeds': r.seeds[:5], 'score': r.score} + for r in recs + ])) + + # Candidate tracks from the discovery pool grouped by artist (phase 1: no new network). + pool, active_source = [], None + for src in (sources_to_process or []): + pool = self.database.get_discovery_pool_tracks( + limit=5000, new_releases_only=False, source=src, profile_id=profile_id) + if pool: + active_source = src + break + + track_ids = [] + if pool: + by_artist = {} + for t in pool: + an = (getattr(t, 'artist_name', '') or '').lower() + tid = (getattr(t, 'spotify_track_id', None) if active_source == 'spotify' + else getattr(t, 'itunes_track_id', None) if active_source == 'itunes' + else getattr(t, 'deezer_track_id', None)) + if an and tid: + by_artist.setdefault(an, []).append({'name': getattr(t, 'track_name', ''), 'id': tid}) + candidates = aggregate_candidate_tracks(recs, by_artist, per_artist=3, limit=50) + track_ids = [c['id'] for c in candidates if c.get('id')] + if track_ids: + self.database.save_curated_playlist('listening_recs_tracks', track_ids, profile_id=profile_id) + + logger.info("[Listening Recs] %d recommended artists, %d candidate tracks", + len(recs), len(track_ids)) + except Exception as e: + logger.debug("[Listening Recs] generation skipped: %s", e) + def sync_spotify_library_cache(self, profile_id=1): """Sync user's saved Spotify albums into the local cache. From 9d091207f6130c7ed193b25eeffe4949a82c1cf3 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 23 Jun 2026 23:17:50 -0700 Subject: [PATCH 19/20] fix: atomic file placement so Jellyfin can't index a half-written track (null-disc) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit safe_move_file used shutil.move, which for a CROSS-filesystem move (downloads volume -> library volume, common in Docker/NAS) copies the file to the FINAL path incrementally. A media-server real-time watcher (Jellyfin) can catch that partial file mid-write and cache it with null/incomplete metadata — tracks landing with no disc. Inconsistent because it only bites cross-fs and races the scan tick; 're-add library' fixes it (rescan reads the now-complete file) and the on-disk tags are fine — exactly the reported symptoms. Fix: same-fs uses an atomic os.replace (also overwrites dst); cross-fs copies to a HIDDEN temp sibling, fsyncs, then atomic os.replace into place (+ temp cleanup on failure). A watcher only ever sees the COMPLETE file. EXDEV/EPERM/EACCES + the old string check route here, so detection is strictly broader than before. Tests: same-fs move, simulated EXDEV routes to the atomic path and leaves no partial temp, helper completes+cleans, helper cleans temp + preserves source on failure. Existing replace-destination test still green; 574 imports+relocate tests pass. --- core/imports/file_ops.py | 72 +++++++++++++++++++------ tests/imports/test_import_file_ops.py | 75 +++++++++++++++++++++++++++ 2 files changed, 130 insertions(+), 17 deletions(-) diff --git a/core/imports/file_ops.py b/core/imports/file_ops.py index c3620d78..9923c39f 100644 --- a/core/imports/file_ops.py +++ b/core/imports/file_ops.py @@ -2,6 +2,7 @@ from __future__ import annotations +import errno import logging import os import re @@ -102,6 +103,41 @@ def cleanup_slskd_dedup_siblings(source_path) -> List[str]: return deleted +def _atomic_cross_device_move(src: Path, dst: Path) -> None: + """Move ``src`` to ``dst`` across filesystems WITHOUT ever exposing a partial file at + the final path. + + Copies into a hidden temp sibling of ``dst`` (same filesystem), fsyncs, then does an + atomic ``os.replace`` into place, then deletes ``src``. A media-server file watcher + (Jellyfin/Plex real-time monitoring) therefore only ever indexes the COMPLETE file — + an incremental in-place copy was what Jellyfin could catch mid-write and cache with + null/incomplete metadata (tracks landing with no disc). Cleans up the temp on failure. + """ + src, dst = Path(src), Path(dst) + tmp = dst.parent / f".{dst.name}.ssync-tmp" + try: + with open(src, "rb") as f_src, open(tmp, "wb") as f_dst: + shutil.copyfileobj(f_src, f_dst) + f_dst.flush() + os.fsync(f_dst.fileno()) + try: + shutil.copystat(str(src), str(tmp)) # preserve mtime/permissions (copy2-like) + except OSError: + pass + os.replace(str(tmp), str(dst)) # atomic within dst's filesystem + except Exception: + try: + if tmp.exists(): + tmp.unlink() + except OSError: + pass + raise + try: + src.unlink() + except OSError: + logger.info(f"Could not delete source after cross-device move (may be owned by another process): {src}") + + def safe_move_file(src, dst): """Move a file safely across filesystems.""" src = Path(src) @@ -129,7 +165,13 @@ def safe_move_file(src, dst): break try: - shutil.move(str(src), str(dst)) + # Same-filesystem move: an atomic rename that also overwrites dst. A media-server + # watcher (Jellyfin/Plex real-time monitoring) therefore never sees a partial file + # at the final name. Cross-filesystem raises EXDEV (some network mounts raise + # EPERM/EACCES) and we copy atomically below instead of letting the move write the + # destination incrementally — the partial-file-at-final-name is what caused tracks + # to land in Jellyfin with null/incomplete metadata (no disc). + os.replace(str(src), str(dst)) return except FileNotFoundError: if dst.exists(): @@ -137,8 +179,6 @@ def safe_move_file(src, dst): return raise except (OSError, PermissionError) as e: - error_msg = str(e).lower() - if dst.exists() and dst.stat().st_size > 0: logger.warning(f"Move raised {type(e).__name__} but destination exists, treating as success: {e}") try: @@ -147,23 +187,21 @@ def safe_move_file(src, dst): logger.info(f"Could not delete source file (may be owned by another process): {src}") return - if "cross-device" in error_msg or "operation not permitted" in error_msg or "permission denied" in error_msg: - logger.warning(f"Cross-device move detected, using fallback copy method: {e}") + error_msg = str(e).lower() + cross_device = ( + getattr(e, "errno", None) in (errno.EXDEV, errno.EPERM, errno.EACCES) + or "cross-device" in error_msg + or "operation not permitted" in error_msg + or "permission denied" in error_msg + ) + if cross_device: + logger.warning(f"Cross-device move, using atomic copy+rename: {e}") try: - with open(src, "rb") as f_src: - with open(dst, "wb") as f_dst: - shutil.copyfileobj(f_src, f_dst) - f_dst.flush() - os.fsync(f_dst.fileno()) - - try: - src.unlink() - except PermissionError: - logger.info(f"Could not delete source file (may be owned by another process): {src}") - logger.info(f"Successfully moved file using fallback method: {src} -> {dst}") + _atomic_cross_device_move(src, dst) + logger.info(f"Successfully moved file atomically across filesystems: {src} -> {dst}") return except Exception as fallback_error: - logger.error(f"Fallback copy also failed: {fallback_error}") + logger.error(f"Atomic cross-device move failed: {fallback_error}") raise raise diff --git a/tests/imports/test_import_file_ops.py b/tests/imports/test_import_file_ops.py index ce9ecd4a..a23df527 100644 --- a/tests/imports/test_import_file_ops.py +++ b/tests/imports/test_import_file_ops.py @@ -169,3 +169,78 @@ def test_read_staging_file_metadata_uses_filename_fallbacks_when_tags_are_invali "track_number": 2, "disc_number": 1, } + + +# ── atomic cross-filesystem move (Jellyfin null-disc mitigation) ────────────── +import errno # noqa: E402 +import os # noqa: E402 + +import pytest # noqa: E402 + +from core.imports import file_ops as _fo # noqa: E402 +from core.imports.file_ops import _atomic_cross_device_move # noqa: E402 + + +def test_same_fs_move_moves_and_removes_source(tmp_path): + src = tmp_path / "s.flac" + src.write_text("hello") + dst = tmp_path / "lib" / "t.flac" # parent created by safe_move_file + safe_move_file(src, dst) + assert dst.read_text() == "hello" + assert not src.exists() + + +def test_cross_device_move_routes_to_atomic_and_completes(tmp_path, monkeypatch): + # Simulate a cross-filesystem move: the same-fs os.replace raises EXDEV, and the + # atomic helper's temp->dst replace (same fs) succeeds. The file must complete and + # no partial temp file may be left at the final name's directory. + src = tmp_path / "s.flac" + src.write_text("payload") + dstdir = tmp_path / "lib" + dstdir.mkdir() + dst = dstdir / "t.flac" + + real_replace = os.replace + + def fake_replace(a, b): + if str(a) == str(src): # the cross-fs move attempt + raise OSError(errno.EXDEV, "Invalid cross-device link") + return real_replace(a, b) # the temp -> dst rename (same fs) + + monkeypatch.setattr(_fo.os, "replace", fake_replace) + safe_move_file(src, dst) + + assert dst.read_text() == "payload" + assert not src.exists() + assert not list(dstdir.glob(".*ssync-tmp")) # complete file only, no leftover temp + + +def test_atomic_helper_completes_and_cleans_temp(tmp_path): + src = tmp_path / "s.flac" + src.write_text("payload") + dstdir = tmp_path / "d" + dstdir.mkdir() + dst = dstdir / "t.flac" + _atomic_cross_device_move(src, dst) + assert dst.read_text() == "payload" + assert not src.exists() + assert not list(dstdir.glob(".*ssync-tmp")) + + +def test_atomic_helper_cleans_temp_and_keeps_source_on_failure(tmp_path, monkeypatch): + src = tmp_path / "s.flac" + src.write_text("payload") + dstdir = tmp_path / "d" + dstdir.mkdir() + dst = dstdir / "t.flac" + + def boom(_a, _b): + raise OSError("replace failed") + + monkeypatch.setattr(_fo.os, "replace", boom) + with pytest.raises(OSError): + _atomic_cross_device_move(src, dst) + + assert src.exists() # source preserved on failure + assert not dst.exists() # no partial final file + assert not list(dstdir.glob(".*ssync-tmp")) # temp cleaned up From d647fc8ad172fcf14ac0e55616bdac25e30fac6b Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 23 Jun 2026 23:24:39 -0700 Subject: [PATCH 20/20] Release 2.7.7: version bump + What's New / version modal + PR description + docker-publish default tag - _SOULSYNC_BASE_VERSION -> 2.7.7; docker-publish default version_tag -> 2.7.7 - pr_description.md rewritten for 2.7.7 (the #915 primary-source parity headline, #913 listening-recs foundation, jellyfin atomic-write, and the #905/#908/#909/#910/#911/#912/#914/#916/#917/#918 batch) - WHATS_NEW: replaced the 2.7.6 block with 2.7.7 (current release + a brief 'earlier versions' summary) - VERSION_MODAL_SECTIONS: promoted the 2.7.7 highlights, rolled 2.7.6 into the 'Earlier' aggregator --- .github/workflows/docker-publish.yml | 4 +- pr_description.md | 43 ++++++++-------- web_server.py | 2 +- webui/static/helper.js | 75 ++++++++++++++-------------- 4 files changed, 63 insertions(+), 61 deletions(-) diff --git a/.github/workflows/docker-publish.yml b/.github/workflows/docker-publish.yml index dc089154..48dac197 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.7.6)' + description: 'Version tag (e.g. 2.7.7)' required: true - default: '2.7.6' + default: '2.7.7' jobs: build-and-push: diff --git a/pr_description.md b/pr_description.md index 501ebcc7..22a15ec1 100644 --- a/pr_description.md +++ b/pr_description.md @@ -1,41 +1,44 @@ -# soulsync 2.7.6 — `dev` → `main` +# soulsync 2.7.7 — `dev` → `main` -patch release on top of 2.7.5. the headline is going the *other* way with playlists — exporting them TO listenbrainz — plus youtube liked-music sync, a deep-scan data-loss guard, and a round of dashboard performance work. +a fix-heavy patch on top of 2.7.6 — a big sweep of reported issues, the start of listening-driven recommendations, and a metadata-parity fix that stops downloads from needing a manual reorganize afterward. --- ## what's new -### export playlists to listenbrainz (#903) -soulsync already pulls playlists IN from everywhere — now it can push one back OUT. every mirrored-playlist card gets a 📤 export button: pick **download .jspf** (a standard playlist file you can hand-upload anywhere) or **sync to listenbrainz** (creates the playlist straight on your LB account). each track is matched to its musicbrainz *recording* id via a cheapest-first waterfall — cache → your library (`musicbrainz_recording_id`) → the file's own tag → a live musicbrainz lookup — with the result cached so the same song never costs twice. live "matching N/M · X matched" status shows on the card, and re-syncing **updates the same LB playlist in place** instead of making duplicates. tracks that can't be resolved to an MBID are skipped (LB requires them) and counted so you see the coverage. +### downloads now tag + path like reorganize does (#915) +the headline fix. when you add or redownload music, post-processing used to backfill missing album data from **spotify only** — so an iTunes/deezer-primary user kept a "lean" context and the path **dropped the `$year`** while the release date defaulted to `YYYY-01-01`. you'd then run a reorganize to fix it every time. now post-processing (and redownload) pull the full album from your **primary metadata source** — the exact same place reorganize/enrich read — so the year, real release date, and album type are right the first time. covers the add/download flow and single-track redownload (iTunes + deezer). -### youtube liked-music sync (#902) -you can now sync your youtube music **Liked Music** playlist (`music.youtube.com/playlist?list=LM`). it's a private playlist, so it needs auth — and the existing "read a browser's cookies" option only works when the browser is on the same machine. added a **"paste cookies.txt"** option in Settings → YouTube so server/docker installs (and anyone on a browser like Zen that yt-dlp can't read) can supply their login from anywhere. +### listening-driven recommendations — foundation (#913) +the start of "discover based on what you actually listen to." during the watchlist scan, soulsync now ranks artists you'd love but don't own — seeded from your top-played artists, scored by **consensus** (who's similar to *many* of your favorites), play weight, and similarity strength — and builds a candidate track list from them. generated and stored now; the discover row + synced playlist come next. -### deep scan won't relocate your library (#904) -a standalone Deep Scan moves files it doesn't recognize into Staging for import. if the DB was empty/out of sync with disk (a volume swap, a DB reset, external tag edits), it treated your **entire** library as "unrecognized" and relocated all of it — one user lost ~1,500 tracks into Staging. now a guard refuses the move when the unrecognized share is implausibly large (the desync signature), leaves everything in place, and warns instead. plus a **"Transfer is my permanent library — never move files out"** toggle for people whose Transfer folder *is* their live library. - -### dashboard performance -a pass at the "soulsync makes my GPU work hard just sitting there" complaints. the sidebar sweep animates `transform` instead of `left` (no per-frame layout), particle glows are pre-rendered sprites instead of per-frame gradients, blur radii + redundant/invisible card shadows are trimmed, and low-power machines auto-drop to performance mode. all the visible effects stay; they're just cheaper to draw. +### jellyfin stops indexing half-written tracks +multi-disc tracks landing with "no disc" in jellyfin turned out to be a write race: a cross-filesystem move (downloads volume → library volume) wrote the file to its final path **incrementally**, and jellyfin's real-time watcher could catch it mid-write and cache incomplete metadata. now the final placement is **atomic** — copy to a hidden temp sibling, then an atomic rename — so a watcher only ever sees the complete file. ### fixes -- **file-import manual matches stick (#901)** — a manual match on a file-imported playlist track is no longer forgotten on re-sync (the tracks now carry a stable id; existing ones are backfilled once). -- **manual match heals a stale Plex key** — a Find & Add match whose stored Plex ratingKey went stale is now re-resolved against a live Plex search instead of silently breaking. -- **multi-disc albums** — a track now files into the Disc folder that matches its own disc tag (no more disc-2 tracks landing in Disc 1), and a track is never written disc-less. -- **auto-download track numbers** — a track auto-grabbed from the playlist pipeline / wishlist / watchlist now gets its real in-album position instead of being tagged `1/1`. +- **navidrome playlists doubling (#905)** — every resync re-added the whole playlist (a 4-song list grew to 12). reconcile read the server's current tracks via a missing attribute, so it always thought the playlist was empty. fixed; also pushes a deduped list. +- **youtube playlists capped at ~100 (#908)** — a yt-dlp/youtube regression truncated big playlists (Liked Music came back as 104). worked around to page past it (~200) until the upstream fix lands. +- **album redownload grabbed the wrong edition (#911)** — it did a fresh search instead of using the album's matched source id, so a 66-track OST could redownload as a 19-track single. now uses the canonical matched source. +- **iTunes albums over 50 tracks truncated (#918)** — the iTunes lookup defaulted to 50 entities; now requests the full album. +- **enhanced view showed multi-disc tracks as missing (#916)** — owned disc-2+ tracks (stored as disc 1) no longer flag as "missing"; matched by title like reorganize. +- **reorganize vs "(feat. X)" (#914)** — a bare local title now matches an iTunes track titled "Song (feat. Artist)" instead of reporting it not-in-tracklist. +- **"I have this" dropped the year (#917)** — it rebuilt a yearless path and copied into a new folder; now reuses the album's existing folder. +- **full refresh imported 0 tracks (#910)** — every track insert failed on a missing `year` column; added it + a migration so older DBs self-heal. +- **youtube discovery "Unknown Artist" (#909)** — when youtube hands back only a title, the matched artist now backfills the column instead of leaving "Unknown Artist". +- **empty folder cleaner toggle did nothing (#912)** — the "also remove image/sidecar-only folders" option read the wrong config key; now honored. --- ## a brief recap of what came before -2.7.5 was a fix-heavy cycle — matching & artwork accuracy, the HiFi preview mess (#895) — plus M3U import (#893), ignore-list management (#897), and per-playlist file naming. 2.7.4 was re-identify; 2.7.3 the Quality Upgrade Finder + ignore-list (#874); 2.7.2 playlist-folder mirroring + M3U export; 2.7.1 download verification + a review queue (#852); 2.7.0 made multi-user real. +2.7.6 went the *other* way with playlists — exporting them TO listenbrainz — plus youtube liked-music sync, a deep-scan data-loss guard (#904), and dashboard performance work. 2.7.5 was matching & artwork accuracy + M3U import; 2.7.4 re-identify; 2.7.3 the Quality Upgrade Finder; 2.7.2 playlist-folder mirroring; 2.7.1 download verification; 2.7.0 made multi-user real. --- ## tests -additive + fail-safe — new behavior is opt-in or guarded, nothing existing rewired. new seam/regression suites across the listenbrainz export (JSPF build, the MBID waterfall + dedup, the persistent cache, the LB create/update-in-place client), the youtube cookie precedence, and the #904 deep-scan guard (incl. the 1,500-file regression). the listenbrainz push + update-in-place were also validated live against a real account. relevant suites green; `ruff check` clean on touched modules. +additive + fail-safe — new behavior is guarded or scoped, nothing existing rewired. new seam/regression suites across the `year`-column migration (#910), the navidrome reconcile fix (#905 — reverting the one-char change flips the tests red), feat-matching (#914), the multi-disc not-missing logic (#916), the iTunes full-album limit (#918, proven live against the real API), the "I have this" year recovery (#917), the primary-source backfill (#915), the listening-recs core (#913), and atomic file placement. relevant suites green; `ruff check` clean repo-wide. ## post-merge -- [ ] tag `v2.7.6` on `main` -- [ ] docker-publish with `version_tag: 2.7.6` +- [ ] tag `v2.7.7` on `main` +- [ ] docker-publish with `version_tag: 2.7.7` - [ ] discord announce (auto-fired by the workflow) -- [ ] reply on #902 / #903 / #904 +- [ ] reply on the issue batch (#905 / #908 / #909 / #910 / #911 / #912 / #913 / #914 / #915 / #916 / #917 / #918) diff --git a/web_server.py b/web_server.py index 51bfc4c5..a282de9a 100644 --- a/web_server.py +++ b/web_server.py @@ -40,7 +40,7 @@ logger = setup_logging(_log_level, _log_path) # App version — single source of truth for backup metadata, system-info, update check, etc. # Semver: MAJOR.MINOR.PATCH. Bump at each dev→main release. -_SOULSYNC_BASE_VERSION = "2.7.6" +_SOULSYNC_BASE_VERSION = "2.7.7" def _build_version_string(): """Append short commit hash to version when available (e.g. 2.35+abc1234).""" diff --git a/webui/static/helper.js b/webui/static/helper.js index 10173057..98f77c85 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3404,17 +3404,22 @@ function closeHelperSearch() { const WHATS_NEW = { // Convention: keep only the CURRENT release here, plus a single brief // "Earlier versions" summary entry. Don't accumulate old per-version blocks. - '2.7.6': [ - { date: 'June 2026 — 2.7.6 release' }, - { title: 'Export playlists to ListenBrainz (#903)', desc: 'soulsync already pulls playlists IN — now it can push one OUT. every mirrored-playlist card gets a 📤 export button: download a standard .jspf file, or sync the playlist straight to your ListenBrainz account. each track is matched to its musicbrainz recording id (cache → your library → file tag → live musicbrainz), with live "matching N/M" status on the card. re-syncing updates the SAME LB playlist in place instead of duplicating it.', page: 'sync' }, - { title: 'YouTube Liked Music sync (#902)', desc: 'you can now sync your youtube music "Liked Music" playlist (music.youtube.com/playlist?list=LM). it\'s private, so it needs auth — added a "paste cookies.txt" option in Settings → YouTube so server/docker installs (or browsers yt-dlp can\'t read, like Zen) can supply their login from anywhere.', page: 'settings' }, - { title: 'Deep Scan won\'t relocate your library (#904)', desc: 'a standalone Deep Scan moves unrecognized files into Staging. if the DB was empty/out of sync with disk, it treated your ENTIRE library as unrecognized and relocated all of it. now a guard refuses the move when the unrecognized share is implausibly large, leaves files in place, and warns — plus a "Transfer is my permanent library — never move files out" toggle.', page: 'settings' }, - { title: 'Dashboard performance', desc: 'a pass at "soulsync works my GPU hard just sitting there". the sidebar sweep animates transform not left, particle glows are cached sprites, blur radii + redundant/invisible card shadows are trimmed, and low-power machines auto-drop to performance mode. the effects all stay — they\'re just cheaper to draw.', page: 'dashboard' }, - { title: 'File-import manual matches stick (#901)', desc: 'a manual match on a file-imported playlist track is no longer forgotten on re-sync — the tracks now carry a stable id (existing ones backfilled once), so your pick survives.', page: 'sync' }, - { title: 'Manual match heals a stale Plex key', desc: 'a Find & Add match whose stored Plex ratingKey went stale is now re-resolved against a live Plex search instead of silently breaking on the next sync.', page: 'sync' }, - { title: 'Multi-disc albums', desc: 'a track now files into the Disc folder that matches its own disc tag (no more disc-2 tracks landing in Disc 1), and a track is never written disc-less.', page: 'downloads' }, - { title: 'Auto-download track numbers', desc: 'a track auto-grabbed from the playlist pipeline / wishlist / watchlist now gets its real in-album position instead of being tagged 1/1.', page: 'downloads' }, - { title: 'Earlier versions', desc: '2.7.5 was a fix-heavy cycle — matching & artwork accuracy, the HiFi preview mess (#895), M3U import (#893), ignore-list management (#897), per-playlist file naming. 2.7.4 added re-identify; 2.7.3 the Quality Upgrade Finder + ignore-list (#874); 2.7.2 playlist-folder mirroring + M3U export; 2.7.1 download verification + a review queue (#852); 2.7.0 made multi-user real.' }, + '2.7.7': [ + { date: 'June 2026 — 2.7.7 release' }, + { title: 'Downloads tag + path like Reorganize (#915)', desc: 'adding/redownloading music used to backfill missing album data from Spotify ONLY — so an iTunes/Deezer-primary user kept a "lean" context, the path dropped the $year and the date defaulted to YYYY-01-01 until you ran a reorganize. now post-processing AND redownload pull the full album from your PRIMARY metadata source (the same place reorganize/enrich read), so the year, real release date and album type are right the first time.', page: 'downloads' }, + { title: 'Listening-driven recommendations — foundation (#913)', desc: 'the start of "discover based on what you actually listen to". during the watchlist scan, soulsync now ranks artists you\'d love but don\'t own — seeded from your top-played artists, scored by consensus (similar to MANY of your favorites), play weight and similarity — and builds a candidate track list. generated + stored now; the discover row + synced playlist come next.', page: 'discover' }, + { title: 'Jellyfin stops indexing half-written tracks', desc: 'multi-disc tracks landing with "no disc" in jellyfin was a write race — a cross-filesystem move wrote the file to its final path incrementally and jellyfin caught it mid-write. final placement is now atomic (temp sibling + atomic rename), so a watcher only ever sees the COMPLETE file.', page: 'downloads' }, + { title: 'Navidrome playlists doubling (#905)', desc: 'every resync re-added the whole playlist (a 4-song list grew to 12) — reconcile read the server\'s current tracks via a missing attribute, so it always thought the playlist was empty. fixed, plus a deduped push.', page: 'sync' }, + { title: 'YouTube playlists capped at ~100 (#908)', desc: 'a yt-dlp/youtube regression truncated big playlists (Liked Music came back as 104). worked around to page past it (~200) until the upstream fix lands.', page: 'sync' }, + { title: 'Album redownload grabbed the wrong edition (#911)', desc: 'redownload did a fresh search instead of using the album\'s matched source id, so a 66-track OST could come back as a 19-track single. now uses the canonical matched source.', page: 'library' }, + { title: 'iTunes albums over 50 tracks truncated (#918)', desc: 'the iTunes lookup defaulted to 50 entities, cutting big albums off in the download window. now requests the full album.', page: 'library' }, + { title: 'Enhanced view showed multi-disc tracks as missing (#916)', desc: 'owned disc-2+ tracks (stored as disc 1) no longer flag as "missing" — matched by title like reorganize.', page: 'library' }, + { title: 'Reorganize vs "(feat. X)" (#914)', desc: 'a bare local title now matches an iTunes track titled "Song (feat. Artist)" instead of being reported not-in-tracklist.', page: 'library' }, + { title: '"I have this" dropped the year (#917)', desc: 'it rebuilt a yearless path and copied into a NEW folder; now reuses the album\'s existing folder.', page: 'library' }, + { title: 'Full Refresh imported 0 tracks (#910)', desc: 'every track insert failed on a missing year column; added it + a migration so older DBs self-heal.', page: 'settings' }, + { title: 'YouTube discovery "Unknown Artist" (#909)', desc: 'when youtube hands back only a title, the matched artist now backfills the column instead of leaving "Unknown Artist".', page: 'sync' }, + { title: 'Empty Folder Cleaner toggle did nothing (#912)', desc: 'the "also remove image/sidecar-only folders" option read the wrong config key; now honored.', page: 'settings' }, + { title: 'Earlier versions', desc: '2.7.6 went the OTHER way with playlists — exporting them TO listenbrainz (#903) — plus youtube liked-music sync (#902), a deep-scan data-loss guard (#904), and dashboard perf. 2.7.5 was matching & artwork accuracy + M3U import; 2.7.4 re-identify; 2.7.3 the Quality Upgrade Finder; 2.7.2 playlist-folder mirroring; 2.7.1 download verification; 2.7.0 made multi-user real.' }, ], }; @@ -3445,45 +3450,39 @@ const WHATS_NEW = { // usage_note?: 'optional hint shown at the bottom' } const VERSION_MODAL_SECTIONS = [ { - title: "Export playlists to ListenBrainz (#903)", - description: "soulsync already pulls playlists IN from everywhere — now it can push one back OUT.", + title: "Downloads tag + path like Reorganize (#915)", + description: "the headline fix — adding or redownloading music now gets the year, release date and album type right the FIRST time, instead of needing a manual reorganize after.", features: [ - "📤 export button on every mirrored-playlist card — download a standard .jspf file, or sync the playlist straight onto your ListenBrainz account", - "each track is matched to its musicbrainz recording id via a cheapest-first waterfall — cache → your library → the file's own tag → a live musicbrainz lookup — and the result is cached so the same song never costs twice", - "live \"matching N/M · X matched\" status on the card, and re-syncing updates the SAME LB playlist in place instead of making duplicates", - ], - usage_note: "find it on the 📤 button when you hover a mirrored playlist card. tracks that can't be resolved to a musicbrainz id are skipped (LB requires them) and counted, so you see the coverage.", - }, - { - title: "YouTube Liked Music sync (#902)", - description: "sync your youtube music \"Liked Music\" playlist (music.youtube.com/playlist?list=LM).", - features: [ - "it's a private playlist, so it needs auth — the existing browser-cookie option only works when the browser is on the same machine as soulsync", - "added a \"paste cookies.txt\" option in Settings → YouTube so server/docker installs (or browsers yt-dlp can't read, like Zen) can supply their login from anywhere", + "post-processing used to backfill missing album data from Spotify ONLY — so an iTunes/Deezer-primary user kept a \"lean\" context, the path dropped the $year, and the release date defaulted to YYYY-01-01", + "now post-processing AND single-track redownload pull the full album from your PRIMARY metadata source — the exact same place reorganize / manual enrich read", + "so the $year folder, real release date and album type land correctly on the first pass (iTunes + Deezer)", ], }, { - title: "Deep Scan won't relocate your library (#904)", - description: "a standalone Deep Scan moves unrecognized files into Staging — which went very wrong when the DB was out of sync with disk.", + title: "Listening-driven recommendations — foundation (#913)", + description: "the start of \"discover based on what you actually listen to.\"", features: [ - "if the DB was empty/desynced, a scan treated your ENTIRE library as unrecognized and relocated all of it (one user lost ~1,500 tracks into Staging)", - "now a guard refuses the move when the unrecognized share is implausibly large (the desync signature), leaves everything in place, and warns instead", - "new \"Transfer is my permanent library — never move files out\" toggle for people whose Transfer folder IS their live library", + "during the watchlist scan, soulsync ranks artists you'd love but don't own — seeded from your top-played artists", + "scored by consensus (similar to MANY of your favorites), play weight, and similarity strength — not just \"appears in a list\"", + "generated + stored now; the discover row + a synced playlist come next", ], }, { - title: "Dashboard performance + fixes", - description: "lighter dashboard, and a handful of matching/import fixes.", + title: "A big batch of fixes", + description: "a fix-heavy cycle — playlists, downloads, multi-disc and the enhanced view.", features: [ - "dashboard — the sidebar sweep animates transform not left, particle glows are cached sprites, blur/shadow excess is trimmed, and low-power machines auto-drop to performance mode (the effects all stay, just cheaper to draw)", - "#901 — a manual match on a file-imported playlist track now survives re-sync (stable ids, existing ones backfilled)", - "a Find & Add match whose stored Plex key went stale is re-resolved against a live Plex search instead of breaking", - "multi-disc albums file each track in the Disc folder matching its tag (no disc-less / wrong-disc tracks); auto-grabbed tracks get their real in-album number instead of 1/1", + "jellyfin \"no disc\" tracks — a cross-filesystem move wrote the file to its final path incrementally and jellyfin caught it mid-write; final placement is now atomic, so a watcher only ever sees the complete file", + "#905 — navidrome playlists doubling every resync (reconcile thought the playlist was empty); fixed + a deduped push", + "#908 — youtube playlists capped at ~100 (a yt-dlp/youtube regression); worked around to ~200 until upstream lands", + "#911 — album redownload grabbed the wrong edition (fresh search vs the matched source id); now uses the canonical source", + "#918 — iTunes albums over 50 tracks were truncated in the download window; now requests the full album", + "#916 — the enhanced view flagged multi-disc tracks as missing; now matched by title like reorganize", + "#914 #917 #910 #909 #912 — reorganize vs \"(feat. X)\", \"I have this\" dropping the year, Full Refresh importing 0 tracks, youtube \"Unknown Artist\", and the Empty Folder Cleaner toggle that did nothing", ], }, { - title: "Earlier in 2.7.5", - description: "a fix-heavy cycle — matching & artwork accuracy plus a few quality-of-life features.", + title: "Earlier in 2.7.6 / 2.7.5", + description: "2.7.6 went the OTHER way with playlists — exporting them TO listenbrainz (#903) — plus youtube liked-music sync (#902), a deep-scan data-loss guard (#904), and dashboard perf. before that, 2.7.5 was a fix-heavy cycle — matching & artwork accuracy plus a few quality-of-life features.", features: [ "special-edition cover art (a \"Gustave Edition\" uses its OWN cover), deezer track numbers, and the \"The\" duplicate fix", "HiFi 30-second previews disguised as full songs are caught and rejected (#895)",