From 1a2da016e4c6efbdf78f50fc03a512ec2777a5c9 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Thu, 7 May 2026 15:44:47 -0700 Subject: [PATCH] Add download buttons + bulk action to artist top-tracks sidebar MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #513 (s66jones). The artist detail page already showed a "Popular on Last.fm" sidebar — list of an artist's top tracks by playcount, with a play button per row but no download action. Issue #513 wanted a way to grab those tracks the same way zotify let users grab "top X songs" without pulling the full discography. Pulls from the configured primary metadata source (Spotify `artist_top_tracks`, Deezer `/artist/{id}/top`) when available, falls back to the existing Last.fm display-only mode for sources that don't expose popularity ranking (iTunes / Discogs / MusicBrainz). Source label in the section title shifts to match. Each row gets a hover-revealed download button that wishlists the single track via the existing /api/add-album-to-wishlist endpoint (preserves the track's real album metadata, so the wishlist worker later places the file in its proper album folder). A "Download All" footer button opens the standard download modal in PLAYLIST context, not album context — the virtual playlist_id is `top_tracks__` which doesn't match any of the album-prefix checks in `startMissingTracksProcess` (downloads.js). That keeps `is_album_download=false`, so the master worker doesn't inject a wrapper context as `_explicit_album_context`. Each track downloads using its own real album metadata, files land in proper per-album folders on disk (not a fake "Top Tracks" folder). Backend additions: - `SpotifyClient.get_artist_top_tracks(artist_id, country, limit)` — wraps `spotipy.artist_top_tracks`, returns up to 10 tracks for the market (Spotify's API cap). UI-side limit trim only. - `DeezerClient.get_artist_top_tracks(artist_id, limit)` — wraps `/artist/{id}/top?limit=N`, converts Deezer's raw shape to the same Spotify-compatible dict layout (id, name, artists, album with album_type / total_tracks / images, duration_ms, track_number, disc_number) so downstream code doesn't branch on source. - `GET /api/artist//top-tracks` — dispatches to whichever client matches the primary source. Resolves per-source artist IDs from the DB row first (matching what /discography already does) so a Spotify ID in the URL still works when Deezer is primary, and vice versa. Returns `{success, source, tracks, resolved_artist_id}` on hit; `{success: False, reason: 'unsupported_source' | 'spotify_not_authenticated' | 'deezer_unavailable' | 'no_tracks_found'}` on miss so the frontend can decide whether to fall through to Last.fm. Frontend: - `_loadArtistTopTracks` tries the metadata source first, falls through to the legacy `/api/artist/0/lastfm-top-tracks` call if the source can't deliver. Section title and per-row UI shift based on which source answered. - New per-row `.hero-top-track-download` button (hover-revealed). - New `.hero-top-tracks-download-all` footer button — only visible when metadata-source mode rendered the list (Last.fm fallback hides it since rows have no track IDs to download). Tests: 10 new tests pin the client methods — - Spotify: returns track list, honors UI limit cap, returns empty when unauthed / artist_id missing / API throws. - Deezer: shape conversion to Spotify-compatible dict, empty when no data / artist_id missing, limit clamping at upper bound, default fallback when limit=0, malformed entries skipped. The Flask endpoint dispatcher itself isn't covered by the new test file because importing web_server at test-collection time spins up worker threads that race with caplog-using tests elsewhere in the suite (specifically test_library_reorganize_orchestrator). Endpoint verified manually; the underlying client methods (the load-bearing logic) are covered. 2204/2204 full suite green (was 2194 + 10 new). --- core/deezer_client.py | 68 ++++++++ core/spotify_client.py | 25 +++ tests/test_artist_top_tracks_clients.py | 218 ++++++++++++++++++++++++ web_server.py | 102 +++++++++++ webui/index.html | 3 +- webui/static/helper.js | 1 + webui/static/library.js | 191 +++++++++++++++++++-- webui/static/style.css | 49 ++++++ 8 files changed, 641 insertions(+), 16 deletions(-) create mode 100644 tests/test_artist_top_tracks_clients.py diff --git a/core/deezer_client.py b/core/deezer_client.py index 0da94c76..59330239 100644 --- a/core/deezer_client.py +++ b/core/deezer_client.py @@ -599,6 +599,74 @@ class DeezerClient: return result + def get_artist_top_tracks(self, artist_id: str, limit: int = 10) -> List[Dict[str, Any]]: + """Return the artist's top tracks in Spotify-compatible dict format. + + Wraps Deezer's `/artist/{id}/top?limit=N`. Returns dicts with the same + shape Spotify's `artist_top_tracks` produces — id, name, artists, album + (with album_type / total_tracks / release_date / images), duration_ms, + track_number, disc_number — so callers don't need to branch on source. + """ + if not artist_id: + return [] + try: + limit = max(1, min(int(limit or 10), 100)) + except (TypeError, ValueError): + limit = 10 + + data = self._api_get(f'artist/{artist_id}/top', {'limit': limit}) + if not data or 'data' not in data: + return [] + + tracks = [] + for track_data in data['data']: + if not isinstance(track_data, dict): + continue + artist_data = track_data.get('artist') or {} + album_data = track_data.get('album') or {} + + # Build images list from any cover sizes Deezer returned for the album + images = [] + if isinstance(album_data, dict): + for size_key, dim in [('cover_xl', 1000), ('cover_big', 500), + ('cover_medium', 250), ('cover_small', 56)]: + if album_data.get(size_key): + images.append({'url': album_data[size_key], 'height': dim, 'width': dim}) + + # Deezer `/artist/{id}/top` results don't include record_type on the + # nested album object; we don't have a track-count to infer from + # either. Default 'album' so the path-builder template variable + # always has something to substitute (existing behavior elsewhere). + album_payload = { + 'id': str(album_data.get('id', '')) if isinstance(album_data, dict) else '', + 'name': album_data.get('title', '') if isinstance(album_data, dict) else '', + 'album_type': 'album', + 'images': images, + 'release_date': '', + 'total_tracks': 0, + 'artists': [{'name': artist_data.get('name', '')}] if isinstance(artist_data, dict) else [], + } + + tracks.append({ + 'id': str(track_data.get('id', '')), + 'name': track_data.get('title', ''), + 'artists': [{ + 'id': str(artist_data.get('id', '')) if isinstance(artist_data, dict) else '', + 'name': artist_data.get('name', '') if isinstance(artist_data, dict) else '', + }], + 'album': album_payload, + 'duration_ms': (track_data.get('duration') or 0) * 1000, # Deezer is seconds + 'popularity': track_data.get('rank', 0), + 'preview_url': track_data.get('preview'), + 'external_urls': {'deezer': track_data['link']} if track_data.get('link') else {}, + 'track_number': track_data.get('track_position'), + 'disc_number': track_data.get('disk_number', 1), + 'explicit': bool(track_data.get('explicit_lyrics', False)), + '_source': 'deezer', + }) + + return tracks + def get_artist_info(self, artist_id: str) -> Optional[Dict[str, Any]]: """Get full artist details — returns Spotify-compatible dict (metadata source interface). diff --git a/core/spotify_client.py b/core/spotify_client.py index c0d8ca4f..09ede9c4 100644 --- a/core/spotify_client.py +++ b/core/spotify_client.py @@ -1753,6 +1753,31 @@ class SpotifyClient: logger.debug(f"Cannot use fallback for Spotify artist ID: {artist_id}") return None + @rate_limited + def get_artist_top_tracks(self, artist_id: str, country: str = 'US', limit: int = 10) -> List[Dict[str, Any]]: + """Return up to 10 top tracks for an artist (Spotify caps the response at 10). + + Spotify's `artist_top_tracks` endpoint always returns ~10 tracks for the given + market regardless of any limit param. The `limit` argument here is honored as + a UI-side trim only — it never reduces the number of API calls. Tracks come + back in Spotify's standard track-object shape (id, name, artists, album, ...) + so the rest of the pipeline can consume them unchanged. + """ + if not artist_id: + return [] + if not self.is_spotify_authenticated(): + return [] + try: + result = self.sp.artist_top_tracks(artist_id, country=country) + except Exception as e: + _detect_and_set_rate_limit(e, 'get_artist_top_tracks') + logger.warning(f"Spotify artist_top_tracks failed for {artist_id}: {e}") + return [] + if not result: + return [] + tracks = result.get('tracks', []) or [] + return tracks[:max(1, int(limit or 10))] + @rate_limited def get_artists_batch(self, artist_ids: List[str]) -> Dict[str, Dict]: """Get multiple artists, using cache where possible, batch API for misses. diff --git a/tests/test_artist_top_tracks_clients.py b/tests/test_artist_top_tracks_clients.py new file mode 100644 index 00000000..fcb2e33f --- /dev/null +++ b/tests/test_artist_top_tracks_clients.py @@ -0,0 +1,218 @@ +"""Tests for the new artist top-tracks client methods. + +Issue #513: surface an artist's "top X popular songs" for one-click download +without pulling the entire discography. Spotify and Deezer expose this via +native APIs; iTunes / Discogs / MusicBrainz don't, so the frontend falls +back to the existing Last.fm display-only sidebar. + +Scope: client methods only. The Flask endpoint that wraps them is small +enough (source dispatch + DB id resolution + JSON response) that +exercising the underlying client methods is the load-bearing test layer. +A full-app Flask test client wasn't worth pulling in here — importing +``web_server`` at test-collection time spins up worker threads that race +with caplog-using tests elsewhere in the suite. +""" + +from __future__ import annotations + +from unittest.mock import MagicMock + +from core.deezer_client import DeezerClient +from core.spotify_client import SpotifyClient + + +# --------------------------------------------------------------------------- +# Spotify client method +# --------------------------------------------------------------------------- + + +def test_spotify_get_artist_top_tracks_returns_track_list(monkeypatch): + """Wraps spotipy's `artist_top_tracks` and returns the `tracks` array.""" + client = SpotifyClient.__new__(SpotifyClient) + fake_sp = MagicMock() + fake_sp.artist_top_tracks.return_value = { + 'tracks': [ + {'id': 't1', 'name': 'Song A', 'artists': [{'name': 'Artist'}]}, + {'id': 't2', 'name': 'Song B', 'artists': [{'name': 'Artist'}]}, + {'id': 't3', 'name': 'Song C', 'artists': [{'name': 'Artist'}]}, + ] + } + client.sp = fake_sp + monkeypatch.setattr(client, 'is_spotify_authenticated', lambda: True) + + result = client.get_artist_top_tracks('artist_id', country='US', limit=10) + + assert len(result) == 3 + assert [t['id'] for t in result] == ['t1', 't2', 't3'] + fake_sp.artist_top_tracks.assert_called_once_with('artist_id', country='US') + + +def test_spotify_get_artist_top_tracks_honors_ui_limit(monkeypatch): + """Spotify always returns up to 10 tracks; the limit param is a UI trim only.""" + client = SpotifyClient.__new__(SpotifyClient) + fake_sp = MagicMock() + fake_sp.artist_top_tracks.return_value = { + 'tracks': [{'id': f't{i}', 'name': f'Song {i}'} for i in range(10)] + } + client.sp = fake_sp + monkeypatch.setattr(client, 'is_spotify_authenticated', lambda: True) + + result = client.get_artist_top_tracks('artist_id', limit=3) + assert len(result) == 3 + + +def test_spotify_get_artist_top_tracks_returns_empty_when_unauthed(monkeypatch): + """No API call should fire when Spotify isn't authenticated. Lets the + endpoint return `success=False, reason=spotify_not_authenticated` + instead of throwing.""" + client = SpotifyClient.__new__(SpotifyClient) + fake_sp = MagicMock() + client.sp = fake_sp + monkeypatch.setattr(client, 'is_spotify_authenticated', lambda: False) + + result = client.get_artist_top_tracks('artist_id') + assert result == [] + fake_sp.artist_top_tracks.assert_not_called() + + +def test_spotify_get_artist_top_tracks_returns_empty_when_artist_id_missing(monkeypatch): + """Defensive guard — no API call for empty/None artist id.""" + client = SpotifyClient.__new__(SpotifyClient) + fake_sp = MagicMock() + client.sp = fake_sp + monkeypatch.setattr(client, 'is_spotify_authenticated', lambda: True) + + assert client.get_artist_top_tracks('') == [] + assert client.get_artist_top_tracks(None) == [] + fake_sp.artist_top_tracks.assert_not_called() + + +def test_spotify_get_artist_top_tracks_swallows_api_errors(monkeypatch): + """Network/auth exceptions surface as empty list, not a crash — + the endpoint relies on this to fall through gracefully.""" + client = SpotifyClient.__new__(SpotifyClient) + fake_sp = MagicMock() + fake_sp.artist_top_tracks.side_effect = RuntimeError("boom") + client.sp = fake_sp + monkeypatch.setattr(client, 'is_spotify_authenticated', lambda: True) + + result = client.get_artist_top_tracks('artist_id') + assert result == [] + + +# --------------------------------------------------------------------------- +# Deezer client method +# --------------------------------------------------------------------------- + + +def test_deezer_get_artist_top_tracks_returns_spotify_compatible_shape(monkeypatch): + """Deezer's raw shape gets converted to the same dict layout the + Spotify endpoint produces (id, name, artists, album, duration_ms, + track_number, etc) so downstream code doesn't need to branch.""" + client = DeezerClient() + + raw_response = { + 'data': [ + { + 'id': 1001, + 'title': 'Some Hit', + 'duration': 200, # seconds + 'rank': 850000, + 'preview': 'https://example/preview.mp3', + 'link': 'https://deezer.com/track/1001', + 'track_position': 3, + 'disk_number': 1, + 'explicit_lyrics': False, + 'artist': {'id': 50, 'name': 'Test Artist'}, + 'album': { + 'id': 200, 'title': 'Greatest Hits', + 'cover_xl': 'https://example/cover_xl.jpg', + 'cover_big': 'https://example/cover_big.jpg', + }, + }, + ] + } + monkeypatch.setattr(client, '_api_get', lambda path, params=None: raw_response) + + tracks = client.get_artist_top_tracks('50', limit=5) + + assert len(tracks) == 1 + t = tracks[0] + assert t['id'] == '1001' + assert t['name'] == 'Some Hit' + assert t['duration_ms'] == 200_000 # converted to ms + assert t['track_number'] == 3 + assert t['disc_number'] == 1 + assert t['artists'] == [{'id': '50', 'name': 'Test Artist'}] + assert t['album']['id'] == '200' + assert t['album']['name'] == 'Greatest Hits' + assert t['album']['album_type'] == 'album' + assert any(img['url'] == 'https://example/cover_xl.jpg' for img in t['album']['images']) + assert t['_source'] == 'deezer' + + +def test_deezer_get_artist_top_tracks_empty_when_no_data(monkeypatch): + """Missing artist or empty response → empty list. Endpoint relies on + this to report `success=False, reason=no_tracks_found`.""" + client = DeezerClient() + monkeypatch.setattr(client, '_api_get', lambda path, params=None: None) + + assert client.get_artist_top_tracks('50') == [] + + +def test_deezer_get_artist_top_tracks_empty_when_artist_id_missing(monkeypatch): + """Defensive guard — no API call for empty artist id.""" + client = DeezerClient() + called = {'count': 0} + + def fake_api(*args, **kwargs): + called['count'] += 1 + return {'data': []} + + monkeypatch.setattr(client, '_api_get', fake_api) + + assert client.get_artist_top_tracks('') == [] + assert called['count'] == 0 + + +def test_deezer_get_artist_top_tracks_clamps_limit(monkeypatch): + """Limit param gets clamped at the upper bound (Deezer's max ~100) + and falls back to the default when the caller passes 0/None.""" + client = DeezerClient() + captured = {} + + def fake_api(path, params=None): + captured['params'] = params + return {'data': []} + + monkeypatch.setattr(client, '_api_get', fake_api) + + # Excessive limit clamped down to 100 + client.get_artist_top_tracks('50', limit=10000) + assert captured['params']['limit'] == 100 + + # 0 → falsy, falls back to default 10 (better than 1 — caller probably + # wanted "give me a sensible top-N", not "give me a single track") + client.get_artist_top_tracks('50', limit=0) + assert captured['params']['limit'] == 10 + + # Small valid limit passes through + client.get_artist_top_tracks('50', limit=3) + assert captured['params']['limit'] == 3 + + +def test_deezer_get_artist_top_tracks_skips_malformed_entries(monkeypatch): + """Defensive — non-dict entries in the response array get filtered out + rather than crashing the loop.""" + client = DeezerClient() + monkeypatch.setattr(client, '_api_get', lambda path, params=None: { + 'data': [ + None, # malformed: skipped + {'id': 1, 'title': 'Real', 'artist': {'name': 'A'}, 'album': {}}, + 'not a dict', # malformed: skipped + ] + }) + + tracks = client.get_artist_top_tracks('50') + assert len(tracks) == 1 + assert tracks[0]['name'] == 'Real' diff --git a/web_server.py b/web_server.py index 9599fef7..c0ee826d 100644 --- a/web_server.py +++ b/web_server.py @@ -8610,6 +8610,108 @@ def get_artist_image(artist_id): logger.error(f"Error fetching artist image: {e}") return jsonify({"success": False, "image_url": None, "error": str(e)}) +@app.route('/api/artist//top-tracks', methods=['GET']) +def get_artist_top_tracks_endpoint(artist_id): + """Return an artist's top-N tracks via the primary metadata source. + + Issue #513: users want a "top X popular songs" path that doesn't pull + the entire discography. Spotify's `artist_top_tracks` endpoint and + Deezer's `/artist/{id}/top` both expose this; iTunes / Discogs / + MusicBrainz don't have popularity ranking, so this endpoint returns + `success=False` for those primary sources and the frontend falls back + to the existing Last.fm display-only sidebar. + + Resolves per-source artist IDs from the DB row (matching what + /discography already does) so a Spotify ID in the URL still works + when Deezer is primary, and vice versa. + """ + try: + primary_source = _get_metadata_fallback_source() + if primary_source not in ('spotify', 'deezer'): + return jsonify({ + 'success': False, + 'reason': 'unsupported_source', + 'source': primary_source, + 'tracks': [], + }) + + try: + limit = max(1, min(int(request.args.get('limit', 10)), 50)) + except (TypeError, ValueError): + limit = 10 + + # Per-source ID resolution from the DB — same pattern as + # /discography. Without this, the frontend's chosen ID type + # (Spotify, Deezer, iTunes, library DB id) decides which source + # can answer; we want the URL ID to be neutral. + resolved_id = artist_id + try: + _db = get_database() + _conn = _db._get_connection() + try: + _cur = _conn.cursor() + _cur.execute(""" + SELECT spotify_artist_id, deezer_id + FROM artists + WHERE id = ? + OR spotify_artist_id = ? + OR itunes_artist_id = ? + OR deezer_id = ? + OR musicbrainz_id = ? + LIMIT 1 + """, (artist_id, artist_id, artist_id, artist_id, artist_id)) + _row = _cur.fetchone() + if _row: + if primary_source == 'spotify' and _row['spotify_artist_id']: + resolved_id = str(_row['spotify_artist_id']) + elif primary_source == 'deezer' and _row['deezer_id']: + resolved_id = str(_row['deezer_id']) + finally: + _conn.close() + except Exception as e: + logger.debug("top-tracks per-source ID resolution failed: %s", e) + + tracks = [] + if primary_source == 'spotify': + if not spotify_client or not spotify_client.is_spotify_authenticated(): + return jsonify({ + 'success': False, + 'reason': 'spotify_not_authenticated', + 'source': 'spotify', + 'tracks': [], + }) + market = config_manager.get('spotify.market', 'US') or 'US' + tracks = spotify_client.get_artist_top_tracks(resolved_id, country=market, limit=limit) + else: # deezer + deezer_client = _get_deezer_client() + if not deezer_client: + return jsonify({ + 'success': False, + 'reason': 'deezer_unavailable', + 'source': 'deezer', + 'tracks': [], + }) + tracks = deezer_client.get_artist_top_tracks(resolved_id, limit=limit) + + if not tracks: + return jsonify({ + 'success': False, + 'reason': 'no_tracks_found', + 'source': primary_source, + 'tracks': [], + }) + + return jsonify({ + 'success': True, + 'source': primary_source, + 'resolved_artist_id': resolved_id, + 'tracks': tracks, + }) + except Exception as e: + logger.exception("Error fetching artist top tracks for %s", artist_id) + return jsonify({"success": False, "error": str(e), "tracks": []}), 500 + + @app.route('/api/artist//discography', methods=['GET']) def get_artist_discography(artist_id): """Get an artist's complete discography (albums and singles)""" diff --git a/webui/index.html b/webui/index.html index 59622010..3de06f8e 100644 --- a/webui/index.html +++ b/webui/index.html @@ -2431,8 +2431,9 @@ diff --git a/webui/static/helper.js b/webui/static/helper.js index 2f9b396a..2188196a 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3432,6 +3432,7 @@ const WHATS_NEW = { '2.4.2': [ // --- post-2.4.1 dev work — entries hidden by _getLatestWhatsNewVersion until the build version bumps --- { date: 'Unreleased — 2.4.2 dev cycle' }, + { title: 'Artist Top Tracks: Per-Row + Bulk Download', desc: 'github issue #513 (s66jones): wanted a way to grab an artist\'s top X popular songs without pulling the full discography (the zotify workflow). artist detail page already had a "popular on last.fm" sidebar, but it was display-only — play button per row, no download. now when your primary metadata source is spotify or deezer, that sidebar pulls top tracks via the source\'s native popularity endpoint (spotify `artist_top_tracks` returns 10 per market, deezer `/artist/{id}/top` supports up to 100), each row gets a wishlist-add button on hover, and a "download all" footer button opens the existing wishlist modal with all top tracks pre-loaded. files land in their REAL album folders on disk (not a fake "top tracks" folder) because each track carries its actual album metadata. itunes / discogs / musicbrainz primary still falls back to the existing last.fm playcount display (no popularity ranking on those sources). 10 new tests pin the spotify + deezer client method behavior (auth gate, limit clamping, malformed response handling, spotify-compatible shape conversion).', page: 'library' }, { title: 'Fix: AcoustID Verification Let Instrumentals Pass As Vocal Tracks', desc: 'discord report (corruption [BWC]): downloads coming through as instrumental versions when the user expected the vocal cut. slipped past acoustid verification because the title-similarity normalizer strips parentheticals and version-suffix tags ("(Instrumental)", "- Live", etc) so legit name variations don\'t false-fail the comparison. side effect: "in my feelings" and "in my feelings (instrumental)" both normalize to "in my feelings", title sim is 1.0, file passes verification despite being the wrong cut. fix: detect the version label on each side BEFORE normalization runs — if expected and matched disagree (one is original, the other is instrumental / live / acoustic / remix / etc), reject as version mismatch. reuses `MusicMatchingEngine.detect_version_type` so post-download verification uses the same patterns the pre-download soulseek matcher already applies (no duplicated regex tables). also gates the secondary fallback scan, so a wrong-version variant in the same fingerprint cluster can\'t win the loop after the best match is rejected. 6 new tests pin the four direction cases (instrumental returned for vocal request → fail, vocal returned for instrumental request → fail, live vs acoustic → fail, matching versions on both sides → pass) plus the original-to-original happy path and the secondary-scan gate.', page: 'downloads' }, { title: 'Fix: Search Picker Defaulted to Spotify on Non-Admin Profiles', desc: 'github issue #515 (jaruca): admin sets primary metadata source to deezer / itunes / discogs, but every non-admin profile saw spotify as the active source on the search page and global search popover, requiring manual click each time. cause: `shared-helpers.js` resolved the active source by fetching `/api/settings` — that endpoint is `@admin_only` because it returns full config including credentials, so non-admin profiles got 403 and silently fell back to the hardcoded `spotify` default. fix: read from `/status` instead, which is public and already returns `metadata_source` for the dashboard. one-line scope change, behavior preserved for admins (same value, different endpoint), non-admins now see the real configured source.', page: 'search' }, { title: 'Internal: Stop Swallowing Exceptions Silently', desc: 'github issue #369 (johnbaumb): the codebase had ~300 `except Exception: pass` blocks — and another ~30 bare `except: pass` ones — across web_server.py, every metadata client, every download/import worker, the repair jobs, and most service modules. when one of those paths failed at runtime, the failure was completely invisible: no log line, no telemetry, nothing. you\'d see "downloads stopped working after a few hours" or "enrichment never finishes" and there was nothing to grep for in app.log because the exception had been thrown straight into the void. swept all of them. converted to `except Exception as e: logger.debug(": %s", e)` so failures land in the log with enough context to grep. bare `except:` cases (which also swallow KeyboardInterrupt and SystemExit — actively wrong) got upgraded to `except Exception:` first so ctrl-c works correctly. ~14 cleanup-path sites (atexit handlers, finally-block conn.close calls) were intentionally left silent with explicit `# noqa: S110` comments — logging during shutdown can itself crash because file handles get torn down before the handler fires. and added ruff S110 to the lint config so this pattern fails CI going forward — drift fails at PR review instead of at runtime against a wedged worker thread. zero behavior change to any happy path; just made the failure paths inspectable. test suite (2188 tests) green throughout the sweep.' }, diff --git a/webui/static/library.js b/webui/static/library.js index 0e6bfa56..783c16ad 100644 --- a/webui/static/library.js +++ b/webui/static/library.js @@ -1441,32 +1441,126 @@ function updateArtistHeroSection(artist, discography) { } // Lazy-load top tracks sidebar - if (artist.lastfm_url || artist.lastfm_listeners) { - _loadArtistTopTracks(artist.name); - } + // Always try metadata-source top tracks (Spotify / Deezer); fall back to + // Last.fm playcount when the source can't deliver. Last.fm-only mode is + // display-only (no download action), matching the legacy behavior. + _loadArtistTopTracks(artist.name); } +// Source label shown in the sidebar title. +const _TOP_TRACKS_SOURCE_LABELS = { + spotify: 'Top Tracks (Spotify)', + deezer: 'Top Tracks (Deezer)', + lastfm: 'Popular on Last.fm', +}; + async function _loadArtistTopTracks(artistName) { const sidebar = document.getElementById('artist-hero-sidebar'); const container = document.getElementById('hero-top-tracks'); + const titleEl = document.getElementById('hero-sidebar-title'); + const downloadAllBtn = document.getElementById('hero-top-tracks-download-all'); if (!sidebar || !container) return; + sidebar.style.display = 'none'; + if (downloadAllBtn) downloadAllBtn.style.display = 'none'; + + const _fmtNum = (n) => { + if (!n || n <= 0) return '0'; + if (n >= 1000000) return (n / 1000000).toFixed(1).replace(/\.0$/, '') + 'M'; + if (n >= 1000) return (n / 1000).toFixed(1).replace(/\.0$/, '') + 'K'; + return n.toLocaleString(); + }; + + const _escAttr = (s) => (s || '').replace(/&/g, '&').replace(/"/g, '"').replace(//g, '>'); + + // ── Pass 1: metadata-source top tracks (Spotify / Deezer) ── + // Returns full track objects (id, artists, album, etc) so each row gets + // a real download action via the existing wishlist-add flow. The + // backend gracefully reports `success=False` for sources that don't + // expose popularity ranking (iTunes / Discogs / MusicBrainz), so the + // sidebar can fall through to the Last.fm display-only mode below. + const artistId = artistDetailPageState.currentArtistId; + if (artistId) { + try { + const params = new URLSearchParams({ limit: '10' }); + const resp = await fetch(`/api/artist/${encodeURIComponent(artistId)}/top-tracks?${params}`); + if (resp.ok) { + const data = await resp.json(); + if (data && data.success && Array.isArray(data.tracks) && data.tracks.length > 0) { + if (titleEl) titleEl.textContent = _TOP_TRACKS_SOURCE_LABELS[data.source] || 'Top Tracks'; + + // Stash the resolved tracks on the container so the + // bulk-download button below can hand them to the + // existing wishlist modal without refetching. + container._topTracksPayload = { + source: data.source, + tracks: data.tracks, + artistName, + artistId, + }; + + container.innerHTML = data.tracks.map((t, i) => { + const trackName = t.name || ''; + const trackArtists = (t.artists && t.artists.length) + ? t.artists.map(a => (a && a.name) ? a.name : '').filter(Boolean).join(', ') + : artistName; + return ` +
+ ${i + 1} + + ${_escAttr(trackName)} + +
+ `; + }).join(''); + + container.onclick = (e) => { + const playBtn = e.target.closest('.hero-top-track-play'); + if (playBtn) { + e.stopPropagation(); + playStatsTrack(playBtn.dataset.track, playBtn.dataset.artist, ''); + return; + } + const dlBtn = e.target.closest('.hero-top-track-download'); + if (dlBtn) { + e.stopPropagation(); + const idx = parseInt(dlBtn.dataset.index, 10); + const payload = container._topTracksPayload; + if (payload && Number.isFinite(idx) && payload.tracks[idx]) { + _topTrackDownloadOne(payload.tracks[idx], payload.artistName); + } + } + }; + + // Wire the bulk "Download All" footer button + if (downloadAllBtn) { + downloadAllBtn.style.display = ''; + downloadAllBtn.onclick = (e) => { + e.stopPropagation(); + const payload = container._topTracksPayload; + if (payload) _topTrackDownloadAll(payload); + }; + } + + sidebar.style.display = ''; + return; + } + } + } catch (e) { + console.debug('Top tracks metadata-source fetch failed:', e); + } + } + + // ── Pass 2 (fallback): Last.fm playcount, display-only ── try { const resp = await fetch(`/api/artist/0/lastfm-top-tracks?name=${encodeURIComponent(artistName)}`); const data = await resp.json(); if (!data.success || !data.tracks || data.tracks.length === 0) { - sidebar.style.display = 'none'; return; } - const _fmtNum = (n) => { - if (!n || n <= 0) return '0'; - if (n >= 1000000) return (n / 1000000).toFixed(1).replace(/\.0$/, '') + 'M'; - if (n >= 1000) return (n / 1000).toFixed(1).replace(/\.0$/, '') + 'K'; - return n.toLocaleString(); - }; - - const _escAttr = (s) => (s || '').replace(/&/g, '&').replace(/"/g, '"').replace(//g, '>'); + if (titleEl) titleEl.textContent = _TOP_TRACKS_SOURCE_LABELS.lastfm; + container._topTracksPayload = null; container.innerHTML = data.tracks.map((t, i) => `
${i + 1} @@ -1476,7 +1570,6 @@ async function _loadArtistTopTracks(artistName) {
`).join(''); - // Attach play handlers via delegation (avoids inline JS escaping issues) container.onclick = (e) => { const btn = e.target.closest('.hero-top-track-play'); if (btn) { @@ -1486,8 +1579,76 @@ async function _loadArtistTopTracks(artistName) { }; sidebar.style.display = ''; } catch (e) { - console.debug('Failed to load top tracks:', e); - sidebar.style.display = 'none'; + console.debug('Failed to load top tracks (Last.fm fallback):', e); + } +} + +// Per-row download — wishlist a single track using its full metadata. +async function _topTrackDownloadOne(track, artistName) { + try { + const trackArtists = (track.artists && track.artists.length) + ? track.artists + : [{ name: artistName }]; + const album = (track.album && typeof track.album === 'object') ? track.album : {}; + const resp = await fetch('/api/add-album-to-wishlist', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ + track: { ...track, artists: trackArtists }, + artist: { id: artistDetailPageState.currentArtistId || '', name: artistName }, + album: album, + source_type: 'top_tracks', + source_context: { + artist_name: artistName, + album_name: album.name || '', + album_type: album.album_type || 'album', + }, + }), + }); + const data = await resp.json(); + if (data && data.success) { + showToast(`Added "${track.name}" to wishlist`, 'success'); + } else { + showToast(`Failed to wishlist "${track.name}": ${(data && data.error) || 'unknown'}`, 'error'); + } + } catch (e) { + console.error('top track wishlist add failed:', e); + showToast('Failed to add track to wishlist', 'error'); + } +} + +// Bulk download — open the standard download modal in PLAYLIST context, +// not album context. The virtualPlaylistId intentionally doesn't start +// with `artist_album_` / `enhanced_search_album_` / etc, so +// `startMissingTracksProcess` (downloads.js) sets is_album_download=false +// and the master worker skips injecting the wrapper as `_explicit_album_context`. +// Result: each track downloads using its own real album metadata, files +// land in the proper per-album folders on disk. +function _topTrackDownloadAll({ source, tracks, artistName, artistId }) { + const virtualPlaylistId = `top_tracks_${source}_${artistId || 'unknown'}`; + const playlistName = `${artistName} — Top Tracks`; + const wrapperAlbum = { + id: virtualPlaylistId, + name: playlistName, + album_type: 'compilation', + images: [], + total_tracks: tracks.length, + artists: [{ id: artistId || '', name: artistName }], + }; + const artistObj = { + id: artistId || '', + name: artistName, + source: source, + }; + if (typeof openDownloadMissingModalForArtistAlbum === 'function') { + // contextType='playlist' tells the modal to render the playlist + // hero (not the album hero); the playlist_id prefix above is what + // actually drives the per-track album-folder routing on download. + openDownloadMissingModalForArtistAlbum( + virtualPlaylistId, playlistName, tracks, wrapperAlbum, artistObj, true, 'playlist' + ); + } else { + showToast('Download modal not available', 'error'); } } diff --git a/webui/static/style.css b/webui/static/style.css index 31603610..b49adeab 100644 --- a/webui/static/style.css +++ b/webui/static/style.css @@ -24342,6 +24342,55 @@ div.artist-hero-badge { font-variant-numeric: tabular-nums; } +/* Per-row wishlist button — only shown on metadata-source rows + (Spotify / Deezer). Last.fm rows show the playcount instead. */ +.hero-top-track-download { + width: 22px; + height: 22px; + border-radius: 50%; + border: none; + background: rgba(255, 255, 255, 0.06); + color: rgba(255, 255, 255, 0.5); + font-size: 11px; + cursor: pointer; + display: flex; + align-items: center; + justify-content: center; + flex-shrink: 0; + opacity: 0; + transition: all 0.2s; +} +.hero-top-track:hover .hero-top-track-download { + opacity: 1; +} +.hero-top-track-download:hover { + background: rgba(var(--accent-rgb), 0.25); + color: rgb(var(--accent-rgb)); + transform: scale(1.1); +} + +/* Footer "Download All" button on the top-tracks sidebar — only shown + when metadata-source provided downloadable tracks. */ +.hero-top-tracks-download-all { + margin-top: 12px; + width: 100%; + padding: 8px 12px; + border: 1px solid rgba(var(--accent-rgb), 0.3); + border-radius: 8px; + background: rgba(var(--accent-rgb), 0.08); + color: rgb(var(--accent-rgb)); + font-size: 0.78em; + font-weight: 600; + letter-spacing: 0.04em; + text-transform: uppercase; + cursor: pointer; + transition: all 0.2s; +} +.hero-top-tracks-download-all:hover { + background: rgba(var(--accent-rgb), 0.18); + border-color: rgba(var(--accent-rgb), 0.5); +} + /* Mobile responsiveness */ @media (max-width: 768px) { .artist-hero-section {