From 0701bcc21321dbf237a386dfdf896de69ad86960 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Fri, 8 May 2026 07:14:36 -0700 Subject: [PATCH 1/5] PersonalizedPlaylistsService: bake in ID-validity gate, lift selectors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit User-facing bug found in the discover-page audit: multiple sections (hidden gems, discovery shuffle, popular picks, decade browser, genre browser) had no `WHERE (spotify_track_id IS NOT NULL OR itunes_track_id IS NOT NULL ...)` gate. Tracks with no source IDs in the discovery pool got displayed, the user clicked download, the download silently failed because there was nothing to look up. Lift + gate `PersonalizedPlaylistsService` had 5 selection methods that all shared the same shape — connect to DB, run a SELECT against `discovery_pool` with different WHERE clauses, optionally apply diversity, return list of track dicts. ~366 lines of business logic, ~55% of which was repeated boilerplate. Three new private helpers consolidate everything: - `_select_discovery_tracks(*, source, extra_where, extra_params, order_by, fetch_limit, extra_columns)` — shared SELECT against `discovery_pool`. The mandatory ID gate is hard-coded into the WHERE clause: no opt-out flag, every method inherits it for free. Plus the source filter and the blacklist filter — same shape every selector needs. - `_apply_diversity_filter(tracks, *, max_per_album, max_per_artist, limit)` — per-album / per-artist cap loop, returns trimmed list. Lifted from the inline duplicates in decade / genre / popular_picks. - `_compute_adaptive_diversity_limits(tracks, *, relaxed=False)` — step-function tiers based on unique-artist count. `relaxed=True` gives the slightly looser limits the genre playlist used vs the decade playlist. Re-enable 4 library methods `get_recently_added`, `get_top_tracks`, `get_forgotten_favorites`, `get_familiar_favorites` were all stubs (`return []`) because they predated the schema columns they need. Schema now has them: `tracks.created_at`, `tracks.play_count`, `tracks.last_played`, and the source ID columns added in earlier work. New `_select_library_tracks(*, where_clause, params, order_by, limit)` helper mirrors the discovery selector but targets the `tracks` table joined against `albums` + `artists`. Mandatory ID gate lives in the helper too: every library method automatically rejects rows where spotify_track_id, itunes_track_id, deezer_id, musicbrainz_recording_id, AND audiodb_id are all NULL. Selection rules: - `get_recently_added` — ORDER BY created_at DESC - `get_top_tracks` — WHERE play_count > 0 ORDER BY play_count DESC - `get_forgotten_favorites` — WHERE play_count > 5 AND last_played < (now - 90 days) ORDER BY play_count DESC - `get_familiar_favorites` — WHERE play_count BETWEEN 3 AND 15 Tests `tests/test_personalized_playlists_id_gate.py` — 17 tests pinning: - `_select_discovery_tracks` filters NULL-id rows, honors source + blacklist + extra_where - `_apply_diversity_filter` caps per-album + per-artist + stops at limit - `_compute_adaptive_diversity_limits` returns the right tier for unique-artist count + relaxed flag - All 5 discovery methods (decade, popular_picks, hidden_gems, discovery_shuffle, genre is exercised via the helper) reject NULL-id rows - All 4 library methods reject NULL-id rows + honor their play-count rules Behavior preserved Same diversity tiers, same over-fetch multipliers (10x for decade / genre, 3x for popular_picks), same `popularity DESC, RANDOM()` ordering, same `popularity >= 60` / `< 40` thresholds, same blacklist filter. Public method signatures unchanged — `web_server.py` needs zero edits. Net file: 1089 → ~1170 LOC (helpers + docstrings), but actual business logic across the 9 methods went from ~418 lines down to ~195 (-53%). 2222/2222 full suite green (was 2205 + 17 new). Ruff clean. --- core/personalized_playlists.py | 840 ++++++++++--------- tests/test_personalized_playlists_id_gate.py | 423 ++++++++++ 2 files changed, 884 insertions(+), 379 deletions(-) create mode 100644 tests/test_personalized_playlists_id_gate.py diff --git a/core/personalized_playlists.py b/core/personalized_playlists.py index b4c5aed7..9227178d 100644 --- a/core/personalized_playlists.py +++ b/core/personalized_playlists.py @@ -105,8 +105,191 @@ class PersonalizedPlaylistsService: from core.metadata_service import get_primary_source return get_primary_source() + # Standard column set returned by every discovery_pool selector. + # Callers can request additional columns via the `extra_columns` parameter + # of `_select_discovery_tracks` (e.g. `release_date`, `artist_genres`). + _STANDARD_DISCOVERY_COLUMNS: Tuple[str, ...] = ( + 'spotify_track_id', + 'itunes_track_id', + 'track_name', + 'artist_name', + 'album_name', + 'album_cover_url', + 'duration_ms', + 'popularity', + 'track_data_json', + 'source', + ) + + def _select_discovery_tracks( + self, + *, + source: str, + extra_where: str = "", + extra_params: tuple = (), + order_by: str = "RANDOM()", + fetch_limit: int, + extra_columns: tuple = (), + ) -> List[Dict]: + """ + Shared selector for discovery_pool playlist methods. + + Builds and runs a SELECT against `discovery_pool` with a baked-in + ID-validity gate so callers cannot accidentally return rows with no + usable source IDs (which would fail downstream when the user clicks + download). + + The WHERE clause always includes: + source = ? + AND (spotify_track_id IS NOT NULL OR itunes_track_id IS NOT NULL) + AND LOWER(artist_name) NOT IN + (SELECT LOWER(artist_name) FROM discovery_artist_blacklist) + + The ID gate is mandatory and not opt-out by design — if a future + method needs to skip it, that's a design discussion, not a flag. + + Callers compose additional filters via `extra_where` (a SQL fragment + beginning with "AND ...") and `extra_params` (positional bindings for + any `?` placeholders inside `extra_where`). + + Diversity filtering is the caller's responsibility — apply + `_apply_diversity_filter` to the returned list if needed. + + Args: + source: discovery_pool.source value to filter on. + extra_where: optional SQL fragment appended to the WHERE clause. + Must start with "AND " if non-empty. + extra_params: positional bindings for `?` placeholders in + `extra_where`. + order_by: ORDER BY expression, used as-is (e.g. "RANDOM()", + "popularity DESC, RANDOM()"). + fetch_limit: LIMIT applied to the query. Callers that intend to + run a diversity filter should over-fetch (e.g. `limit * 3`). + extra_columns: additional columns to SELECT beyond + `_STANDARD_DISCOVERY_COLUMNS` (e.g. `('release_date',)`). + + Returns: + List of track dicts via `_build_track_dict`. Returns `[]` on any + error (logged at error level). + """ + try: + columns = self._STANDARD_DISCOVERY_COLUMNS + tuple(extra_columns) + select_cols = ",\n ".join(columns) + + query = f""" + SELECT + {select_cols} + FROM discovery_pool + WHERE source = ? + AND (spotify_track_id IS NOT NULL OR itunes_track_id IS NOT NULL) + AND LOWER(artist_name) NOT IN (SELECT LOWER(artist_name) FROM discovery_artist_blacklist) + {extra_where} + ORDER BY {order_by} + LIMIT ? + """ + + params = (source,) + tuple(extra_params) + (fetch_limit,) + + with self.database._get_connection() as conn: + cursor = conn.cursor() + cursor.execute(query, params) + rows = cursor.fetchall() + + return [self._build_track_dict(row, source) for row in rows] + + except Exception as e: + logger.error(f"Error in _select_discovery_tracks (source={source}): {e}") + return [] + + def _apply_diversity_filter( + self, + tracks: List[Dict], + *, + max_per_album: int, + max_per_artist: int, + limit: int, + ) -> List[Dict]: + """ + Apply per-album / per-artist diversity caps to a track list. + + Iterates `tracks` in order, accepting each only if its album count + is still under `max_per_album` AND its artist count is still under + `max_per_artist`. Stops once `limit` tracks have been accepted. + + Returns a new list, already trimmed to at most `limit` items. + """ + tracks_by_album: Dict[str, int] = {} + tracks_by_artist: Dict[str, int] = {} + diverse_tracks: List[Dict] = [] + + for track in tracks: + album = track['album_name'] + artist = track['artist_name'] + + album_count = tracks_by_album.get(album, 0) + artist_count = tracks_by_artist.get(artist, 0) + + if album_count < max_per_album and artist_count < max_per_artist: + diverse_tracks.append(track) + tracks_by_album[album] = album_count + 1 + tracks_by_artist[artist] = artist_count + 1 + + if len(diverse_tracks) >= limit: + break + + return diverse_tracks + + def _compute_adaptive_diversity_limits( + self, + tracks: List[Dict], + *, + relaxed: bool = False, + ) -> Tuple[int, int]: + """ + Pick (max_per_album, max_per_artist) caps based on artist variety. + + Mirrors the step-functions previously inlined in the decade and + genre playlist methods. With more unique artists we can be strict; + with fewer we relax to still hit the requested track count. + + When `relaxed=True` (used for genre playlists), the moderate and + low bands use looser caps and an extra "very limited" tier kicks + in below 5 unique artists. + + Args: + tracks: candidate track list to inspect. + relaxed: True to apply the looser genre-playlist limits. + + Returns: + Tuple of (max_per_album, max_per_artist). + """ + unique_artists = len(set(t['artist_name'] for t in tracks)) + + if relaxed: + # Genre playlist tiers + if unique_artists >= 20: + return 3, 5 + if unique_artists >= 10: + return 4, 10 + if unique_artists >= 5: + return 6, 15 + return 8, 25 + + # Decade-style strict tiers + if unique_artists >= 20: + return 3, 5 + if unique_artists >= 10: + return 4, 8 + return 5, 12 + def _build_track_dict(self, row, source: str) -> Dict: - """Build a standardized track dictionary from a database row.""" + """Build a standardized track dictionary from a database row. + + If the row carries the optional `artist_genres` column (selected via + `_select_discovery_tracks(extra_columns=('artist_genres',))`), the raw + JSON string is passed through under `_artist_genres_raw` so callers + can run Python-side genre matching without re-querying the row. + """ # Convert sqlite3.Row to dict if needed (Row objects don't support .get()) if hasattr(row, 'keys'): row = dict(row) @@ -118,7 +301,7 @@ class PersonalizedPlaylistsService: except: track_data = None - return { + result = { 'track_id': row.get('spotify_track_id') or row.get('itunes_track_id') or row.get('deezer_track_id'), 'spotify_track_id': row.get('spotify_track_id'), 'itunes_track_id': row.get('itunes_track_id'), @@ -130,9 +313,19 @@ class PersonalizedPlaylistsService: 'duration_ms': row.get('duration_ms', 0), 'popularity': row.get('popularity', 0), 'track_data_json': track_data, - 'source': source + 'source': source, } + # Pass through optional extra columns under underscore-prefixed keys + # so callers that requested them via `extra_columns` can use them + # without having to re-query. + if 'artist_genres' in row: + result['_artist_genres_raw'] = row.get('artist_genres') + if 'release_date' in row: + result['_release_date'] = row.get('release_date') + + return result + @staticmethod def get_parent_genre(spotify_genre: str) -> str: """ @@ -152,52 +345,133 @@ class PersonalizedPlaylistsService: # LIBRARY-BASED PLAYLISTS # ======================================== - def get_recently_added(self, limit: int = 50) -> List[Dict]: + def _select_library_tracks( + self, + *, + where_clause: str = "", + params: tuple = (), + order_by: str = "t.created_at DESC", + limit: int, + ) -> List[Dict]: """ - Get recently added tracks from library. + Shared selector for library-based playlist methods. - Returns tracks ordered by date_added DESC + Builds and runs a SELECT against `tracks` joined to `albums` and + `artists`, with a baked-in ID-validity gate so callers cannot + accidentally return rows with no usable source IDs (which would fail + downstream when the user clicks download or tries to match the track + against an external metadata source). - NOTE: This requires library tracks to have Spotify metadata which may not be available. - Returns empty list if schema incompatible. + The WHERE clause always includes: + (t.spotify_track_id IS NOT NULL + OR t.itunes_track_id IS NOT NULL + OR t.deezer_id IS NOT NULL + OR t.musicbrainz_recording_id IS NOT NULL + OR t.audiodb_id IS NOT NULL) + + The ID gate is mandatory and not opt-out by design — mirrors the + shared `_select_discovery_tracks` helper. + + Args: + where_clause: optional SQL fragment appended to the WHERE clause. + Must start with "AND " if non-empty. + params: positional bindings for `?` placeholders in `where_clause`. + order_by: ORDER BY expression, used as-is. + limit: LIMIT applied to the query. + + Returns: + List of track dicts in the standard `_build_track_dict` shape with + `source='library'`. Returns `[]` on any error (logged at error + level). """ try: - logger.warning("Recently Added requires Spotify-linked library tracks - returning empty") - return [] + query = f""" + SELECT + t.spotify_track_id AS spotify_track_id, + t.itunes_track_id AS itunes_track_id, + t.deezer_id AS deezer_track_id, + t.title AS track_name, + ar.name AS artist_name, + al.title AS album_name, + al.thumb_url AS album_cover_url, + t.duration AS duration_ms + FROM tracks t + LEFT JOIN albums al ON t.album_id = al.id + LEFT JOIN artists ar ON t.artist_id = ar.id + WHERE (t.spotify_track_id IS NOT NULL + OR t.itunes_track_id IS NOT NULL + OR t.deezer_id IS NOT NULL + OR t.musicbrainz_recording_id IS NOT NULL + OR t.audiodb_id IS NOT NULL) + {where_clause} + ORDER BY {order_by} + LIMIT ? + """ + + bound_params = tuple(params) + (limit,) + + with self.database._get_connection() as conn: + cursor = conn.cursor() + cursor.execute(query, bound_params) + rows = cursor.fetchall() + + results: List[Dict] = [] + for row in rows: + row_dict = dict(row) if hasattr(row, 'keys') else row + # Library tracks have no popularity score and no JSON blob; + # fill the standard shape with the documented defaults. + row_dict.setdefault('popularity', 0) + row_dict.setdefault('track_data_json', '{}') + # Guard against NULL artist/album joins so downstream string + # ops (diversity filter, dedupe) don't blow up. + if row_dict.get('artist_name') is None: + row_dict['artist_name'] = 'Unknown' + if row_dict.get('album_name') is None: + row_dict['album_name'] = 'Unknown' + if row_dict.get('track_name') is None: + row_dict['track_name'] = 'Unknown' + if row_dict.get('duration_ms') is None: + row_dict['duration_ms'] = 0 + results.append(self._build_track_dict(row_dict, 'library')) + + return results except Exception as e: - logger.error(f"Error getting recently added tracks: {e}") + logger.error(f"Error in _select_library_tracks: {e}") return [] + def get_recently_added(self, limit: int = 50) -> List[Dict]: + """Get recently added tracks from the local library, newest first.""" + tracks = self._select_library_tracks( + order_by="t.created_at DESC", + limit=limit, + ) + logger.info(f"Recently Added: selected {len(tracks)} library tracks") + return tracks + def get_top_tracks(self, limit: int = 50) -> List[Dict]: - """ - Get user's all-time top tracks based on play count. - - NOTE: This requires library tracks to have Spotify metadata which may not be available. - Returns empty list if schema incompatible. - """ - try: - logger.warning("Top Tracks requires Spotify-linked library tracks - returning empty") - return [] - - except Exception as e: - logger.error(f"Error getting top tracks: {e}") - return [] + """Get the user's all-time top library tracks by play count.""" + tracks = self._select_library_tracks( + where_clause="AND t.play_count > 0", + order_by="t.play_count DESC", + limit=limit, + ) + logger.info(f"Top Tracks: selected {len(tracks)} library tracks") + return tracks def get_forgotten_favorites(self, limit: int = 50) -> List[Dict]: - """ - Get tracks you loved but haven't played recently. - - NOTE: This requires library tracks to have Spotify metadata which may not be available. - Returns empty list if schema incompatible. - """ - try: - logger.warning("Forgotten Favorites requires Spotify-linked library tracks - returning empty") - return [] - - except Exception as e: - logger.error(f"Error getting forgotten favorites: {e}") - return [] + """Get tracks the user loved (>5 plays) but hasn't played in 90+ days.""" + tracks = self._select_library_tracks( + where_clause=( + "AND t.play_count > 5 " + "AND t.last_played IS NOT NULL " + "AND t.last_played < datetime('now', '-90 days')" + ), + order_by="t.play_count DESC", + limit=limit, + ) + logger.info(f"Forgotten Favorites: selected {len(tracks)} library tracks") + return tracks def get_decade_playlist(self, decade: int, limit: int = 100, source: str = None) -> List[Dict]: """ @@ -208,99 +482,46 @@ class PersonalizedPlaylistsService: limit: Maximum tracks to return source: Optional source filter ('spotify' or 'itunes'), auto-detects if not provided """ - try: - start_year = decade - end_year = decade + 9 + start_year = decade + end_year = decade + 9 + active_source = source or self._get_active_source() - # Determine active source if not specified - active_source = source or self._get_active_source() + # Over-fetch 10x for diversity filtering headroom. + all_tracks = self._select_discovery_tracks( + source=active_source, + extra_where=( + "AND release_date IS NOT NULL " + "AND CAST(SUBSTR(release_date, 1, 4) AS INTEGER) BETWEEN ? AND ?" + ), + extra_params=(start_year, end_year), + order_by="RANDOM()", + fetch_limit=limit * 10, + extra_columns=('release_date',), + ) - with self.database._get_connection() as conn: - cursor = conn.cursor() - - # Query discovery_pool - get 10x more for diversity filtering, filtered by source - cursor.execute(""" - SELECT - spotify_track_id, - itunes_track_id, - track_name, - artist_name, - album_name, - album_cover_url, - duration_ms, - popularity, - release_date, - track_data_json, - source - FROM discovery_pool - WHERE release_date IS NOT NULL - AND CAST(SUBSTR(release_date, 1, 4) AS INTEGER) BETWEEN ? AND ? - AND source = ? - AND LOWER(artist_name) NOT IN (SELECT LOWER(artist_name) FROM discovery_artist_blacklist) - ORDER BY RANDOM() - LIMIT ? - """, (start_year, end_year, active_source, limit * 10)) - - rows = cursor.fetchall() - all_tracks = [] - for row in rows: - all_tracks.append(self._build_track_dict(row, active_source)) - - if not all_tracks: - logger.warning(f"No tracks found for {decade}s") - return [] - - # Shuffle first for randomness - import random - random.shuffle(all_tracks) - - # Count unique artists to determine diversity level - unique_artists = len(set(track['artist_name'] for track in all_tracks)) - - # Adaptive diversity limits based on artist variety - if unique_artists >= 20: - # Good variety - apply diversity constraints - max_per_album = 3 - max_per_artist = 5 - elif unique_artists >= 10: - # Moderate variety - more lenient - max_per_album = 4 - max_per_artist = 8 - else: - # Low variety - very lenient to hit 50 tracks - max_per_album = 5 - max_per_artist = 12 - - logger.info(f"{decade}s has {unique_artists} unique artists - using limits: {max_per_album} per album, {max_per_artist} per artist") - - # Apply diversity constraints - tracks_by_album = {} - tracks_by_artist = {} - diverse_tracks = [] - - for track in all_tracks: - album = track['album_name'] - artist = track['artist_name'] - - # Count current tracks for this album/artist - album_count = tracks_by_album.get(album, 0) - artist_count = tracks_by_artist.get(artist, 0) - - if album_count < max_per_album and artist_count < max_per_artist: - diverse_tracks.append(track) - tracks_by_album[album] = album_count + 1 - tracks_by_artist[artist] = artist_count + 1 - - if len(diverse_tracks) >= limit: - break - - logger.info(f"Found {len(diverse_tracks)} tracks from {decade}s in discovery pool (adaptive diversity)") - return diverse_tracks[:limit] - - except Exception as e: - logger.error(f"Error getting decade playlist for {decade}s: {e}") + if not all_tracks: + logger.warning(f"No tracks found for {decade}s") return [] + random.shuffle(all_tracks) + + max_per_album, max_per_artist = self._compute_adaptive_diversity_limits(all_tracks) + unique_artists = len(set(t['artist_name'] for t in all_tracks)) + logger.info( + f"{decade}s has {unique_artists} unique artists - using limits: " + f"{max_per_album} per album, {max_per_artist} per artist" + ) + + diverse_tracks = self._apply_diversity_filter( + all_tracks, + max_per_album=max_per_album, + max_per_artist=max_per_artist, + limit=limit, + ) + + logger.info(f"Found {len(diverse_tracks)} tracks from {decade}s in discovery pool (adaptive diversity)") + return diverse_tracks + def get_available_genres(self, source: str = None) -> List[Dict]: """ Get list of consolidated parent genres with track counts from discovery pool. @@ -363,238 +584,128 @@ class PersonalizedPlaylistsService: logger.error(f"Error getting available genres: {e}") return [] + def _genre_matches(self, artist_genres_json: Optional[str], search_keywords: List[str]) -> bool: + """Return True if any artist genre in the JSON-encoded column matches any keyword.""" + if not artist_genres_json: + return False + try: + genres = json.loads(artist_genres_json) + except Exception: + return False + for artist_genre in genres: + artist_genre_lower = artist_genre.lower() + for keyword in search_keywords: + if keyword in artist_genre_lower: + return True + return False + def get_genre_playlist(self, genre: str, limit: int = 50, source: str = None) -> List[Dict]: """ Get tracks from a specific genre with diversity filtering. Uses cached artist genres from database (populated during discovery scan). Supports both parent genres (e.g., "Electronic/Dance") and specific genres (e.g., "house"). + + The genre keyword match runs Python-side over the JSON-encoded artist_genres + column, so this method overfetches via the shared selector then filters. """ - try: - # Determine active source if not specified - active_source = source or self._get_active_source() + active_source = source or self._get_active_source() - with self.database._get_connection() as conn: - cursor = conn.cursor() + # Build keyword list: parent genre expands to all child keywords; + # specific genre uses its own name for partial matching. + if genre in self.GENRE_MAPPING: + search_keywords = [k.lower() for k in self.GENRE_MAPPING[genre]] + logger.info(f"Matching parent genre '{genre}' with {len(search_keywords)} child keywords") + else: + search_keywords = [genre.lower()] + logger.info(f"Matching specific genre '{genre}' with partial matching") - # Get all tracks with genres from discovery pool, filtered by source - cursor.execute(""" - SELECT - spotify_track_id, - itunes_track_id, - track_name, - artist_name, - album_name, - album_cover_url, - duration_ms, - popularity, - artist_genres, - track_data_json, - source - FROM discovery_pool - WHERE artist_genres IS NOT NULL - AND source = ? - AND LOWER(artist_name) NOT IN (SELECT LOWER(artist_name) FROM discovery_artist_blacklist) - """, (active_source,)) - rows = cursor.fetchall() + # Pull every source row with non-null artist_genres through the shared + # selector (gets the ID gate + blacklist filter for free). Cap at a high + # bound — the Python keyword filter narrows the result drastically. + candidate_tracks = self._select_discovery_tracks( + source=active_source, + extra_where="AND artist_genres IS NOT NULL", + order_by="RANDOM()", + fetch_limit=1_000_000, + extra_columns=('artist_genres',), + ) - # Determine if this is a parent genre or specific genre - is_parent_genre = genre in self.GENRE_MAPPING - search_keywords = [] - - if is_parent_genre: - # Use all child genre keywords for matching - search_keywords = self.GENRE_MAPPING[genre] - logger.info(f"Matching parent genre '{genre}' with {len(search_keywords)} child keywords") - else: - # Use the genre name itself for partial matching - search_keywords = [genre.lower()] - logger.info(f"Matching specific genre '{genre}' with partial matching") - - # Filter tracks that match the genre - matching_tracks = [] - - for row in rows: - try: - artist_genres_json = row['artist_genres'] - if artist_genres_json: - genres = json.loads(artist_genres_json) - - # Check if any artist genre matches any search keyword - genre_match = False - for artist_genre in genres: - artist_genre_lower = artist_genre.lower() - for keyword in search_keywords: - if keyword in artist_genre_lower: - genre_match = True - break - if genre_match: - break - - if genre_match: - matching_tracks.append(self._build_track_dict(row, active_source)) - except Exception as e: - logger.debug(f"Error parsing genres for track: {e}") - continue - - if not matching_tracks: - logger.warning(f"No tracks found for genre: {genre}") - return [] - - # Shuffle before limiting for better variety - random.shuffle(matching_tracks) - - # Limit to 10x for diversity filtering - all_tracks = matching_tracks[:limit * 10] if len(matching_tracks) > limit * 10 else matching_tracks - - if not all_tracks: - return [] - - # Apply adaptive diversity filtering (relaxed for genres) - unique_artists = len(set(track['artist_name'] for track in all_tracks)) - - if unique_artists >= 20: - max_per_album = 3 - max_per_artist = 5 - elif unique_artists >= 10: - max_per_album = 4 - max_per_artist = 10 - elif unique_artists >= 5: - max_per_album = 6 - max_per_artist = 15 - else: - # Very limited artist pool - be more lenient - max_per_album = 8 - max_per_artist = 25 - - logger.info(f"Genre '{genre}' has {unique_artists} artists, {len(all_tracks)} total tracks - limits: {max_per_album}/album, {max_per_artist}/artist") - - # Shuffle and apply diversity - random.shuffle(all_tracks) - tracks_by_album = {} - tracks_by_artist = {} - diverse_tracks = [] - - for track in all_tracks: - album = track['album_name'] - artist = track['artist_name'] - - album_count = tracks_by_album.get(album, 0) - artist_count = tracks_by_artist.get(artist, 0) - - if album_count < max_per_album and artist_count < max_per_artist: - diverse_tracks.append(track) - tracks_by_album[album] = album_count + 1 - tracks_by_artist[artist] = artist_count + 1 - - if len(diverse_tracks) >= limit: - break - - logger.info(f"Found {len(diverse_tracks)} tracks for genre '{genre}'") - return diverse_tracks[:limit] - - except Exception as e: - logger.error(f"Error getting genre playlist for {genre}: {e}") + if not candidate_tracks: + logger.warning(f"No tracks with genre data found for source: {active_source}") return [] + # `_build_track_dict` stashes the raw `artist_genres` column under + # `_artist_genres_raw` (since we requested it via `extra_columns`), + # so the keyword match can run without re-querying. + matching_tracks = [ + track for track in candidate_tracks + if self._genre_matches(track.get('_artist_genres_raw'), search_keywords) + ] + + if not matching_tracks: + logger.warning(f"No tracks found for genre: {genre}") + return [] + + random.shuffle(matching_tracks) + + # Cap candidate set at 10x limit for diversity filtering headroom. + all_tracks = matching_tracks[:limit * 10] if len(matching_tracks) > limit * 10 else matching_tracks + + max_per_album, max_per_artist = self._compute_adaptive_diversity_limits(all_tracks, relaxed=True) + unique_artists = len(set(t['artist_name'] for t in all_tracks)) + logger.info( + f"Genre '{genre}' has {unique_artists} artists, {len(all_tracks)} total tracks - " + f"limits: {max_per_album}/album, {max_per_artist}/artist" + ) + + random.shuffle(all_tracks) + diverse_tracks = self._apply_diversity_filter( + all_tracks, + max_per_album=max_per_album, + max_per_artist=max_per_artist, + limit=limit, + ) + + logger.info(f"Found {len(diverse_tracks)} tracks for genre '{genre}'") + return diverse_tracks + # ======================================== # DISCOVERY POOL PLAYLISTS # ======================================== def get_popular_picks(self, limit: int = 50) -> List[Dict]: - """Get high popularity tracks from discovery pool with diversity (max 2 tracks per album/artist)""" - # Determine active source + """Get high popularity tracks from discovery pool with diversity (max 2 per album, 3 per artist).""" active_source = self._get_active_source() - try: - with self.database._get_connection() as conn: - cursor = conn.cursor() + # Over-fetch 3x so the diversity filter has room to spread albums/artists. + all_tracks = self._select_discovery_tracks( + source=active_source, + extra_where="AND popularity >= 60", + order_by="popularity DESC, RANDOM()", + fetch_limit=limit * 3, + ) - # Get more tracks than needed to allow for filtering, filtered by source - cursor.execute(""" - SELECT - spotify_track_id, - itunes_track_id, - track_name, - artist_name, - album_name, - album_cover_url, - duration_ms, - popularity, - track_data_json, - source - FROM discovery_pool - WHERE popularity >= 60 AND source = ? - AND LOWER(artist_name) NOT IN (SELECT LOWER(artist_name) FROM discovery_artist_blacklist) - ORDER BY popularity DESC, RANDOM() - LIMIT ? - """, (active_source, limit * 3)) + diverse_tracks = self._apply_diversity_filter( + all_tracks, + max_per_album=2, + max_per_artist=3, + limit=limit, + ) - rows = cursor.fetchall() - all_tracks = [self._build_track_dict(row, active_source) for row in rows] - - # Apply diversity constraint: max 2 tracks per album, max 3 per artist - tracks_by_album = {} - tracks_by_artist = {} - diverse_tracks = [] - - for track in all_tracks: - album = track['album_name'] - artist = track['artist_name'] - - # Count current tracks for this album/artist - album_count = tracks_by_album.get(album, 0) - artist_count = tracks_by_artist.get(artist, 0) - - # Apply limits: max 2 per album, max 3 per artist - if album_count < 2 and artist_count < 3: - diverse_tracks.append(track) - tracks_by_album[album] = album_count + 1 - tracks_by_artist[artist] = artist_count + 1 - - if len(diverse_tracks) >= limit: - break - - logger.info(f"Popular Picks ({active_source}): Selected {len(diverse_tracks)} tracks with diversity") - return diverse_tracks[:limit] - - except Exception as e: - logger.error(f"Error getting popular picks: {e}") - return [] + logger.info(f"Popular Picks ({active_source}): selected {len(diverse_tracks)} tracks with diversity") + return diverse_tracks def get_hidden_gems(self, limit: int = 50) -> List[Dict]: - """Get low popularity (underground/indie) tracks from discovery pool""" - # Determine active source + """Get low-popularity (underground/indie) tracks from discovery pool.""" active_source = self._get_active_source() - - try: - with self.database._get_connection() as conn: - cursor = conn.cursor() - - cursor.execute(""" - SELECT - spotify_track_id, - itunes_track_id, - track_name, - artist_name, - album_name, - album_cover_url, - duration_ms, - popularity, - track_data_json, - source - FROM discovery_pool - WHERE popularity < 40 AND source = ? - AND LOWER(artist_name) NOT IN (SELECT LOWER(artist_name) FROM discovery_artist_blacklist) - ORDER BY RANDOM() - LIMIT ? - """, (active_source, limit)) - - rows = cursor.fetchall() - return [self._build_track_dict(row, active_source) for row in rows] - - except Exception as e: - logger.error(f"Error getting hidden gems: {e}") - return [] + tracks = self._select_discovery_tracks( + source=active_source, + extra_where="AND popularity < 40", + order_by="RANDOM()", + fetch_limit=limit, + ) + logger.info(f"Hidden Gems ({active_source}): selected {len(tracks)} tracks") + return tracks def get_discovery_shuffle(self, limit: int = 50) -> List[Dict]: """ @@ -602,53 +713,24 @@ class PersonalizedPlaylistsService: Different every time you call it! """ - # Determine active source active_source = self._get_active_source() - - try: - with self.database._get_connection() as conn: - cursor = conn.cursor() - - cursor.execute(""" - SELECT - spotify_track_id, - itunes_track_id, - track_name, - artist_name, - album_name, - album_cover_url, - duration_ms, - popularity, - track_data_json, - source - FROM discovery_pool - WHERE source = ? - AND LOWER(artist_name) NOT IN (SELECT LOWER(artist_name) FROM discovery_artist_blacklist) - ORDER BY RANDOM() - LIMIT ? - """, (active_source, limit)) - - rows = cursor.fetchall() - return [self._build_track_dict(row, active_source) for row in rows] - - except Exception as e: - logger.error(f"Error getting discovery shuffle: {e}") - return [] + tracks = self._select_discovery_tracks( + source=active_source, + order_by="RANDOM()", + fetch_limit=limit, + ) + logger.info(f"Discovery Shuffle ({active_source}): selected {len(tracks)} tracks") + return tracks def get_familiar_favorites(self, limit: int = 50) -> List[Dict]: - """ - Get tracks with medium play counts (3-15 plays) - your reliable go-tos. - - NOTE: This requires library tracks to have Spotify metadata which may not be available. - Returns empty list if schema incompatible. - """ - try: - logger.warning("Familiar Favorites requires Spotify-linked library tracks - returning empty") - return [] - - except Exception as e: - logger.error(f"Error getting familiar favorites: {e}") - return [] + """Get tracks with medium play counts (3-15 plays) - reliable go-tos.""" + tracks = self._select_library_tracks( + where_clause="AND t.play_count BETWEEN 3 AND 15", + order_by="t.play_count DESC", + limit=limit, + ) + logger.info(f"Familiar Favorites: selected {len(tracks)} library tracks") + return tracks # ======================================== # DAILY MIX (HYBRID PLAYLISTS) diff --git a/tests/test_personalized_playlists_id_gate.py b/tests/test_personalized_playlists_id_gate.py new file mode 100644 index 00000000..ccc315a9 --- /dev/null +++ b/tests/test_personalized_playlists_id_gate.py @@ -0,0 +1,423 @@ +"""Tests for the lifted PersonalizedPlaylistsService selectors and the +mandatory ID-validity gate every section must enforce. + +Context: discover-page selection methods used to return tracks/albums +with all source IDs NULL — the UI displayed them, the user clicked +download, the download silently failed because there was nothing to +look up. This test file pins the gate at the helper level so a future +section can't accidentally bypass it. + +Coverage: +- `_select_discovery_tracks` filters out rows where every source ID is NULL +- `_select_discovery_tracks` honors source filter + blacklist filter +- `_select_library_tracks` enforces the same gate against the `tracks` table +- `_apply_diversity_filter` caps per-album + per-artist counts +- `_compute_adaptive_diversity_limits` returns the right tier for the + unique-artist count + relaxed flag +- The 5 discovery_pool methods (decade / genre / popular_picks / + hidden_gems / discovery_shuffle) each filter NULL-id rows +- The 4 library methods (recently_added / top_tracks / + forgotten_favorites / familiar_favorites) each filter NULL-id rows +""" + +from __future__ import annotations + +import sqlite3 +from contextlib import contextmanager +from unittest.mock import patch + +import pytest + +from core.personalized_playlists import PersonalizedPlaylistsService + + +# --------------------------------------------------------------------------- +# Fake DB +# --------------------------------------------------------------------------- + + +class _FakeDatabase: + """Wraps an in-memory sqlite connection so the service's + `database._get_connection()` calls work the same as in production. + + The schema mirrors the real `discovery_pool` + `tracks` + `albums` + + `artists` shape just enough for the selection methods to exercise. + """ + + def __init__(self): + self._conn = sqlite3.connect(":memory:") + self._conn.row_factory = sqlite3.Row + self._setup_schema() + + def _setup_schema(self): + cursor = self._conn.cursor() + cursor.executescript(""" + CREATE TABLE discovery_pool ( + id INTEGER PRIMARY KEY, + source TEXT NOT NULL, + spotify_track_id TEXT, + itunes_track_id TEXT, + deezer_track_id TEXT, + track_name TEXT, + artist_name TEXT, + album_name TEXT, + album_cover_url TEXT, + duration_ms INTEGER, + popularity INTEGER, + release_date TEXT, + artist_genres TEXT, + track_data_json TEXT + ); + CREATE TABLE discovery_artist_blacklist ( + artist_name TEXT PRIMARY KEY + ); + CREATE TABLE artists ( + id INTEGER PRIMARY KEY, + name TEXT + ); + CREATE TABLE albums ( + id INTEGER PRIMARY KEY, + title TEXT, + artist_id INTEGER, + thumb_url TEXT + ); + CREATE TABLE tracks ( + id INTEGER PRIMARY KEY, + album_id INTEGER, + artist_id INTEGER, + title TEXT, + duration INTEGER, + created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, + play_count INTEGER DEFAULT 0, + last_played TIMESTAMP, + spotify_track_id TEXT, + itunes_track_id TEXT, + deezer_id TEXT, + musicbrainz_recording_id TEXT, + audiodb_id TEXT + ); + """) + self._conn.commit() + + @contextmanager + def _get_connection(self): + # Match the production interface (`with database._get_connection() as conn`) + try: + yield self._conn + finally: + pass + + def insert_discovery_track(self, **kwargs): + cols = ", ".join(kwargs.keys()) + placeholders = ", ".join(["?"] * len(kwargs)) + self._conn.execute( + f"INSERT INTO discovery_pool ({cols}) VALUES ({placeholders})", + tuple(kwargs.values()), + ) + self._conn.commit() + + def insert_library_track(self, *, artist_name='Library Artist', + album_name='Library Album', track_name='Library Track', + **track_kwargs): + # Insert artist + album + track in one shot for convenience. + cur = self._conn.cursor() + cur.execute("INSERT INTO artists (name) VALUES (?)", (artist_name,)) + artist_id = cur.lastrowid + cur.execute( + "INSERT INTO albums (title, artist_id, thumb_url) VALUES (?, ?, ?)", + (album_name, artist_id, 'http://x/cover.jpg'), + ) + album_id = cur.lastrowid + track_kwargs.setdefault('duration', 200000) + cur.execute( + f"""INSERT INTO tracks (album_id, artist_id, title, {', '.join(track_kwargs.keys())}) + VALUES (?, ?, ?, {', '.join(['?'] * len(track_kwargs))})""", + (album_id, artist_id, track_name, *track_kwargs.values()), + ) + self._conn.commit() + + def blacklist(self, artist_name): + self._conn.execute( + "INSERT INTO discovery_artist_blacklist (artist_name) VALUES (?)", + (artist_name,), + ) + self._conn.commit() + + +@pytest.fixture +def service(): + """Service with a fresh in-memory DB. `_get_active_source` patched + to return 'spotify' so every selector targets the same source.""" + db = _FakeDatabase() + svc = PersonalizedPlaylistsService(db) + with patch.object(svc, '_get_active_source', return_value='spotify'): + yield svc, db + + +# --------------------------------------------------------------------------- +# `_select_discovery_tracks` — the helper everyone goes through +# --------------------------------------------------------------------------- + + +def test_discovery_helper_filters_null_id_rows(service): + svc, db = service + db.insert_discovery_track( + source='spotify', spotify_track_id='sp1', track_name='Has Spotify ID', + artist_name='A', album_name='A', popularity=50, + ) + db.insert_discovery_track( + source='spotify', spotify_track_id=None, itunes_track_id=None, + track_name='No IDs', artist_name='B', album_name='B', popularity=50, + ) + db.insert_discovery_track( + source='spotify', itunes_track_id='it1', track_name='Has iTunes ID', + artist_name='C', album_name='C', popularity=50, + ) + + tracks = svc._select_discovery_tracks( + source='spotify', + order_by='track_name', + fetch_limit=100, + ) + names = sorted(t['track_name'] for t in tracks) + assert names == ['Has Spotify ID', 'Has iTunes ID'] + + +def test_discovery_helper_filters_blacklisted_artists(service): + svc, db = service + db.insert_discovery_track( + source='spotify', spotify_track_id='sp1', track_name='Keep', + artist_name='Good Artist', album_name='X', + ) + db.insert_discovery_track( + source='spotify', spotify_track_id='sp2', track_name='Drop', + artist_name='Bad Artist', album_name='X', + ) + db.blacklist('bad artist') + + tracks = svc._select_discovery_tracks( + source='spotify', + order_by='track_name', + fetch_limit=100, + ) + assert [t['track_name'] for t in tracks] == ['Keep'] + + +def test_discovery_helper_honors_source_filter(service): + svc, db = service + db.insert_discovery_track( + source='spotify', spotify_track_id='sp1', track_name='SP', + artist_name='A', album_name='X', + ) + db.insert_discovery_track( + source='itunes', itunes_track_id='it1', track_name='IT', + artist_name='A', album_name='X', + ) + + tracks = svc._select_discovery_tracks( + source='spotify', + order_by='track_name', + fetch_limit=100, + ) + assert [t['track_name'] for t in tracks] == ['SP'] + + +def test_discovery_helper_honors_extra_where(service): + svc, db = service + db.insert_discovery_track( + source='spotify', spotify_track_id='sp1', track_name='Pop60', + artist_name='A', album_name='X', popularity=60, + ) + db.insert_discovery_track( + source='spotify', spotify_track_id='sp2', track_name='Pop20', + artist_name='A', album_name='X', popularity=20, + ) + + tracks = svc._select_discovery_tracks( + source='spotify', + extra_where='AND popularity >= 50', + order_by='popularity DESC', + fetch_limit=100, + ) + assert [t['track_name'] for t in tracks] == ['Pop60'] + + +# --------------------------------------------------------------------------- +# Diversity filter +# --------------------------------------------------------------------------- + + +def test_diversity_filter_caps_per_album(): + svc = PersonalizedPlaylistsService(_FakeDatabase()) + tracks = [ + {'track_name': f't{i}', 'artist_name': 'A', 'album_name': 'Album1'} + for i in range(10) + ] + out = svc._apply_diversity_filter( + tracks, max_per_album=3, max_per_artist=10, limit=10, + ) + assert len(out) == 3 + + +def test_diversity_filter_caps_per_artist(): + svc = PersonalizedPlaylistsService(_FakeDatabase()) + tracks = [ + {'track_name': f't{i}', 'artist_name': 'OnlyArtist', 'album_name': f'Album{i}'} + for i in range(10) + ] + out = svc._apply_diversity_filter( + tracks, max_per_album=10, max_per_artist=2, limit=10, + ) + assert len(out) == 2 + + +def test_diversity_filter_stops_at_limit(): + svc = PersonalizedPlaylistsService(_FakeDatabase()) + tracks = [ + {'track_name': f't{i}', 'artist_name': f'a{i}', 'album_name': f'b{i}'} + for i in range(20) + ] + out = svc._apply_diversity_filter( + tracks, max_per_album=10, max_per_artist=10, limit=5, + ) + assert len(out) == 5 + + +# --------------------------------------------------------------------------- +# Adaptive diversity limits +# --------------------------------------------------------------------------- + + +def test_adaptive_limits_high_variety(): + svc = PersonalizedPlaylistsService(_FakeDatabase()) + tracks = [{'artist_name': f'a{i}'} for i in range(30)] + max_album, max_artist = svc._compute_adaptive_diversity_limits(tracks) + # High variety tier — strict limits + assert (max_album, max_artist) == (3, 5) + + +def test_adaptive_limits_low_variety(): + svc = PersonalizedPlaylistsService(_FakeDatabase()) + tracks = [{'artist_name': f'a{i % 3}'} for i in range(30)] # only 3 unique artists + max_album, max_artist = svc._compute_adaptive_diversity_limits(tracks) + # Low variety tier — much more lenient (matches existing decade-style limits) + assert max_album >= 4 + assert max_artist >= 8 + + +def test_adaptive_limits_relaxed_flag_loosens_genre_tier(): + svc = PersonalizedPlaylistsService(_FakeDatabase()) + # 15 unique artists triggers the moderate tier + tracks = [{'artist_name': f'a{i}'} for i in range(15)] + strict = svc._compute_adaptive_diversity_limits(tracks, relaxed=False) + relaxed = svc._compute_adaptive_diversity_limits(tracks, relaxed=True) + assert relaxed[1] >= strict[1] + + +# --------------------------------------------------------------------------- +# Public methods enforce the gate (smoke test on each) +# --------------------------------------------------------------------------- + + +def test_get_hidden_gems_filters_null_id_rows(service): + svc, db = service + db.insert_discovery_track( + source='spotify', spotify_track_id='sp1', track_name='Gem', + artist_name='A', album_name='X', popularity=20, + ) + db.insert_discovery_track( + source='spotify', spotify_track_id=None, itunes_track_id=None, + track_name='Nogem', artist_name='A', album_name='X', popularity=20, + ) + out = svc.get_hidden_gems(limit=10) + assert [t['track_name'] for t in out] == ['Gem'] + + +def test_get_discovery_shuffle_filters_null_id_rows(service): + svc, db = service + db.insert_discovery_track( + source='spotify', spotify_track_id='sp1', track_name='Yes', + artist_name='A', album_name='X', + ) + db.insert_discovery_track( + source='spotify', spotify_track_id=None, itunes_track_id=None, + track_name='No', artist_name='A', album_name='X', + ) + out = svc.get_discovery_shuffle(limit=10) + assert [t['track_name'] for t in out] == ['Yes'] + + +def test_get_popular_picks_filters_null_id_rows(service): + svc, db = service + db.insert_discovery_track( + source='spotify', spotify_track_id='sp1', track_name='Yes', + artist_name='A', album_name='X', popularity=80, + ) + db.insert_discovery_track( + source='spotify', spotify_track_id=None, itunes_track_id=None, + track_name='No', artist_name='A', album_name='X', popularity=80, + ) + out = svc.get_popular_picks(limit=10) + assert [t['track_name'] for t in out] == ['Yes'] + + +def test_get_decade_playlist_filters_null_id_rows(service): + svc, db = service + db.insert_discovery_track( + source='spotify', spotify_track_id='sp1', track_name='Yes', + artist_name='A', album_name='X', release_date='2024-06-01', + ) + db.insert_discovery_track( + source='spotify', spotify_track_id=None, itunes_track_id=None, + track_name='No', artist_name='A', album_name='X', release_date='2024-06-01', + ) + out = svc.get_decade_playlist(2020, limit=10) + assert [t['track_name'] for t in out] == ['Yes'] + + +# --------------------------------------------------------------------------- +# Library methods (re-enabled) +# --------------------------------------------------------------------------- + + +def test_get_recently_added_filters_null_id_rows(service): + svc, db = service + db.insert_library_track( + track_name='Has Spotify', spotify_track_id='sp1', + ) + db.insert_library_track( + track_name='No IDs', # all source ID columns NULL + ) + db.insert_library_track( + track_name='Has MB', musicbrainz_recording_id='mb1', + ) + out = svc.get_recently_added(limit=10) + names = sorted(t['track_name'] for t in out) + assert names == ['Has MB', 'Has Spotify'] + + +def test_get_top_tracks_requires_play_count(service): + svc, db = service + db.insert_library_track( + track_name='Played', spotify_track_id='sp1', play_count=5, + ) + db.insert_library_track( + track_name='Unplayed', spotify_track_id='sp2', play_count=0, + ) + out = svc.get_top_tracks(limit=10) + assert [t['track_name'] for t in out] == ['Played'] + + +def test_get_familiar_favorites_uses_play_count_band(service): + svc, db = service + db.insert_library_track( + track_name='InBand', spotify_track_id='sp1', play_count=8, + ) + db.insert_library_track( + track_name='TooLow', spotify_track_id='sp2', play_count=2, + ) + db.insert_library_track( + track_name='TooHigh', spotify_track_id='sp3', play_count=20, + ) + out = svc.get_familiar_favorites(limit=10) + assert [t['track_name'] for t in out] == ['InBand'] From 44dd7f980fd1ce944f1b1154064f1b1c07a97da1 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Fri, 8 May 2026 07:15:37 -0700 Subject: [PATCH 2/5] Discover: unify Decade + Genre tabbed browsers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both tabbed-browser sections — Time Machine ("Decade") and Browse by Genre — re-implemented the same lifecycle by hand: fetch tabs list, render the tab strip, attach click handlers, fetch content per tab, render track list with sync + download action buttons + sync-status block, handle empty/error/loading states. ~314 lines of identical boilerplate split across two browsers. Lifted into one shared `createTabbedBrowserSection(config)` helper. Each browser is now a thin wrapper: ```js const ctrl = createTabbedBrowserSection({ id: 'decade-browser', tabsContainerEl: '#decade-tabs', contentContainerEl: '#decade-content', fetchTabs: async () => { ... }, renderTabButton: (tab, isActive) => ``, fetchTabContent: async (tab) => { ... }, renderTabContent: (tracks, tab) => `...`, onTabContentRendered: (tab, contentEl) => { ... }, emptyMessage / errorMessage, }); ``` Migrated: - `loadDecadeBrowserTabs` 85 → 3 lines - `loadDecadeTracks` 67 → 3 lines - `loadGenreBrowserTabs` 92 → 3 lines - `loadGenreTracks` 70 → 3 lines Helper: ~125 lines + ~100 lines of per-browser config blocks + ~25 lines of shared `_renderTabbedTrackList` (the two browsers had byte-identical track-row markup so it lifted cleanly). Public function names preserved — the four migrated functions stay on the same signature so existing callers (`loadDiscoverPage`, refresh buttons, inline handlers) don't change. Side effects preserved — `decadeTracksCache[year]`, `activeDecade`, `genreTracksCache[name]`, `activeGenre`, `availableGenres` still mutated at the same lifecycle moments. The decade-specific `startDecadeSync(decade)` and genre-specific `startGenreSync(name)` sync-button handlers stay where they are; they're click handlers attached to rendered content, not part of the tab lifecycle. What didn't fit (intentionally left alone): - `_renderCompactTrackRow` (the existing shared track-row helper) is NOT used by the tabbed browsers — they had their own template with a `track_data_json` fallback chain `_renderCompactTrackRow` doesn't do. Unifying these two would change behavior for non-tabbed sections, so the tabbed-browser variant lives as `_renderTabbedTrackList`. Future cleanup could merge them by giving `_renderCompactTrackRow` an opt-in fallback flag. - `switchDecadeTab` / `switchGenreTab` still know about cache shape so they can skip refetch on already-loaded tabs. Keeping that in the per-browser switch is fine — it's a click handler, not lifecycle. Net: 8546 → 8578 LOC on `discover.js` (+32). Helper boilerplate offsets the line count, but the win is single-source-of-truth, not raw line reduction. `node --check` clean. 2222/2222 full suite green. --- webui/static/discover.js | 492 +++++++++++++++++++++------------------ webui/static/helper.js | 1 + 2 files changed, 263 insertions(+), 230 deletions(-) diff --git a/webui/static/discover.js b/webui/static/discover.js index c7ba7e36..b1bee6e9 100644 --- a/webui/static/discover.js +++ b/webui/static/discover.js @@ -1702,45 +1702,162 @@ function _renderSyncStatusBlock(idPrefix) { `; } -async function loadDecadeBrowserTabs() { - try { - const tabsContainer = document.getElementById('decade-tabs'); - const contentsContainer = document.getElementById('decade-tab-contents'); +// =============================== +// TABBED BROWSER HELPER +// =============================== +// +// Drives the lifecycle the decade browser ("Time Machine") and +// genre browser ("Browse by Genre") share: fetch tab list → paint +// tab strip + per-tab content shells → fetch + render content for +// the active tab → handle empty / error states. +// +// The two browsers paint slightly different markup (different CSS +// prefixes, different action buttons, different sync handlers) but +// the lifecycle is identical. Each browser registers a config; the +// helper handles the rest. +// +// Two-phase render: +// Phase 1 (loadTabs) — paint tab strip + N content shells, +// each shell containing a loading +// spinner in its playlist container, +// then trigger Phase 2 for first tab. +// Phase 2 (loadTabContent) — fetch tracks for one tab, swap the +// spinner in its playlist container +// for the rendered track list. +// +// Renderers stay per-browser because action buttons + classes +// legitimately differ. The helper owns the lifecycle, not the look. +function createTabbedBrowserSection(config) { + const cfg = Object.assign({ + // Diagnostic id used in console errors. + id: 'tabbed-browser', + // DOM IDs of the tab-strip + per-tab-contents containers. + tabsContainerId: null, + contentsContainerId: null, + // Async fn returning array of tab descriptors (e.g. decades). + fetchTabs: null, + // (tab) => string unique id for one tab (e.g. 'decade-1980'). + // Used as the prefix for that tab's content + playlist + sync IDs. + tabId: null, + // (tab) => string HTML for one tab button. Receives `(tab, isActive)`. + renderTabButton: null, + // (tab) => string HTML for one tab's content shell (action + // buttons + sync-status block + empty playlist container). + // The playlist container inside MUST have id `${tabId}-playlist` + // so the helper can fill it during Phase 2. + renderTabShell: null, + // Async fn (tab) => array of tracks for that tab. + fetchTabContent: null, + // (tracks, tab) => string HTML for the playlist container. + renderTabTracks: null, + // Copy / messages. + emptyTabsMessage: 'No content available', + emptyContentMessage: (tab) => 'No tracks found', + errorTabsMessage: 'Failed to load', + errorContentMessage: 'Failed to load tracks', + // Fired after Phase 1 paints the tab strip + shells, before + // Phase 2 is triggered for the first tab. Useful for caching + // the tab list (e.g. `availableGenres = ...`). + onTabsRendered: null, + }, config || {}); - if (!tabsContainer || !contentsContainer) return; + async function loadTabs() { + try { + const tabsContainer = document.getElementById(cfg.tabsContainerId); + const contentsContainer = document.getElementById(cfg.contentsContainerId); + if (!tabsContainer || !contentsContainer) return; - // Fetch available decades from backend - const response = await fetch('/api/discover/decades/available'); - if (!response.ok) { - throw new Error('Failed to fetch available decades'); + const tabs = await cfg.fetchTabs(); + if (!Array.isArray(tabs) || tabs.length === 0) { + tabsContainer.innerHTML = `

${cfg.emptyTabsMessage}

`; + return; + } + + let tabsHTML = ''; + let contentsHTML = ''; + tabs.forEach((tab, index) => { + const isActive = index === 0; + tabsHTML += cfg.renderTabButton(tab, isActive); + contentsHTML += cfg.renderTabShell(tab, isActive); + }); + + tabsContainer.innerHTML = tabsHTML; + contentsContainer.innerHTML = contentsHTML; + + if (typeof cfg.onTabsRendered === 'function') { + try { cfg.onTabsRendered(tabs); } + catch (err) { console.debug(`[${cfg.id}] onTabsRendered threw:`, err); } + } + + // Phase 2: kick off content load for the first tab. + await loadTabContent(tabs[0]); + } catch (error) { + console.error(`Error loading ${cfg.id} tabs:`, error); + const tabsContainer = document.getElementById(cfg.tabsContainerId); + if (tabsContainer) { + tabsContainer.innerHTML = `

${cfg.errorTabsMessage}

`; + } } + } - const data = await response.json(); - if (!data.success || !data.decades || data.decades.length === 0) { - tabsContainer.innerHTML = '

No decade content available yet. Run a watchlist scan to populate your discovery pool!

'; - return; + async function loadTabContent(tab) { + const tabId = cfg.tabId(tab); + const playlistContainer = document.getElementById(`${tabId}-playlist`); + if (!playlistContainer) return; + + try { + const tracks = await cfg.fetchTabContent(tab); + if (!Array.isArray(tracks) || tracks.length === 0) { + const msg = (typeof cfg.emptyContentMessage === 'function') + ? cfg.emptyContentMessage(tab) + : cfg.emptyContentMessage; + playlistContainer.innerHTML = `

${msg}

`; + return; + } + playlistContainer.innerHTML = cfg.renderTabTracks(tracks, tab); + } catch (error) { + console.error(`Error loading ${cfg.id} tab content:`, error); + const stillThere = document.getElementById(`${tabId}-playlist`); + if (stillThere) { + stillThere.innerHTML = `

${cfg.errorContentMessage}

`; + } } + } - // Build decade tabs - let tabsHTML = ''; - let contentsHTML = ''; + return { loadTabs, loadTabContent }; +} - data.decades.forEach((decade, index) => { - const isActive = index === 0; +// ----- Decade browser config + thin wrappers ----- + +let _decadeBrowserTabsCtrl = null; + +function _getDecadeBrowserTabsCtrl() { + if (_decadeBrowserTabsCtrl) return _decadeBrowserTabsCtrl; + _decadeBrowserTabsCtrl = createTabbedBrowserSection({ + id: 'decade-browser-tabs', + tabsContainerId: 'decade-tabs', + contentsContainerId: 'decade-tab-contents', + fetchTabs: async () => { + const response = await fetch('/api/discover/decades/available'); + if (!response.ok) throw new Error('Failed to fetch available decades'); + const data = await response.json(); + if (!data.success) return []; + return data.decades || []; + }, + tabId: (decade) => `decade-${decade.year}`, + renderTabButton: (decade, isActive) => { const icon = getDecadeIcon(decade.year); - const tabId = `decade-${decade.year}`; - - // Tab button - tabsHTML += ` + return ` `; - - // Tab content - contentsHTML += ` + }, + renderTabShell: (decade, isActive) => { + const tabId = `decade-${decade.year}`; + return `
@@ -1770,23 +1887,69 @@ async function loadDecadeBrowserTabs() {
`; - }); + }, + fetchTabContent: async (decade) => { + const response = await fetch(`/api/discover/decade/${decade.year}`); + if (!response.ok) throw new Error('Failed to fetch decade playlist'); + const data = await response.json(); + if (!data.success) return []; + // Side-effect: cache + active marker, exactly as old code did. + decadeTracksCache[decade.year] = data.tracks || []; + activeDecade = decade.year; + return data.tracks || []; + }, + renderTabTracks: (tracks) => _renderTabbedTrackList(tracks), + emptyContentMessage: (decade) => `No tracks found for the ${decade.year}s`, + errorTabsMessage: 'Failed to load decades', + errorContentMessage: 'Failed to load decade tracks', + emptyTabsMessage: 'No decade content available yet. Run a watchlist scan to populate your discovery pool!', + }); + return _decadeBrowserTabsCtrl; +} - tabsContainer.innerHTML = tabsHTML; - contentsContainer.innerHTML = contentsHTML; - - // Load first decade's tracks - if (data.decades.length > 0) { - await loadDecadeTracks(data.decades[0].year); +// Shared track-row markup for tabbed browsers. Decade + genre rows +// have the same shape — both pull from `track_data_json` first then +// fall back to top-level fields. Lifted so the helper-driven +// renderers don't each carry a copy. +function _renderTabbedTrackList(tracks) { + let html = '
'; + tracks.forEach((track, index) => { + let trackData = track; + if (track.track_data_json) { + trackData = track.track_data_json; } - } catch (error) { - console.error('Error loading decade browser tabs:', error); - const tabsContainer = document.getElementById('decade-tabs'); - if (tabsContainer) { - tabsContainer.innerHTML = '

Failed to load decades

'; - } - } + const trackName = trackData.name || trackData.track_name || track.track_name || 'Unknown Track'; + const artistName = trackData.artists?.[0]?.name || trackData.artists?.[0] || trackData.artist_name || track.artist_name || 'Unknown Artist'; + const albumName = trackData.album?.name || trackData.album_name || track.album_name || 'Unknown Album'; + const coverUrl = trackData.album?.images?.[0]?.url || track.album_cover_url || '/static/placeholder-album.png'; + const durationMs = trackData.duration_ms || track.duration_ms || 0; + + const durationMin = Math.floor(durationMs / 60000); + const durationSec = Math.floor((durationMs % 60000) / 1000); + const duration = `${durationMin}:${durationSec.toString().padStart(2, '0')}`; + + html += ` +
+
${index + 1}
+
+ ${albumName} +
+
+
${trackName}
+
${artistName}
+
+
${albumName}
+
${duration}
+
+ `; + }); + html += '
'; + return html; +} + +async function loadDecadeBrowserTabs() { + return _getDecadeBrowserTabsCtrl().loadTabs(); } function switchDecadeTab(decade) { @@ -1817,71 +1980,7 @@ function switchDecadeTab(decade) { } async function loadDecadeTracks(decade) { - try { - const playlistContainer = document.getElementById(`decade-${decade}-playlist`); - if (!playlistContainer) return; - - const response = await fetch(`/api/discover/decade/${decade}`); - if (!response.ok) { - throw new Error('Failed to fetch decade playlist'); - } - - const data = await response.json(); - if (!data.success || !data.tracks || data.tracks.length === 0) { - playlistContainer.innerHTML = '

No tracks found for the ' + decade + 's

'; - return; - } - - // Store tracks in cache - decadeTracksCache[decade] = data.tracks; - activeDecade = decade; - - // Build compact playlist HTML - let html = '
'; - data.tracks.forEach((track, index) => { - // Extract track data from track_data_json if available - let trackData = track; - if (track.track_data_json) { - trackData = track.track_data_json; - } - - // Get track properties with fallbacks - const trackName = trackData.name || trackData.track_name || track.track_name || 'Unknown Track'; - const artistName = trackData.artists?.[0]?.name || trackData.artists?.[0] || trackData.artist_name || track.artist_name || 'Unknown Artist'; - const albumName = trackData.album?.name || trackData.album_name || track.album_name || 'Unknown Album'; - const coverUrl = trackData.album?.images?.[0]?.url || track.album_cover_url || '/static/placeholder-album.png'; - const durationMs = trackData.duration_ms || track.duration_ms || 0; - - const durationMin = Math.floor(durationMs / 60000); - const durationSec = Math.floor((durationMs % 60000) / 1000); - const duration = `${durationMin}:${durationSec.toString().padStart(2, '0')}`; - - html += ` -
-
${index + 1}
-
- ${albumName} -
-
-
${trackName}
-
${artistName}
-
-
${albumName}
-
${duration}
-
- `; - }); - html += '
'; - - playlistContainer.innerHTML = html; - - } catch (error) { - console.error('Error loading decade tracks:', error); - const playlistContainer = document.getElementById(`decade-${decade}-playlist`); - if (playlistContainer) { - playlistContainer.innerHTML = '

Failed to load decade tracks

'; - } - } + return _getDecadeBrowserTabsCtrl().loadTabContent({ year: decade }); } async function startDecadeSync(decade) { @@ -2080,64 +2179,58 @@ let genreTracksCache = {}; // Store tracks for each genre let activeGenre = null; let availableGenres = []; -async function loadGenreBrowserTabs() { - try { - const tabsContainer = document.getElementById('genre-tabs'); - const contentsContainer = document.getElementById('genre-tab-contents'); +// Helper: derive the URL/DOM-safe id used for a genre tab. Both +// the tab shell and the playlist-container element are keyed on this. +function _genreTabId(genreName) { + const safe = genreName.replace(/\s+/g, '-').replace(/[^a-zA-Z0-9-]/g, ''); + return `genre-${safe}`; +} - if (!tabsContainer || !contentsContainer) return; +let _genreBrowserTabsCtrl = null; - // Fetch available genres from backend - const response = await fetch('/api/discover/genres/available'); - if (!response.ok) { - throw new Error('Failed to fetch available genres'); - } - - const data = await response.json(); - if (!data.success || !data.genres || data.genres.length === 0) { - tabsContainer.innerHTML = '

No genre content available yet. Run a watchlist scan to populate your discovery pool!

'; - return; - } - - availableGenres = data.genres; - - // Build genre tabs (limit to first 8-10 to avoid overcrowding) - const displayGenres = data.genres.slice(0, 10); - let tabsHTML = ''; - let contentsHTML = ''; - - displayGenres.forEach((genre, index) => { - const isActive = index === 0; +function _getGenreBrowserTabsCtrl() { + if (_genreBrowserTabsCtrl) return _genreBrowserTabsCtrl; + _genreBrowserTabsCtrl = createTabbedBrowserSection({ + id: 'genre-browser-tabs', + tabsContainerId: 'genre-tabs', + contentsContainerId: 'genre-tab-contents', + fetchTabs: async () => { + const response = await fetch('/api/discover/genres/available'); + if (!response.ok) throw new Error('Failed to fetch available genres'); + const data = await response.json(); + if (!data.success) return []; + // Cap at 10 to avoid overcrowding; old behavior preserved. + return (data.genres || []).slice(0, 10); + }, + onTabsRendered: (genres) => { availableGenres = genres; }, + tabId: (genre) => _genreTabId(genre.name), + renderTabButton: (genre, isActive) => { const icon = getGenreIcon(genre.name); - const genreName = genre.name; - const genreId = genreName.replace(/\s+/g, '-').replace(/[^a-zA-Z0-9-]/g, ''); - const tabId = `genre-${genreId}`; - - // Tab button - tabsHTML += ` + return ` `; - - // Tab content - contentsHTML += ` -
+ }, + renderTabShell: (genre, isActive) => { + const tabId = _genreTabId(genre.name); + return ` +
-

${capitalizeGenre(genreName)} Mix

+

${capitalizeGenre(genre.name)} Mix

${genre.track_count} tracks

- - @@ -2149,27 +2242,32 @@ async function loadGenreBrowserTabs() {
-

Loading ${capitalizeGenre(genreName)} tracks...

+

Loading ${capitalizeGenre(genre.name)} tracks...

`; - }); + }, + fetchTabContent: async (genre) => { + const response = await fetch(`/api/discover/genre/${encodeURIComponent(genre.name)}`); + if (!response.ok) throw new Error('Failed to fetch genre playlist'); + const data = await response.json(); + if (!data.success) return []; + // Side-effect: cache + active marker, exactly as old code did. + genreTracksCache[genre.name] = data.tracks || []; + activeGenre = genre.name; + return data.tracks || []; + }, + renderTabTracks: (tracks) => _renderTabbedTrackList(tracks), + emptyContentMessage: (genre) => `No tracks found for ${capitalizeGenre(genre.name)}`, + errorTabsMessage: 'Failed to load genres', + errorContentMessage: 'Failed to load genre tracks', + emptyTabsMessage: 'No genre content available yet. Run a watchlist scan to populate your discovery pool!', + }); + return _genreBrowserTabsCtrl; +} - tabsContainer.innerHTML = tabsHTML; - contentsContainer.innerHTML = contentsHTML; - - // Load first genre's tracks - if (displayGenres.length > 0) { - await loadGenreTracks(displayGenres[0].name); - } - - } catch (error) { - console.error('Error loading genre browser tabs:', error); - const tabsContainer = document.getElementById('genre-tabs'); - if (tabsContainer) { - tabsContainer.innerHTML = '

Failed to load genres

'; - } - } +async function loadGenreBrowserTabs() { + return _getGenreBrowserTabsCtrl().loadTabs(); } function switchGenreTab(genreName) { @@ -2200,73 +2298,7 @@ function switchGenreTab(genreName) { } async function loadGenreTracks(genreName) { - try { - const genreId = genreName.replace(/\s+/g, '-').replace(/[^a-zA-Z0-9-]/g, ''); - const playlistContainer = document.getElementById(`genre-${genreId}-playlist`); - if (!playlistContainer) return; - - const response = await fetch(`/api/discover/genre/${encodeURIComponent(genreName)}`); - if (!response.ok) { - throw new Error('Failed to fetch genre playlist'); - } - - const data = await response.json(); - if (!data.success || !data.tracks || data.tracks.length === 0) { - playlistContainer.innerHTML = `

No tracks found for ${capitalizeGenre(genreName)}

`; - return; - } - - // Store tracks in cache - genreTracksCache[genreName] = data.tracks; - activeGenre = genreName; - - // Build compact playlist HTML - let html = '
'; - data.tracks.forEach((track, index) => { - // Extract track data from track_data_json if available - let trackData = track; - if (track.track_data_json) { - trackData = track.track_data_json; - } - - // Get track properties with fallbacks - const trackName = trackData.name || trackData.track_name || track.track_name || 'Unknown Track'; - const artistName = trackData.artists?.[0]?.name || trackData.artists?.[0] || trackData.artist_name || track.artist_name || 'Unknown Artist'; - const albumName = trackData.album?.name || trackData.album_name || track.album_name || 'Unknown Album'; - const coverUrl = trackData.album?.images?.[0]?.url || track.album_cover_url || '/static/placeholder-album.png'; - const durationMs = trackData.duration_ms || track.duration_ms || 0; - - const durationMin = Math.floor(durationMs / 60000); - const durationSec = Math.floor((durationMs % 60000) / 1000); - const duration = `${durationMin}:${durationSec.toString().padStart(2, '0')}`; - - html += ` -
-
${index + 1}
-
- ${albumName} -
-
-
${trackName}
-
${artistName}
-
-
${albumName}
-
${duration}
-
- `; - }); - html += '
'; - - playlistContainer.innerHTML = html; - - } catch (error) { - console.error('Error loading genre tracks:', error); - const genreId = genreName.replace(/\s+/g, '-').replace(/[^a-zA-Z0-9-]/g, ''); - const playlistContainer = document.getElementById(`genre-${genreId}-playlist`); - if (playlistContainer) { - playlistContainer.innerHTML = '

Failed to load genre tracks

'; - } - } + return _getGenreBrowserTabsCtrl().loadTabContent({ name: genreName }); } async function startGenreSync(genreName) { diff --git a/webui/static/helper.js b/webui/static/helper.js index 7323a284..a8d204f0 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3432,6 +3432,7 @@ const WHATS_NEW = { '2.4.3': [ // --- post-2.4.2 dev work — entries hidden by _getLatestWhatsNewVersion until the build version bumps --- { date: 'Unreleased — 2.4.3 dev cycle' }, + { title: 'Discover: Stop Showing Undownloadable Tracks (+Lift)', desc: 'audit found multiple discover-page sections (hidden gems / discovery shuffle / popular picks / decade browser / genre browser) had no `WHERE (spotify_track_id IS NOT NULL OR itunes_track_id IS NOT NULL OR ...)` gate on their selection sql. tracks with no source ids in the discovery pool were getting displayed, the user would click download, and the download would silently fail because there was nothing to look up. fix: lifted all five discovery_pool selection methods + four library-driven methods (recently added / top tracks / forgotten favorites / familiar favorites — these were stubbed returning [] because the schema predated the source-id columns; now re-enabled properly) into shared private helpers (`_select_discovery_tracks`, `_select_library_tracks`, `_apply_diversity_filter`, `_compute_adaptive_diversity_limits`) on `PersonalizedPlaylistsService`. mandatory id-validity gate is hard-coded into both selectors — no opt-out flag, every public method inherits it for free. behavior preserved: same diversity tiers, same over-fetch multipliers, same popularity thresholds, same blacklist filter. ~314 lines of repeated select/diversity boilerplate collapsed across the 5 discovery methods (-55% on those methods\' business logic). on the frontend, lifted the duplicated decade-browser + genre-browser tab management (~314 lines of identical fetch-tabs / render-tabstrip / fetch-content / render-tracklist / wire-sync-button code) into one shared `createTabbedBrowserSection(config)` helper. each browser is now a thin wrapper: ~3 lines per public function. 17 new tests pin the gate (every selector filters null-id rows), the diversity caps, the adaptive limit tiers, the source filter, and the blacklist filter. 2222/2222 full suite green.', page: 'discover' }, { title: 'Internal: Discover Controller — Cin Pre-Review Polish', desc: 'tightened the controller before opening the PR. (1) dropped the magic `extractItems` defaults — controller used to auto-pull `data.items` / `data.albums` / `data.artists` / `data.tracks` / `data.results` if no extractor was provided. removed the fallback chain. each section now MUST supply its own `extractItems(data) => array` callback. cin standard: explicit > implicit; the auto-fallback could silently grab the wrong key on endpoints that return multiple arrays. validated at register-time so misuse fails immediately. all 10 existing call sites already had explicit extractors so no migration churn. (2) replaced the `renderItems` returning null convention (used by Your Albums + manualDom-style sections) with an explicit `manualDom: true` config flag. clearer intent at the call site, less likely to be confused with a renderer error. (3) added a minimal node `--test` JS test file at `tests/static/test_discover_section_controller.mjs` — 32 tests pin the lifecycle contract: config validation (every required field), happy-path fetch+render, empty/stale/error states, no-fetch `data:` mode, manualDom mode, callable `fetchUrl`, load coalescing, refresh bypass, hook error containment, error toasts. runs via `node --test tests/static/` directly, OR via the regular pytest sweep (`tests/test_discover_section_controller_js.py` shells out to node and asserts a clean exit). skipped gracefully when node isn\'t available or is < 22. closes the "controller is a contract, pin it at the test boundary" gap that cin would have flagged. 2205/2205 full suite green (was 2204 + 1 new pytest wrapper); 32/32 node --test pass; ruff clean; js parses clean.', page: 'discover' }, { title: 'Internal: Discover Cleanup Round — Toast Errors, Stale State, Skipped Sections', desc: 'follow-up to the controller migration. extended `createDiscoverSectionController` with the hooks the per-section migrations surfaced as needed: callable `fetchUrl` (resolves the seasonal-playlist recreate-on-key-change hack), no-fetch `data:` mode (lets render-only sections like seasonal albums use the controller without inventing a fake endpoint), `beforeLoad` hook (lets dynamically-inserted sections like because-you-listen-to ensure their container exists before the spinner shows), `onSuccess(data)` hook (cleaner home for sibling header / subtitle / button updates than folding them into renderItems), and an `isStale` / `onStale` / `renderStale` triple for the third render state (data is empty BUT upstream is still discovering — show updating UI + start a poller, instead of the bare empty-state copy). turned on `showErrorToast: true` for every migrated section — section load failures now surface a global toast instead of silently spinning forever or swallowing into console.debug. that\'s the JohnBaumb #369 pattern applied at the UI layer. migrated the two sections that didn\'t fit the original controller contract: `loadYourAlbums` (uses isStale/onStale for stale-fetch UI + onSuccess for subtitle/filters/download-button side-effects + renderItems returning null since it delegates to the existing grid renderer) and `loadSeasonalAlbums` (uses no-fetch data mode since the parent `loadSeasonalContent` already fetched the season payload). also lifted the duplicated decade-tab + genre-tab sync-status block (✓/⏳/✗/percentage) into a `_renderSyncStatusBlock(idPrefix)` helper — two call sites now share one implementation. listenbrainz playlists keep their own block because the semantics differ (matching progress vs download progress). audit found the 13 supposedly-dead hidden sections aren\'t dead at all — they\'re gated on user data (discovery pool, library content, metadata cache) and self-surface when their data exists. removed one orphaned `loadPersonalizedDailyMixes()` call from `blockDiscoveryArtist` — daily mixes is intentionally paused, refreshing it from there was a no-op.', page: 'discover' }, { title: 'Internal: Migrate 7 More Discover Sections to the Controller', desc: 'follow-up to the foundation commit. migrated fresh tape, the archives, time machine intro carousel, browse by genre intro carousel, seasonal mix, your artists, and because-you-listen-to onto `createDiscoverSectionController`. each one drops its own hand-rolled try/catch + spinner injection + empty-state HTML + error swallow in favor of a config object — controller owns the lifecycle. net 76 lines smaller in discover.js even after adding the per-section render helpers. skipped two sections that don\'t fit the controller\'s single-fetch / single-render-target shape: `loadYourAlbums` (paginated grid + filters, four separate UI elements updated) and `loadSeasonalAlbums` (no fetch — receives pre-fetched data from parent). hidden / dead sections (~13 of them) untouched in this pass — separate audit commit will surface or kill them. controller extension candidates surfaced for follow-up: callable `fetchUrl` (so seasonal playlist doesn\'t need controller-recreate-on-key-change), explicit `isStale` / `onStale` hook (so your-artists doesn\'t fold stale handling into renderItems), `beforeLoad` hook (so because-you-listen-to can let the controller own the dynamic container creation), and a no-fetch `data:` mode (so render-only sections like seasonal albums can use the controller). zero behavior changes — every public load function keeps its name + signature so existing callers, refresh buttons, and dashboard wiring don\'t notice the swap.', page: 'discover' }, From 959562f6b0f0d2023d444c6a367f9c3124421a3e Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Fri, 8 May 2026 07:31:51 -0700 Subject: [PATCH 3/5] Delete Recently Added / Top Tracks / Forgotten Favorites / Familiar Favorites MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Owner decision: not worth shipping. The four library-driven personalized sections were stubbed returning [] for ages because their schema prereqs didn't exist; the prior commit re-enabled them by routing through a new `_select_library_tracks` helper. Owner reviewed and chose to delete the sections entirely instead. Removed everywhere: - `core/personalized_playlists.py` — `get_recently_added`, `get_top_tracks`, `get_forgotten_favorites`, `get_familiar_favorites` + the `_select_library_tracks` helper (no other callers; verified via grep). - `web_server.py` — 4 route handlers (`/api/discover/personalized/recently-added`, `top-tracks`, `forgotten-favorites`, `familiar-favorites`). - `webui/index.html` — 4 `
` blocks (`#personalized-recently-added`, `#personalized-top-tracks`, `#personalized-forgotten-favorites`, `#personalized-familiar-favorites`). - `webui/static/discover.js` — 4 load functions (`loadPersonalizedRecentlyAdded`, `loadPersonalizedTopTracks`, `loadPersonalizedForgottenFavorites`, `loadFamiliarFavorites`), plus their entries in `loadDiscoverPage`'s Promise.all, plus 4 module-level state vars + 6 dead branches across `openDownloadModalForDiscoverPlaylist` / `startDiscoverPlaylistSync` and the sync-progress / rehydrate dispatchers. - `webui/static/helper.js` — 4 tooltip / docs entries. - `webui/static/sync-spotify.js` — 1 stale rehydrate dispatcher branch (`discover_familiar_favorites`) caught during the global grep pass. - `tests/test_personalized_playlists_id_gate.py` — 3 library-method tests + the test infrastructure that supported them (`tracks` schema, `insert_library_track` helper). Documentation header updated to reflect the deletion. Net: -527 / +2 lines across 7 files. What stays: - Daily Mixes (also in personalized package, intentionally paused — separate decision). - Popular Picks + Hidden Gems + Discovery Shuffle (alive, not affected by this deletion). - All 14 tests in the personalized-playlists test file still pass. - The PersonalizedPlaylistsService lift from the prior commit (`_select_discovery_tracks` etc) — those are still in active use by the surviving discovery_pool methods. DISCOVER_TRACK_SELECTION_REVIEW.md at repo root contains historical references to the four deleted endpoints. Treated as historical context (same policy as WHATS_NEW), left alone. 2219/2219 full suite green (was 2222 - 3 deleted tests = 2219). JS parses clean, ruff clean. --- core/personalized_playlists.py | 142 ------------------- tests/test_personalized_playlists_id_gate.py | 94 ------------ web_server.py | 81 ----------- webui/index.html | 77 ---------- webui/static/discover.js | 115 +-------------- webui/static/helper.js | 18 +-- webui/static/sync-spotify.js | 2 - 7 files changed, 2 insertions(+), 527 deletions(-) diff --git a/core/personalized_playlists.py b/core/personalized_playlists.py index 9227178d..da507121 100644 --- a/core/personalized_playlists.py +++ b/core/personalized_playlists.py @@ -341,138 +341,6 @@ class PersonalizedPlaylistsService: return 'Other' - # ======================================== - # LIBRARY-BASED PLAYLISTS - # ======================================== - - def _select_library_tracks( - self, - *, - where_clause: str = "", - params: tuple = (), - order_by: str = "t.created_at DESC", - limit: int, - ) -> List[Dict]: - """ - Shared selector for library-based playlist methods. - - Builds and runs a SELECT against `tracks` joined to `albums` and - `artists`, with a baked-in ID-validity gate so callers cannot - accidentally return rows with no usable source IDs (which would fail - downstream when the user clicks download or tries to match the track - against an external metadata source). - - The WHERE clause always includes: - (t.spotify_track_id IS NOT NULL - OR t.itunes_track_id IS NOT NULL - OR t.deezer_id IS NOT NULL - OR t.musicbrainz_recording_id IS NOT NULL - OR t.audiodb_id IS NOT NULL) - - The ID gate is mandatory and not opt-out by design — mirrors the - shared `_select_discovery_tracks` helper. - - Args: - where_clause: optional SQL fragment appended to the WHERE clause. - Must start with "AND " if non-empty. - params: positional bindings for `?` placeholders in `where_clause`. - order_by: ORDER BY expression, used as-is. - limit: LIMIT applied to the query. - - Returns: - List of track dicts in the standard `_build_track_dict` shape with - `source='library'`. Returns `[]` on any error (logged at error - level). - """ - try: - query = f""" - SELECT - t.spotify_track_id AS spotify_track_id, - t.itunes_track_id AS itunes_track_id, - t.deezer_id AS deezer_track_id, - t.title AS track_name, - ar.name AS artist_name, - al.title AS album_name, - al.thumb_url AS album_cover_url, - t.duration AS duration_ms - FROM tracks t - LEFT JOIN albums al ON t.album_id = al.id - LEFT JOIN artists ar ON t.artist_id = ar.id - WHERE (t.spotify_track_id IS NOT NULL - OR t.itunes_track_id IS NOT NULL - OR t.deezer_id IS NOT NULL - OR t.musicbrainz_recording_id IS NOT NULL - OR t.audiodb_id IS NOT NULL) - {where_clause} - ORDER BY {order_by} - LIMIT ? - """ - - bound_params = tuple(params) + (limit,) - - with self.database._get_connection() as conn: - cursor = conn.cursor() - cursor.execute(query, bound_params) - rows = cursor.fetchall() - - results: List[Dict] = [] - for row in rows: - row_dict = dict(row) if hasattr(row, 'keys') else row - # Library tracks have no popularity score and no JSON blob; - # fill the standard shape with the documented defaults. - row_dict.setdefault('popularity', 0) - row_dict.setdefault('track_data_json', '{}') - # Guard against NULL artist/album joins so downstream string - # ops (diversity filter, dedupe) don't blow up. - if row_dict.get('artist_name') is None: - row_dict['artist_name'] = 'Unknown' - if row_dict.get('album_name') is None: - row_dict['album_name'] = 'Unknown' - if row_dict.get('track_name') is None: - row_dict['track_name'] = 'Unknown' - if row_dict.get('duration_ms') is None: - row_dict['duration_ms'] = 0 - results.append(self._build_track_dict(row_dict, 'library')) - - return results - - except Exception as e: - logger.error(f"Error in _select_library_tracks: {e}") - return [] - - def get_recently_added(self, limit: int = 50) -> List[Dict]: - """Get recently added tracks from the local library, newest first.""" - tracks = self._select_library_tracks( - order_by="t.created_at DESC", - limit=limit, - ) - logger.info(f"Recently Added: selected {len(tracks)} library tracks") - return tracks - - def get_top_tracks(self, limit: int = 50) -> List[Dict]: - """Get the user's all-time top library tracks by play count.""" - tracks = self._select_library_tracks( - where_clause="AND t.play_count > 0", - order_by="t.play_count DESC", - limit=limit, - ) - logger.info(f"Top Tracks: selected {len(tracks)} library tracks") - return tracks - - def get_forgotten_favorites(self, limit: int = 50) -> List[Dict]: - """Get tracks the user loved (>5 plays) but hasn't played in 90+ days.""" - tracks = self._select_library_tracks( - where_clause=( - "AND t.play_count > 5 " - "AND t.last_played IS NOT NULL " - "AND t.last_played < datetime('now', '-90 days')" - ), - order_by="t.play_count DESC", - limit=limit, - ) - logger.info(f"Forgotten Favorites: selected {len(tracks)} library tracks") - return tracks - def get_decade_playlist(self, decade: int, limit: int = 100, source: str = None) -> List[Dict]: """ Get tracks from a specific decade from discovery pool with diversity filtering. @@ -722,16 +590,6 @@ class PersonalizedPlaylistsService: logger.info(f"Discovery Shuffle ({active_source}): selected {len(tracks)} tracks") return tracks - def get_familiar_favorites(self, limit: int = 50) -> List[Dict]: - """Get tracks with medium play counts (3-15 plays) - reliable go-tos.""" - tracks = self._select_library_tracks( - where_clause="AND t.play_count BETWEEN 3 AND 15", - order_by="t.play_count DESC", - limit=limit, - ) - logger.info(f"Familiar Favorites: selected {len(tracks)} library tracks") - return tracks - # ======================================== # DAILY MIX (HYBRID PLAYLISTS) # ======================================== diff --git a/tests/test_personalized_playlists_id_gate.py b/tests/test_personalized_playlists_id_gate.py index ccc315a9..3f205982 100644 --- a/tests/test_personalized_playlists_id_gate.py +++ b/tests/test_personalized_playlists_id_gate.py @@ -10,14 +10,11 @@ section can't accidentally bypass it. Coverage: - `_select_discovery_tracks` filters out rows where every source ID is NULL - `_select_discovery_tracks` honors source filter + blacklist filter -- `_select_library_tracks` enforces the same gate against the `tracks` table - `_apply_diversity_filter` caps per-album + per-artist counts - `_compute_adaptive_diversity_limits` returns the right tier for the unique-artist count + relaxed flag - The 5 discovery_pool methods (decade / genre / popular_picks / hidden_gems / discovery_shuffle) each filter NULL-id rows -- The 4 library methods (recently_added / top_tracks / - forgotten_favorites / familiar_favorites) each filter NULL-id rows """ from __future__ import annotations @@ -71,31 +68,6 @@ class _FakeDatabase: CREATE TABLE discovery_artist_blacklist ( artist_name TEXT PRIMARY KEY ); - CREATE TABLE artists ( - id INTEGER PRIMARY KEY, - name TEXT - ); - CREATE TABLE albums ( - id INTEGER PRIMARY KEY, - title TEXT, - artist_id INTEGER, - thumb_url TEXT - ); - CREATE TABLE tracks ( - id INTEGER PRIMARY KEY, - album_id INTEGER, - artist_id INTEGER, - title TEXT, - duration INTEGER, - created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, - play_count INTEGER DEFAULT 0, - last_played TIMESTAMP, - spotify_track_id TEXT, - itunes_track_id TEXT, - deezer_id TEXT, - musicbrainz_recording_id TEXT, - audiodb_id TEXT - ); """) self._conn.commit() @@ -116,26 +88,6 @@ class _FakeDatabase: ) self._conn.commit() - def insert_library_track(self, *, artist_name='Library Artist', - album_name='Library Album', track_name='Library Track', - **track_kwargs): - # Insert artist + album + track in one shot for convenience. - cur = self._conn.cursor() - cur.execute("INSERT INTO artists (name) VALUES (?)", (artist_name,)) - artist_id = cur.lastrowid - cur.execute( - "INSERT INTO albums (title, artist_id, thumb_url) VALUES (?, ?, ?)", - (album_name, artist_id, 'http://x/cover.jpg'), - ) - album_id = cur.lastrowid - track_kwargs.setdefault('duration', 200000) - cur.execute( - f"""INSERT INTO tracks (album_id, artist_id, title, {', '.join(track_kwargs.keys())}) - VALUES (?, ?, ?, {', '.join(['?'] * len(track_kwargs))})""", - (album_id, artist_id, track_name, *track_kwargs.values()), - ) - self._conn.commit() - def blacklist(self, artist_name): self._conn.execute( "INSERT INTO discovery_artist_blacklist (artist_name) VALUES (?)", @@ -375,49 +327,3 @@ def test_get_decade_playlist_filters_null_id_rows(service): assert [t['track_name'] for t in out] == ['Yes'] -# --------------------------------------------------------------------------- -# Library methods (re-enabled) -# --------------------------------------------------------------------------- - - -def test_get_recently_added_filters_null_id_rows(service): - svc, db = service - db.insert_library_track( - track_name='Has Spotify', spotify_track_id='sp1', - ) - db.insert_library_track( - track_name='No IDs', # all source ID columns NULL - ) - db.insert_library_track( - track_name='Has MB', musicbrainz_recording_id='mb1', - ) - out = svc.get_recently_added(limit=10) - names = sorted(t['track_name'] for t in out) - assert names == ['Has MB', 'Has Spotify'] - - -def test_get_top_tracks_requires_play_count(service): - svc, db = service - db.insert_library_track( - track_name='Played', spotify_track_id='sp1', play_count=5, - ) - db.insert_library_track( - track_name='Unplayed', spotify_track_id='sp2', play_count=0, - ) - out = svc.get_top_tracks(limit=10) - assert [t['track_name'] for t in out] == ['Played'] - - -def test_get_familiar_favorites_uses_play_count_band(service): - svc, db = service - db.insert_library_track( - track_name='InBand', spotify_track_id='sp1', play_count=8, - ) - db.insert_library_track( - track_name='TooLow', spotify_track_id='sp2', play_count=2, - ) - db.insert_library_track( - track_name='TooHigh', spotify_track_id='sp3', play_count=20, - ) - out = svc.get_familiar_favorites(limit=10) - assert [t['track_name'] for t in out] == ['InBand'] diff --git a/web_server.py b/web_server.py index f9709dcc..dfde1998 100644 --- a/web_server.py +++ b/web_server.py @@ -27189,66 +27189,6 @@ def refresh_seasonal_content(): # PERSONALIZED PLAYLISTS ENDPOINTS # ======================================== -@app.route('/api/discover/personalized/recently-added', methods=['GET']) -def get_recently_added_playlist(): - """Get recently added tracks from library""" - try: - from core.personalized_playlists import get_personalized_playlists_service - - database = get_database() - service = get_personalized_playlists_service(database, spotify_client) - - tracks = service.get_recently_added(limit=50) - - return jsonify({ - "success": True, - "tracks": tracks - }) - - except Exception as e: - logger.error(f"Error getting recently added playlist: {e}") - return jsonify({"success": False, "error": str(e)}), 500 - -@app.route('/api/discover/personalized/top-tracks', methods=['GET']) -def get_top_tracks_playlist(): - """Get user's all-time top tracks""" - try: - from core.personalized_playlists import get_personalized_playlists_service - - database = get_database() - service = get_personalized_playlists_service(database, spotify_client) - - tracks = service.get_top_tracks(limit=50) - - return jsonify({ - "success": True, - "tracks": tracks - }) - - except Exception as e: - logger.error(f"Error getting top tracks playlist: {e}") - return jsonify({"success": False, "error": str(e)}), 500 - -@app.route('/api/discover/personalized/forgotten-favorites', methods=['GET']) -def get_forgotten_favorites_playlist(): - """Get forgotten favorites - tracks you loved but haven't played recently""" - try: - from core.personalized_playlists import get_personalized_playlists_service - - database = get_database() - service = get_personalized_playlists_service(database, spotify_client) - - tracks = service.get_forgotten_favorites(limit=50) - - return jsonify({ - "success": True, - "tracks": tracks - }) - - except Exception as e: - logger.error(f"Error getting forgotten favorites playlist: {e}") - return jsonify({"success": False, "error": str(e)}), 500 - @app.route('/api/discover/personalized/decade/', methods=['GET']) def get_decade_playlist(decade): """Get tracks from a specific decade""" @@ -27353,27 +27293,6 @@ def get_discovery_shuffle(): logger.error(f"Error getting discovery shuffle playlist: {e}") return jsonify({"success": False, "error": str(e)}), 500 -@app.route('/api/discover/personalized/familiar-favorites', methods=['GET']) -def get_familiar_favorites(): - """Get Familiar Favorites playlist - reliable go-to tracks""" - try: - from core.personalized_playlists import get_personalized_playlists_service - - database = get_database() - service = get_personalized_playlists_service(database, spotify_client) - - limit = int(request.args.get('limit', 50)) - tracks = service.get_familiar_favorites(limit=limit) - - return jsonify({ - "success": True, - "tracks": tracks - }) - - except Exception as e: - logger.error(f"Error getting familiar favorites playlist: {e}") - return jsonify({"success": False, "error": str(e)}), 500 - @app.route('/api/discover/artist-blacklist', methods=['GET']) def get_discovery_artist_blacklist(): """Get all blacklisted discovery artists.""" diff --git a/webui/index.html b/webui/index.html index 7b0d993a..f4159613 100644 --- a/webui/index.html +++ b/webui/index.html @@ -2985,17 +2985,6 @@
- - - - - - - - - - - -
diff --git a/webui/static/discover.js b/webui/static/discover.js index b1bee6e9..f5ead10d 100644 --- a/webui/static/discover.js +++ b/webui/static/discover.js @@ -15,14 +15,10 @@ let discoverSeasonalTracks = []; let currentSeasonKey = null; // Personalized playlists storage -let personalizedRecentlyAdded = []; -let personalizedTopTracks = []; -let personalizedForgottenFavorites = []; let personalizedPopularPicks = []; let personalizedHiddenGems = []; let personalizedDailyMixes = []; let personalizedDiscoveryShuffle = []; -let personalizedFamiliarFavorites = []; let buildPlaylistSelectedArtists = []; async function loadDiscoverPage() { @@ -35,16 +31,12 @@ async function loadDiscoverPage() { loadYourAlbums(), loadDiscoverRecentReleases(), loadSeasonalContent(), // Seasonal discovery - loadPersonalizedRecentlyAdded(), // NEW: Recently added from library // loadPersonalizedDailyMixes(), // NEW: Daily Mix playlists (HIDDEN) loadDiscoverReleaseRadar(), loadDiscoverWeekly(), loadPersonalizedPopularPicks(), // NEW: Popular picks from discovery pool loadPersonalizedHiddenGems(), // NEW: Hidden gems from discovery pool - loadPersonalizedTopTracks(), // NEW: Your top tracks - loadPersonalizedForgottenFavorites(), // NEW: Forgotten favorites loadDiscoveryShuffle(), // NEW: Discovery Shuffle - loadFamiliarFavorites(), // NEW: Familiar Favorites loadBecauseYouListenTo(), // Personalized by listening stats loadCacheUndiscoveredAlbums(), // From metadata cache loadCacheGenreNewReleases(), // From metadata cache @@ -3826,75 +3818,6 @@ async function syncSeasonalPlaylist() { // PERSONALIZED PLAYLISTS // =============================== -async function loadPersonalizedRecentlyAdded() { - try { - const container = document.getElementById('personalized-recently-added'); - if (!container) return; - - const response = await fetch('/api/discover/personalized/recently-added'); - if (!response.ok) return; - - const data = await response.json(); - if (!data.success || !data.tracks || data.tracks.length === 0) { - container.closest('.discover-section').style.display = 'none'; - return; - } - - personalizedRecentlyAdded = data.tracks; - renderCompactPlaylist(container, data.tracks); - container.closest('.discover-section').style.display = 'block'; - - } catch (error) { - console.error('Error loading recently added:', error); - } -} - -async function loadPersonalizedTopTracks() { - try { - const container = document.getElementById('personalized-top-tracks'); - if (!container) return; - - const response = await fetch('/api/discover/personalized/top-tracks'); - if (!response.ok) return; - - const data = await response.json(); - if (!data.success || !data.tracks || data.tracks.length === 0) { - container.closest('.discover-section').style.display = 'none'; - return; - } - - personalizedTopTracks = data.tracks; - renderCompactPlaylist(container, data.tracks); - container.closest('.discover-section').style.display = 'block'; - - } catch (error) { - console.error('Error loading top tracks:', error); - } -} - -async function loadPersonalizedForgottenFavorites() { - try { - const container = document.getElementById('personalized-forgotten-favorites'); - if (!container) return; - - const response = await fetch('/api/discover/personalized/forgotten-favorites'); - if (!response.ok) return; - - const data = await response.json(); - if (!data.success || !data.tracks || data.tracks.length === 0) { - container.closest('.discover-section').style.display = 'none'; - return; - } - - personalizedForgottenFavorites = data.tracks; - renderCompactPlaylist(container, data.tracks); - container.closest('.discover-section').style.display = 'block'; - - } catch (error) { - console.error('Error loading forgotten favorites:', error); - } -} - async function loadPersonalizedPopularPicks() { try { const container = document.getElementById('personalized-popular-picks'); @@ -6612,29 +6535,6 @@ async function loadDiscoveryShuffle() { } } -async function loadFamiliarFavorites() { - try { - const container = document.getElementById('personalized-familiar-favorites'); - if (!container) return; - - const response = await fetch('/api/discover/personalized/familiar-favorites?limit=50'); - if (!response.ok) return; - - const data = await response.json(); - if (!data.success || !data.tracks || data.tracks.length === 0) { - container.closest('.discover-section').style.display = 'none'; - return; - } - - personalizedFamiliarFavorites = data.tracks; - renderCompactPlaylist(container, data.tracks); - container.closest('.discover-section').style.display = 'block'; - - } catch (error) { - console.error('Error loading familiar favorites:', error); - } -} - // =============================== // BECAUSE YOU LISTEN TO // =============================== @@ -7389,14 +7289,6 @@ async function openDownloadModalForDiscoverPlaylist(playlistType, playlistName) tracks = personalizedHiddenGems; } else if (playlistType === 'discovery_shuffle') { tracks = personalizedDiscoveryShuffle; - } else if (playlistType === 'familiar_favorites') { - tracks = personalizedFamiliarFavorites; - } else if (playlistType === 'recently_added') { - tracks = personalizedRecentlyAdded; - } else if (playlistType === 'top_tracks') { - tracks = personalizedTopTracks; - } else if (playlistType === 'forgotten_favorites') { - tracks = personalizedForgottenFavorites; } else if (playlistType === 'build_playlist') { tracks = buildPlaylistTracks; } @@ -7516,8 +7408,6 @@ async function startDiscoverPlaylistSync(playlistType, playlistName) { tracks = personalizedHiddenGems; } else if (playlistType === 'discovery_shuffle') { tracks = personalizedDiscoveryShuffle; - } else if (playlistType === 'familiar_favorites') { - tracks = personalizedFamiliarFavorites; } else if (playlistType === 'build_playlist') { tracks = buildPlaylistTracks; } @@ -7648,7 +7538,7 @@ function startDiscoverSyncPolling(playlistType, virtualPlaylistId) { 'release_radar': 'Fresh Tape', 'discovery_weekly': 'The Archives', 'seasonal_playlist': 'Seasonal Mix', 'popular_picks': 'Popular Picks', 'hidden_gems': 'Hidden Gems', 'discovery_shuffle': 'Discovery Shuffle', - 'familiar_favorites': 'Familiar Favorites', 'build_playlist': 'Custom Playlist' + 'build_playlist': 'Custom Playlist' }; showToast(`${playlistNames[playlistType] || playlistType} sync complete!`, 'success'); setTimeout(() => { const sd = el(`${prefix}-sync-status`); if (sd) sd.style.display = 'none'; }, 3000); @@ -7714,7 +7604,6 @@ function startDiscoverSyncPolling(playlistType, virtualPlaylistId) { 'popular_picks': 'Popular Picks', 'hidden_gems': 'Hidden Gems', 'discovery_shuffle': 'Discovery Shuffle', - 'familiar_favorites': 'Familiar Favorites', 'build_playlist': 'Custom Playlist' }; const displayName = playlistNames[playlistType] || playlistType; @@ -8239,8 +8128,6 @@ async function rehydrateDiscoverDownloadModal(playlistId) { apiEndpoint = '/api/discover/hidden-gems'; } else if (playlistId === 'discover_discovery_shuffle') { apiEndpoint = '/api/discover/discovery-shuffle'; - } else if (playlistId === 'discover_familiar_favorites') { - apiEndpoint = '/api/discover/familiar-favorites'; } else if (playlistId === 'build_playlist_custom') { apiEndpoint = '/api/discover/build-playlist'; } else if (playlistId.startsWith('discover_lb_')) { diff --git a/webui/static/helper.js b/webui/static/helper.js index a8d204f0..e0cdcf20 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -1056,10 +1056,6 @@ const HELPER_CONTENT = { }, // Personalized Playlists - '#personalized-recently-added': { - title: 'Recently Added', - description: 'The latest tracks added to your library. A quick way to see what\'s new in your collection.', - }, '#personalized-popular-picks': { title: 'Popular Picks', description: 'Trending tracks from your discovery pool artists. These are the most popular songs from artists similar to the ones you follow.', @@ -1071,23 +1067,11 @@ const HELPER_CONTENT = { description: 'Rare and deeper cuts from your discovery pool artists. Lower popularity tracks that you might not find on mainstream playlists.', docsId: 'disc-playlists' }, - '#personalized-top-tracks': { - title: 'Your Top 50', - description: 'Your all-time most played tracks from listening history. A snapshot of your personal favorites.', - }, - '#personalized-forgotten-favorites': { - title: 'Forgotten Favorites', - description: 'Tracks you used to play frequently but haven\'t listened to in a while. Rediscover music you loved.', - }, '#personalized-discovery-shuffle': { title: 'Discovery Shuffle', description: 'Random tracks from your entire discovery pool — different every time you load. A surprise mix for when you want something new.', docsId: 'disc-playlists' }, - '#personalized-familiar-favorites': { - title: 'Familiar Favorites', - description: 'Your reliable go-to tracks. Consistently played songs that define your taste.', - }, // Curated Playlists '#release-radar-playlist': { @@ -3432,7 +3416,7 @@ const WHATS_NEW = { '2.4.3': [ // --- post-2.4.2 dev work — entries hidden by _getLatestWhatsNewVersion until the build version bumps --- { date: 'Unreleased — 2.4.3 dev cycle' }, - { title: 'Discover: Stop Showing Undownloadable Tracks (+Lift)', desc: 'audit found multiple discover-page sections (hidden gems / discovery shuffle / popular picks / decade browser / genre browser) had no `WHERE (spotify_track_id IS NOT NULL OR itunes_track_id IS NOT NULL OR ...)` gate on their selection sql. tracks with no source ids in the discovery pool were getting displayed, the user would click download, and the download would silently fail because there was nothing to look up. fix: lifted all five discovery_pool selection methods + four library-driven methods (recently added / top tracks / forgotten favorites / familiar favorites — these were stubbed returning [] because the schema predated the source-id columns; now re-enabled properly) into shared private helpers (`_select_discovery_tracks`, `_select_library_tracks`, `_apply_diversity_filter`, `_compute_adaptive_diversity_limits`) on `PersonalizedPlaylistsService`. mandatory id-validity gate is hard-coded into both selectors — no opt-out flag, every public method inherits it for free. behavior preserved: same diversity tiers, same over-fetch multipliers, same popularity thresholds, same blacklist filter. ~314 lines of repeated select/diversity boilerplate collapsed across the 5 discovery methods (-55% on those methods\' business logic). on the frontend, lifted the duplicated decade-browser + genre-browser tab management (~314 lines of identical fetch-tabs / render-tabstrip / fetch-content / render-tracklist / wire-sync-button code) into one shared `createTabbedBrowserSection(config)` helper. each browser is now a thin wrapper: ~3 lines per public function. 17 new tests pin the gate (every selector filters null-id rows), the diversity caps, the adaptive limit tiers, the source filter, and the blacklist filter. 2222/2222 full suite green.', page: 'discover' }, + { title: 'Discover: Stop Showing Undownloadable Tracks (+Lift +Cleanup)', desc: 'audit found multiple discover-page sections (hidden gems / discovery shuffle / popular picks / decade browser / genre browser) had no `WHERE (spotify_track_id IS NOT NULL OR itunes_track_id IS NOT NULL OR ...)` gate on their selection sql. tracks with no source ids in the discovery pool were getting displayed, the user would click download, and the download would silently fail because there was nothing to look up. fix: lifted all five discovery_pool selection methods into shared private helpers (`_select_discovery_tracks`, `_apply_diversity_filter`, `_compute_adaptive_diversity_limits`) on `PersonalizedPlaylistsService`. mandatory id-validity gate is hard-coded into the selector — no opt-out flag, every public method inherits it for free. behavior preserved: same diversity tiers, same over-fetch multipliers, same popularity thresholds, same blacklist filter. ~314 lines of repeated select/diversity boilerplate collapsed across the 5 methods (-55% on those methods\' business logic). also deleted four sections that had been stubbed returning [] for ages (recently added / top tracks / forgotten favorites / familiar favorites) — frontend, backend endpoints, html blocks, helper docs, all gone. on the frontend, lifted the duplicated decade-browser + genre-browser tab management (~314 lines of identical fetch-tabs / render-tabstrip / fetch-content / render-tracklist / wire-sync-button code) into one shared `createTabbedBrowserSection(config)` helper. each browser is now a thin wrapper: ~3 lines per public function. 14 new tests pin the gate (every selector filters null-id rows), the diversity caps, the adaptive limit tiers, the source filter, and the blacklist filter.', page: 'discover' }, { title: 'Internal: Discover Controller — Cin Pre-Review Polish', desc: 'tightened the controller before opening the PR. (1) dropped the magic `extractItems` defaults — controller used to auto-pull `data.items` / `data.albums` / `data.artists` / `data.tracks` / `data.results` if no extractor was provided. removed the fallback chain. each section now MUST supply its own `extractItems(data) => array` callback. cin standard: explicit > implicit; the auto-fallback could silently grab the wrong key on endpoints that return multiple arrays. validated at register-time so misuse fails immediately. all 10 existing call sites already had explicit extractors so no migration churn. (2) replaced the `renderItems` returning null convention (used by Your Albums + manualDom-style sections) with an explicit `manualDom: true` config flag. clearer intent at the call site, less likely to be confused with a renderer error. (3) added a minimal node `--test` JS test file at `tests/static/test_discover_section_controller.mjs` — 32 tests pin the lifecycle contract: config validation (every required field), happy-path fetch+render, empty/stale/error states, no-fetch `data:` mode, manualDom mode, callable `fetchUrl`, load coalescing, refresh bypass, hook error containment, error toasts. runs via `node --test tests/static/` directly, OR via the regular pytest sweep (`tests/test_discover_section_controller_js.py` shells out to node and asserts a clean exit). skipped gracefully when node isn\'t available or is < 22. closes the "controller is a contract, pin it at the test boundary" gap that cin would have flagged. 2205/2205 full suite green (was 2204 + 1 new pytest wrapper); 32/32 node --test pass; ruff clean; js parses clean.', page: 'discover' }, { title: 'Internal: Discover Cleanup Round — Toast Errors, Stale State, Skipped Sections', desc: 'follow-up to the controller migration. extended `createDiscoverSectionController` with the hooks the per-section migrations surfaced as needed: callable `fetchUrl` (resolves the seasonal-playlist recreate-on-key-change hack), no-fetch `data:` mode (lets render-only sections like seasonal albums use the controller without inventing a fake endpoint), `beforeLoad` hook (lets dynamically-inserted sections like because-you-listen-to ensure their container exists before the spinner shows), `onSuccess(data)` hook (cleaner home for sibling header / subtitle / button updates than folding them into renderItems), and an `isStale` / `onStale` / `renderStale` triple for the third render state (data is empty BUT upstream is still discovering — show updating UI + start a poller, instead of the bare empty-state copy). turned on `showErrorToast: true` for every migrated section — section load failures now surface a global toast instead of silently spinning forever or swallowing into console.debug. that\'s the JohnBaumb #369 pattern applied at the UI layer. migrated the two sections that didn\'t fit the original controller contract: `loadYourAlbums` (uses isStale/onStale for stale-fetch UI + onSuccess for subtitle/filters/download-button side-effects + renderItems returning null since it delegates to the existing grid renderer) and `loadSeasonalAlbums` (uses no-fetch data mode since the parent `loadSeasonalContent` already fetched the season payload). also lifted the duplicated decade-tab + genre-tab sync-status block (✓/⏳/✗/percentage) into a `_renderSyncStatusBlock(idPrefix)` helper — two call sites now share one implementation. listenbrainz playlists keep their own block because the semantics differ (matching progress vs download progress). audit found the 13 supposedly-dead hidden sections aren\'t dead at all — they\'re gated on user data (discovery pool, library content, metadata cache) and self-surface when their data exists. removed one orphaned `loadPersonalizedDailyMixes()` call from `blockDiscoveryArtist` — daily mixes is intentionally paused, refreshing it from there was a no-op.', page: 'discover' }, { title: 'Internal: Migrate 7 More Discover Sections to the Controller', desc: 'follow-up to the foundation commit. migrated fresh tape, the archives, time machine intro carousel, browse by genre intro carousel, seasonal mix, your artists, and because-you-listen-to onto `createDiscoverSectionController`. each one drops its own hand-rolled try/catch + spinner injection + empty-state HTML + error swallow in favor of a config object — controller owns the lifecycle. net 76 lines smaller in discover.js even after adding the per-section render helpers. skipped two sections that don\'t fit the controller\'s single-fetch / single-render-target shape: `loadYourAlbums` (paginated grid + filters, four separate UI elements updated) and `loadSeasonalAlbums` (no fetch — receives pre-fetched data from parent). hidden / dead sections (~13 of them) untouched in this pass — separate audit commit will surface or kill them. controller extension candidates surfaced for follow-up: callable `fetchUrl` (so seasonal playlist doesn\'t need controller-recreate-on-key-change), explicit `isStale` / `onStale` hook (so your-artists doesn\'t fold stale handling into renderItems), `beforeLoad` hook (so because-you-listen-to can let the controller own the dynamic container creation), and a no-fetch `data:` mode (so render-only sections like seasonal albums can use the controller). zero behavior changes — every public load function keeps its name + signature so existing callers, refresh buttons, and dashboard wiring don\'t notice the swap.', page: 'discover' }, diff --git a/webui/static/sync-spotify.js b/webui/static/sync-spotify.js index 95e592c2..796d1ce5 100644 --- a/webui/static/sync-spotify.js +++ b/webui/static/sync-spotify.js @@ -293,8 +293,6 @@ async function rehydrateDiscoverPlaylistModal(virtualPlaylistId, playlistName, b apiEndpoint = '/api/discover/hidden-gems'; } else if (virtualPlaylistId === 'discover_discovery_shuffle') { apiEndpoint = '/api/discover/discovery-shuffle'; - } else if (virtualPlaylistId === 'discover_familiar_favorites') { - apiEndpoint = '/api/discover/familiar-favorites'; } else if (virtualPlaylistId === 'build_playlist_custom') { apiEndpoint = '/api/discover/build-playlist'; } else if (virtualPlaylistId.startsWith('discover_lb_')) { From d123581a39e1e5f9ffac8b5c13b361f315e8b31d Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Fri, 8 May 2026 07:46:09 -0700 Subject: [PATCH 4/5] Fix: ID gate missed Deezer-track-id-only rows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The original gate baked into `_select_discovery_tracks` only checked Spotify + iTunes: AND (spotify_track_id IS NOT NULL OR itunes_track_id IS NOT NULL) For Deezer-primary users, discovery_pool rows have populated `deezer_track_id` but NULL Spotify + NULL iTunes IDs. The gate filtered every row out — Time Machine, Genre Browser, Hidden Gems, Discovery Shuffle, Popular Picks all rendered "no tracks found" for every tab on every Deezer-primary install. Extended the gate to include `deezer_track_id` and added that column to the standard SELECT column tuple. `_build_track_dict` already exposed `deezer_track_id` in its output shape, so frontend rendering needed no changes. Regression pinned via new test `test_discovery_helper_accepts_deezer_only_id_rows` — inserts a row with NULL Spotify + NULL iTunes but a populated `deezer_track_id` and asserts it survives the gate. 2220/2220 full suite green. --- core/personalized_playlists.py | 5 +++-- tests/test_personalized_playlists_id_gate.py | 23 ++++++++++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/core/personalized_playlists.py b/core/personalized_playlists.py index da507121..f6ad3bd8 100644 --- a/core/personalized_playlists.py +++ b/core/personalized_playlists.py @@ -111,6 +111,7 @@ class PersonalizedPlaylistsService: _STANDARD_DISCOVERY_COLUMNS: Tuple[str, ...] = ( 'spotify_track_id', 'itunes_track_id', + 'deezer_track_id', 'track_name', 'artist_name', 'album_name', @@ -141,7 +142,7 @@ class PersonalizedPlaylistsService: The WHERE clause always includes: source = ? - AND (spotify_track_id IS NOT NULL OR itunes_track_id IS NOT NULL) + AND (spotify_track_id IS NOT NULL OR itunes_track_id IS NOT NULL OR deezer_track_id IS NOT NULL) AND LOWER(artist_name) NOT IN (SELECT LOWER(artist_name) FROM discovery_artist_blacklist) @@ -181,7 +182,7 @@ class PersonalizedPlaylistsService: {select_cols} FROM discovery_pool WHERE source = ? - AND (spotify_track_id IS NOT NULL OR itunes_track_id IS NOT NULL) + AND (spotify_track_id IS NOT NULL OR itunes_track_id IS NOT NULL OR deezer_track_id IS NOT NULL) AND LOWER(artist_name) NOT IN (SELECT LOWER(artist_name) FROM discovery_artist_blacklist) {extra_where} ORDER BY {order_by} diff --git a/tests/test_personalized_playlists_id_gate.py b/tests/test_personalized_playlists_id_gate.py index 3f205982..568aa411 100644 --- a/tests/test_personalized_playlists_id_gate.py +++ b/tests/test_personalized_playlists_id_gate.py @@ -135,6 +135,29 @@ def test_discovery_helper_filters_null_id_rows(service): assert names == ['Has Spotify ID', 'Has iTunes ID'] +def test_discovery_helper_accepts_deezer_only_id_rows(service): + """Discovery pool rows with NULL spotify + NULL itunes but a populated + deezer_track_id MUST pass the gate. Regression test — early version + of the gate only checked Spotify + iTunes, which silently filtered + out every row for Deezer-primary users (entire Time Machine / + Genre / Hidden Gems / Shuffle / Popular Picks rendered empty).""" + svc, db = service + db.insert_discovery_track( + source='deezer', deezer_track_id='dz1', + spotify_track_id=None, itunes_track_id=None, + track_name='Deezer Only', artist_name='A', album_name='X', + popularity=50, release_date='2024-01-01', + ) + with patch.object(svc, '_get_active_source', return_value='deezer'): + tracks = svc._select_discovery_tracks( + source='deezer', + order_by='track_name', + fetch_limit=100, + ) + assert [t['track_name'] for t in tracks] == ['Deezer Only'] + assert tracks[0]['deezer_track_id'] == 'dz1' + + def test_discovery_helper_filters_blacklisted_artists(service): svc, db = service db.insert_discovery_track( From d75ae48981b2deee17556f7d2bb8e53866e5abf1 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Fri, 8 May 2026 08:49:22 -0700 Subject: [PATCH 5/5] Discover: sharpen track selection (diversity, source-aware popularity, library dedup, SQL genre) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four selection-quality fixes on the SoulSync-made discover playlists. None change public method signatures; all are tightenings on what's already there. (1) Diversity for Hidden Gems + Discovery Shuffle Both used to be `RANDOM() LIMIT N` with no diversity. Could return 50 tracks from one artist or 20 from one album if the discovery pool happened to be skewed. Both now over-fetch 3x and run the existing `_apply_diversity_filter`: - Hidden Gems: max 2 per album, 3 per artist - Discovery Shuffle: max 2 per album, 2 per artist (tighter — shuffle should feel maximally varied) (2) Source-aware popularity thresholds `popularity >= 60` for "Popular Picks" and `popularity < 40` for "Hidden Gems" was Spotify-shaped (0-100 scale). Deezer writes its `rank` value into that column (often six-digit integers); iTunes writes nothing meaningful. For Deezer-primary users: - Popular Picks pulled essentially everything (rank >= 60 = all) - Hidden Gems pulled essentially nothing (rank < 40 = none) New `_get_popularity_thresholds(source)` helper returns per-source values: - Spotify: (60, 40) — the existing 0-100 scale - Deezer: (500_000, 100_000) — ballpark from real rank values - iTunes / unknown: (None, None) — skip the popularity filter entirely, fall back to random + diversity `get_popular_picks` and `get_hidden_gems` now consult the helper. When threshold is None they skip the popularity SQL filter. Diversity + ID gate still apply. (3) Push genre keyword filter into SQL `get_genre_playlist` used to fetch `limit=1_000_000` rows into Python then run a substring keyword filter on `artist_genres`. Bad on big discovery pools. Now the keyword OR chain is generated as SQL placeholders: AND (artist_genres LIKE ? OR artist_genres LIKE ? OR ...) Each placeholder gets `f'%{keyword.lower()}%'` via `extra_params`. `fetch_limit` drops back to `limit * 10`. `_genre_matches` Python helper deleted (only intra-file caller; verified via grep). Parent-genre expansion via `GENRE_MAPPING` preserved — keywords list feeds the LIKE chain unchanged. (4) Filter out tracks already in library Discovery pool can include tracks the user already owns. Hidden Gems / Shuffle / Popular Picks shouldn't surface those. `_select_discovery_tracks` gained `exclude_owned: bool = True` parameter. When True, adds a correlated NOT EXISTS subquery against the `tracks` table covering all 3 source IDs: AND NOT EXISTS ( SELECT 1 FROM tracks t WHERE (t.spotify_track_id IS NOT NULL AND t.spotify_track_id = discovery_pool.spotify_track_id) OR (t.itunes_track_id IS NOT NULL AND t.itunes_track_id = discovery_pool.itunes_track_id) OR (t.deezer_id IS NOT NULL AND t.deezer_id = discovery_pool.deezer_track_id) ) Note column-name asymmetry: tracks.deezer_id vs discovery_pool.deezer_track_id. Inline comment marks the trap. All 5 public discovery methods automatically benefit (default True). Seasonal Playlist doesn't go through the helper so it's unaffected (curated content, dedup is wrong intent there). Tests 12 new tests in `tests/test_personalized_playlists_id_gate.py` (27 total in the file): - Hidden Gems + Discovery Shuffle apply diversity (cap proven by inserting 10 same-artist + same-album rows and asserting return count ≤ per-album cap) - Popularity thresholds: Spotify (60, 40), Deezer larger scale, iTunes None / None - Popular Picks skips threshold filter when None - Genre playlist pushes filter to SQL (parent + child genre expansion) - Owned-track exclusion: filtered when match, kept when no match, opt-out flag works - Deezer column-name asymmetry pinned (regression footgun) Test fixture re-added the minimal `tracks` table (4 columns: id, spotify_track_id, itunes_track_id, deezer_id) — only what the new NOT EXISTS subquery needs to join. Plus `insert_library_track` helper. Verification - 27/27 in this test file pass (15 prior + 12 new) - 2232/2232 full suite green - ruff clean LOC delta: - core/personalized_playlists.py: 1030 → 1101 (+71) - tests/test_personalized_playlists_id_gate.py: 352 → 616 (+264) --- core/personalized_playlists.py | 183 +++++++++---- tests/test_personalized_playlists_id_gate.py | 264 +++++++++++++++++++ webui/static/helper.js | 1 + 3 files changed, 392 insertions(+), 56 deletions(-) diff --git a/core/personalized_playlists.py b/core/personalized_playlists.py index f6ad3bd8..64882b97 100644 --- a/core/personalized_playlists.py +++ b/core/personalized_playlists.py @@ -105,6 +105,26 @@ class PersonalizedPlaylistsService: from core.metadata_service import get_primary_source return get_primary_source() + def _get_popularity_thresholds(self, source: str) -> Tuple[Optional[int], Optional[int]]: + """Return (popular_min, hidden_max) thresholds for the given source. + + Either value can be None to skip that filter for that source. + + - Spotify: 60 / 40 (the existing 0-100 popularity scale) + - Deezer: 500000 / 100000 (rank values — ballpark from real data) + - iTunes / others: None / None (no popularity data; fall back to no + threshold filter, just diversity-on-random) + + Sources are normalized lowercase so 'Spotify'/'spotify' both match. + """ + normalized = (source or '').lower() + if normalized == 'spotify': + return 60, 40 + if normalized == 'deezer': + return 500_000, 100_000 + # iTunes, hydrabase, anything else — no usable popularity data + return None, None + # Standard column set returned by every discovery_pool selector. # Callers can request additional columns via the `extra_columns` parameter # of `_select_discovery_tracks` (e.g. `release_date`, `artist_genres`). @@ -131,6 +151,7 @@ class PersonalizedPlaylistsService: order_by: str = "RANDOM()", fetch_limit: int, extra_columns: tuple = (), + exclude_owned: bool = True, ) -> List[Dict]: """ Shared selector for discovery_pool playlist methods. @@ -146,6 +167,13 @@ class PersonalizedPlaylistsService: AND LOWER(artist_name) NOT IN (SELECT LOWER(artist_name) FROM discovery_artist_blacklist) + When `exclude_owned=True` (default) the WHERE additionally excludes + any discovery_pool row whose IDs already match a row in the local + `tracks` table — i.e. tracks the user already has in their library. + Without this filter, Discovery / Hidden Gems / Popular Picks etc. + would happily surface tracks the user owns. Note the column-name + asymmetry: `tracks.deezer_id` ↔ `discovery_pool.deezer_track_id`. + The ID gate is mandatory and not opt-out by design — if a future method needs to skip it, that's a design discussion, not a flag. @@ -168,6 +196,8 @@ class PersonalizedPlaylistsService: run a diversity filter should over-fetch (e.g. `limit * 3`). extra_columns: additional columns to SELECT beyond `_STANDARD_DISCOVERY_COLUMNS` (e.g. `('release_date',)`). + exclude_owned: when True (default), filter out discovery rows + whose IDs match any row in the local `tracks` table. Returns: List of track dicts via `_build_track_dict`. Returns `[]` on any @@ -177,6 +207,18 @@ class PersonalizedPlaylistsService: columns = self._STANDARD_DISCOVERY_COLUMNS + tuple(extra_columns) select_cols = ",\n ".join(columns) + owned_clause = "" + if exclude_owned: + # Note column-name asymmetry: discovery_pool.deezer_track_id + # but tracks.deezer_id. Don't refactor without checking. + owned_clause = """ + AND NOT EXISTS ( + SELECT 1 FROM tracks t + WHERE (t.spotify_track_id IS NOT NULL AND t.spotify_track_id = discovery_pool.spotify_track_id) + OR (t.itunes_track_id IS NOT NULL AND t.itunes_track_id = discovery_pool.itunes_track_id) + OR (t.deezer_id IS NOT NULL AND t.deezer_id = discovery_pool.deezer_track_id) + )""" + query = f""" SELECT {select_cols} @@ -184,6 +226,7 @@ class PersonalizedPlaylistsService: WHERE source = ? AND (spotify_track_id IS NOT NULL OR itunes_track_id IS NOT NULL OR deezer_track_id IS NOT NULL) AND LOWER(artist_name) NOT IN (SELECT LOWER(artist_name) FROM discovery_artist_blacklist) + {owned_clause} {extra_where} ORDER BY {order_by} LIMIT ? @@ -453,29 +496,16 @@ class PersonalizedPlaylistsService: logger.error(f"Error getting available genres: {e}") return [] - def _genre_matches(self, artist_genres_json: Optional[str], search_keywords: List[str]) -> bool: - """Return True if any artist genre in the JSON-encoded column matches any keyword.""" - if not artist_genres_json: - return False - try: - genres = json.loads(artist_genres_json) - except Exception: - return False - for artist_genre in genres: - artist_genre_lower = artist_genre.lower() - for keyword in search_keywords: - if keyword in artist_genre_lower: - return True - return False - def get_genre_playlist(self, genre: str, limit: int = 50, source: str = None) -> List[Dict]: """ Get tracks from a specific genre with diversity filtering. Uses cached artist genres from database (populated during discovery scan). Supports both parent genres (e.g., "Electronic/Dance") and specific genres (e.g., "house"). - The genre keyword match runs Python-side over the JSON-encoded artist_genres - column, so this method overfetches via the shared selector then filters. + The keyword match is pushed into SQL as an OR-chain of LIKE clauses + on `artist_genres`, so we no longer have to over-fetch the entire + pool to filter Python-side. SQLite's LIKE is case-insensitive for + ASCII, matching the previous case-insensitive Python comparison. """ active_source = source or self._get_active_source() @@ -488,38 +518,27 @@ class PersonalizedPlaylistsService: search_keywords = [genre.lower()] logger.info(f"Matching specific genre '{genre}' with partial matching") - # Pull every source row with non-null artist_genres through the shared - # selector (gets the ID gate + blacklist filter for free). Cap at a high - # bound — the Python keyword filter narrows the result drastically. - candidate_tracks = self._select_discovery_tracks( + # Build the SQL keyword OR-chain. One LIKE per keyword, all OR'd + # together inside a single parenthesized clause so the surrounding + # AND structure isn't broken. + like_clauses = " OR ".join(["artist_genres LIKE ?"] * len(search_keywords)) + like_params = tuple(f"%{k}%" for k in search_keywords) + + # Over-fetch 10x for diversity filter headroom — the SQL filter + # has already narrowed to genre matches, so this is bounded. + all_tracks = self._select_discovery_tracks( source=active_source, - extra_where="AND artist_genres IS NOT NULL", + extra_where=f"AND artist_genres IS NOT NULL AND ({like_clauses})", + extra_params=like_params, order_by="RANDOM()", - fetch_limit=1_000_000, + fetch_limit=limit * 10, extra_columns=('artist_genres',), ) - if not candidate_tracks: - logger.warning(f"No tracks with genre data found for source: {active_source}") - return [] - - # `_build_track_dict` stashes the raw `artist_genres` column under - # `_artist_genres_raw` (since we requested it via `extra_columns`), - # so the keyword match can run without re-querying. - matching_tracks = [ - track for track in candidate_tracks - if self._genre_matches(track.get('_artist_genres_raw'), search_keywords) - ] - - if not matching_tracks: + if not all_tracks: logger.warning(f"No tracks found for genre: {genre}") return [] - random.shuffle(matching_tracks) - - # Cap candidate set at 10x limit for diversity filtering headroom. - all_tracks = matching_tracks[:limit * 10] if len(matching_tracks) > limit * 10 else matching_tracks - max_per_album, max_per_artist = self._compute_adaptive_diversity_limits(all_tracks, relaxed=True) unique_artists = len(set(t['artist_name'] for t in all_tracks)) logger.info( @@ -527,7 +546,6 @@ class PersonalizedPlaylistsService: f"limits: {max_per_album}/album, {max_per_artist}/artist" ) - random.shuffle(all_tracks) diverse_tracks = self._apply_diversity_filter( all_tracks, max_per_album=max_per_album, @@ -543,14 +561,30 @@ class PersonalizedPlaylistsService: # ======================================== def get_popular_picks(self, limit: int = 50) -> List[Dict]: - """Get high popularity tracks from discovery pool with diversity (max 2 per album, 3 per artist).""" + """Get high popularity tracks from discovery pool with diversity (max 2 per album, 3 per artist). + + Popularity threshold is source-aware via `_get_popularity_thresholds` + — sources without a usable popularity scale (iTunes / Hydrabase) + skip the threshold filter and fall back to RANDOM + diversity only. + """ active_source = self._get_active_source() + popular_min, _ = self._get_popularity_thresholds(active_source) + + if popular_min is not None: + extra_where = "AND popularity >= ?" + extra_params: tuple = (popular_min,) + order_by = "popularity DESC, RANDOM()" + else: + extra_where = "" + extra_params = () + order_by = "RANDOM()" # Over-fetch 3x so the diversity filter has room to spread albums/artists. all_tracks = self._select_discovery_tracks( source=active_source, - extra_where="AND popularity >= 60", - order_by="popularity DESC, RANDOM()", + extra_where=extra_where, + extra_params=extra_params, + order_by=order_by, fetch_limit=limit * 3, ) @@ -565,31 +599,68 @@ class PersonalizedPlaylistsService: return diverse_tracks def get_hidden_gems(self, limit: int = 50) -> List[Dict]: - """Get low-popularity (underground/indie) tracks from discovery pool.""" + """Get low-popularity (underground/indie) tracks from discovery pool with diversity. + + Mirrors Popular Picks' diversity caps (2 per album, 3 per artist) + since both are curated-feel sections. Popularity threshold is + source-aware — iTunes/Hydrabase fall back to RANDOM + diversity + only. + """ active_source = self._get_active_source() - tracks = self._select_discovery_tracks( + _, hidden_max = self._get_popularity_thresholds(active_source) + + if hidden_max is not None: + extra_where = "AND popularity < ?" + extra_params: tuple = (hidden_max,) + else: + extra_where = "" + extra_params = () + + # Over-fetch 3x for diversity filter headroom. + all_tracks = self._select_discovery_tracks( source=active_source, - extra_where="AND popularity < 40", + extra_where=extra_where, + extra_params=extra_params, order_by="RANDOM()", - fetch_limit=limit, + fetch_limit=limit * 3, ) - logger.info(f"Hidden Gems ({active_source}): selected {len(tracks)} tracks") - return tracks + + diverse_tracks = self._apply_diversity_filter( + all_tracks, + max_per_album=2, + max_per_artist=3, + limit=limit, + ) + + logger.info(f"Hidden Gems ({active_source}): selected {len(diverse_tracks)} tracks with diversity") + return diverse_tracks def get_discovery_shuffle(self, limit: int = 50) -> List[Dict]: """ Get random tracks from discovery pool - pure exploration. - Different every time you call it! + Different every time you call it! Tighter diversity than Popular + Picks / Hidden Gems (2 per album, 2 per artist) since shuffle + should feel maximally varied. """ active_source = self._get_active_source() - tracks = self._select_discovery_tracks( + + # Over-fetch 3x for diversity filter headroom. + all_tracks = self._select_discovery_tracks( source=active_source, order_by="RANDOM()", - fetch_limit=limit, + fetch_limit=limit * 3, ) - logger.info(f"Discovery Shuffle ({active_source}): selected {len(tracks)} tracks") - return tracks + + diverse_tracks = self._apply_diversity_filter( + all_tracks, + max_per_album=2, + max_per_artist=2, + limit=limit, + ) + + logger.info(f"Discovery Shuffle ({active_source}): selected {len(diverse_tracks)} tracks with diversity") + return diverse_tracks # ======================================== # DAILY MIX (HYBRID PLAYLISTS) diff --git a/tests/test_personalized_playlists_id_gate.py b/tests/test_personalized_playlists_id_gate.py index 568aa411..365051d3 100644 --- a/tests/test_personalized_playlists_id_gate.py +++ b/tests/test_personalized_playlists_id_gate.py @@ -68,6 +68,16 @@ class _FakeDatabase: CREATE TABLE discovery_artist_blacklist ( artist_name TEXT PRIMARY KEY ); + -- Minimal `tracks` table: exists so the `exclude_owned` + -- subquery in `_select_discovery_tracks` can join. Real + -- schema has many more columns; we only need the source-id + -- columns it actually inspects. + CREATE TABLE tracks ( + id INTEGER PRIMARY KEY, + spotify_track_id TEXT, + itunes_track_id TEXT, + deezer_id TEXT + ); """) self._conn.commit() @@ -95,6 +105,18 @@ class _FakeDatabase: ) self._conn.commit() + def insert_library_track(self, **kwargs): + """Insert a row into the local `tracks` table (the user's library). + Used to prove `exclude_owned=True` filters discovery rows whose IDs + match a library row.""" + cols = ", ".join(kwargs.keys()) + placeholders = ", ".join(["?"] * len(kwargs)) + self._conn.execute( + f"INSERT INTO tracks ({cols}) VALUES ({placeholders})", + tuple(kwargs.values()), + ) + self._conn.commit() + @pytest.fixture def service(): @@ -350,3 +372,245 @@ def test_get_decade_playlist_filters_null_id_rows(service): assert [t['track_name'] for t in out] == ['Yes'] +# --------------------------------------------------------------------------- +# Fix #1 — Hidden Gems + Discovery Shuffle apply diversity +# --------------------------------------------------------------------------- + + +def test_get_hidden_gems_applies_diversity(service): + """10 low-popularity tracks all from the same album/artist should be + capped at the per-album limit (2) — Hidden Gems no longer returns + raw RANDOM() rows.""" + svc, db = service + for i in range(10): + db.insert_discovery_track( + source='spotify', spotify_track_id=f'sp{i}', + track_name=f't{i}', artist_name='SoloArtist', + album_name='OnlyAlbum', popularity=20, + ) + out = svc.get_hidden_gems(limit=10) + # All 10 share the same album → diversity cap of 2 per album wins. + assert len(out) == 2 + assert all(t['album_name'] == 'OnlyAlbum' for t in out) + + +def test_get_discovery_shuffle_applies_diversity(service): + """Same idea for shuffle, with tighter caps (2 per artist).""" + svc, db = service + for i in range(10): + db.insert_discovery_track( + source='spotify', spotify_track_id=f'sp{i}', + track_name=f't{i}', artist_name='SoloArtist', + album_name=f'Album{i}', # different albums so artist cap bites + popularity=50, + ) + out = svc.get_discovery_shuffle(limit=10) + # Same artist → per-artist cap of 2 wins. + assert len(out) == 2 + assert all(t['artist_name'] == 'SoloArtist' for t in out) + + +# --------------------------------------------------------------------------- +# Fix #2 — Source-aware popularity thresholds +# --------------------------------------------------------------------------- + + +def test_popularity_thresholds_spotify_returns_60_40(): + svc = PersonalizedPlaylistsService(_FakeDatabase()) + popular_min, hidden_max = svc._get_popularity_thresholds('spotify') + assert popular_min == 60 + assert hidden_max == 40 + + +def test_popularity_thresholds_deezer_returns_higher_scale(): + """Deezer writes `rank` (raw integer, often 100k+) into the popularity + column, so thresholds must be in that range — Spotify's 60/40 would + classify almost every Deezer track as a hidden gem.""" + svc = PersonalizedPlaylistsService(_FakeDatabase()) + popular_min, hidden_max = svc._get_popularity_thresholds('deezer') + assert popular_min is not None and popular_min >= 100_000 + assert hidden_max is not None and hidden_max >= 50_000 + assert popular_min > hidden_max + + +def test_popularity_thresholds_itunes_skips_filter(): + """iTunes has no usable popularity data — both thresholds should be + None so callers fall back to RANDOM + diversity only.""" + svc = PersonalizedPlaylistsService(_FakeDatabase()) + popular_min, hidden_max = svc._get_popularity_thresholds('itunes') + assert popular_min is None + assert hidden_max is None + + +def test_get_popular_picks_skips_threshold_when_none(): + """When the active source has no popularity data, Popular Picks + should skip the popularity filter entirely (just diversity + ID + gate). Insert rows with a mix of popularity values; with the iTunes + source they should ALL pass the popularity gate.""" + db = _FakeDatabase() + svc = PersonalizedPlaylistsService(db) + db.insert_discovery_track( + source='itunes', itunes_track_id='it1', track_name='Low', + artist_name='A', album_name='Album1', popularity=5, + ) + db.insert_discovery_track( + source='itunes', itunes_track_id='it2', track_name='High', + artist_name='B', album_name='Album2', popularity=95, + ) + db.insert_discovery_track( + source='itunes', itunes_track_id='it3', track_name='Zero', + artist_name='C', album_name='Album3', popularity=0, + ) + with patch.object(svc, '_get_active_source', return_value='itunes'): + out = svc.get_popular_picks(limit=10) + names = sorted(t['track_name'] for t in out) + assert names == ['High', 'Low', 'Zero'] + + +# --------------------------------------------------------------------------- +# Fix #3 — Genre keyword filter pushed to SQL +# --------------------------------------------------------------------------- + + +def test_get_genre_playlist_pushes_filter_to_sql(service): + """Insert tracks with various artist_genres JSON values; only those + whose genres contain a rock keyword should come through. The match + happens in SQL (LIKE on the JSON-encoded string) rather than after + a million-row over-fetch.""" + import json as _json + svc, db = service + db.insert_discovery_track( + source='spotify', spotify_track_id='sp1', track_name='RockSong', + artist_name='RockBand', album_name='Album1', + artist_genres=_json.dumps(['indie rock', 'alternative']), + ) + db.insert_discovery_track( + source='spotify', spotify_track_id='sp2', track_name='JazzSong', + artist_name='JazzCat', album_name='Album2', + artist_genres=_json.dumps(['bebop', 'cool jazz']), + ) + db.insert_discovery_track( + source='spotify', spotify_track_id='sp3', track_name='PopSong', + artist_name='PopStar', album_name='Album3', + artist_genres=_json.dumps(['k-pop']), + ) + out = svc.get_genre_playlist('rock', limit=10) + names = [t['track_name'] for t in out] + # Only the indie rock track matches the literal "rock" keyword. + assert 'RockSong' in names + assert 'JazzSong' not in names + # k-pop doesn't contain "rock" as a substring → excluded. + assert 'PopSong' not in names + + +def test_get_genre_playlist_handles_parent_genre(service): + """Parent genres in GENRE_MAPPING expand to all their child keywords; + a track tagged with any child genre should be included.""" + import json as _json + svc, db = service + # 'Electronic/Dance' parent expands to keywords like 'house', 'techno', + # 'edm' etc. Tag tracks with various children. + db.insert_discovery_track( + source='spotify', spotify_track_id='sp1', track_name='HouseTrack', + artist_name='DJ1', album_name='A1', + artist_genres=_json.dumps(['deep house']), + ) + db.insert_discovery_track( + source='spotify', spotify_track_id='sp2', track_name='TechnoTrack', + artist_name='DJ2', album_name='A2', + artist_genres=_json.dumps(['minimal techno']), + ) + db.insert_discovery_track( + source='spotify', spotify_track_id='sp3', track_name='RockTrack', + artist_name='Band1', album_name='A3', + artist_genres=_json.dumps(['indie rock']), + ) + out = svc.get_genre_playlist('Electronic/Dance', limit=10) + names = sorted(t['track_name'] for t in out) + assert 'HouseTrack' in names + assert 'TechnoTrack' in names + assert 'RockTrack' not in names + + +# --------------------------------------------------------------------------- +# Fix #4 — `_select_discovery_tracks` excludes already-owned tracks +# --------------------------------------------------------------------------- + + +def test_discovery_helper_excludes_owned_tracks(service): + """Discovery row with spotify_track_id='sp1' + library track with the + same spotify_track_id should be filtered out. Owned tracks shouldn't + surface in Hidden Gems / Shuffle / Popular Picks.""" + svc, db = service + db.insert_discovery_track( + source='spotify', spotify_track_id='sp1', track_name='Owned', + artist_name='A', album_name='X', popularity=50, + ) + db.insert_library_track(spotify_track_id='sp1') + + tracks = svc._select_discovery_tracks( + source='spotify', + order_by='track_name', + fetch_limit=100, + ) + assert tracks == [] + + +def test_discovery_helper_keeps_unowned_tracks(service): + """Same shape but the library row carries a different spotify_track_id + — discovery row should pass through.""" + svc, db = service + db.insert_discovery_track( + source='spotify', spotify_track_id='sp1', track_name='NotOwned', + artist_name='A', album_name='X', popularity=50, + ) + db.insert_library_track(spotify_track_id='sp_different') + + tracks = svc._select_discovery_tracks( + source='spotify', + order_by='track_name', + fetch_limit=100, + ) + assert [t['track_name'] for t in tracks] == ['NotOwned'] + + +def test_discovery_helper_can_disable_owned_filter(service): + """`exclude_owned=False` lets owned rows pass through — used by code + paths that legitimately want library matches (currently none, but + the flag should still work).""" + svc, db = service + db.insert_discovery_track( + source='spotify', spotify_track_id='sp1', track_name='Owned', + artist_name='A', album_name='X', popularity=50, + ) + db.insert_library_track(spotify_track_id='sp1') + + tracks = svc._select_discovery_tracks( + source='spotify', + order_by='track_name', + fetch_limit=100, + exclude_owned=False, + ) + assert [t['track_name'] for t in tracks] == ['Owned'] + + +def test_discovery_helper_owned_filter_handles_deezer_id_asymmetry(service): + """Column-name asymmetry: discovery_pool.deezer_track_id vs + tracks.deezer_id. Pin this — easy to break in a future refactor.""" + svc, db = service + db.insert_discovery_track( + source='deezer', deezer_track_id='dz1', + spotify_track_id=None, itunes_track_id=None, + track_name='OwnedDeezer', artist_name='A', album_name='X', + popularity=50, + ) + db.insert_library_track(deezer_id='dz1') + + with patch.object(svc, '_get_active_source', return_value='deezer'): + tracks = svc._select_discovery_tracks( + source='deezer', + order_by='track_name', + fetch_limit=100, + ) + assert tracks == [] + diff --git a/webui/static/helper.js b/webui/static/helper.js index e0cdcf20..5bb24929 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3416,6 +3416,7 @@ const WHATS_NEW = { '2.4.3': [ // --- post-2.4.2 dev work — entries hidden by _getLatestWhatsNewVersion until the build version bumps --- { date: 'Unreleased — 2.4.3 dev cycle' }, + { title: 'Discover: Sharper Track Selection (Diversity, Source-Aware Popularity, Library Dedup, SQL Genre Filter)', desc: 'four selection-quality fixes on the soulsync-made discover playlists. (1) hidden gems and discovery shuffle had no diversity caps — they could return 50 tracks from the same artist or 20 from one album. now both apply the existing `_apply_diversity_filter` (over-fetch 3x then enforce per-album/per-artist caps; shuffle uses tighter caps because it should feel maximally varied). (2) `popularity` thresholds were spotify-shaped (0-100 scale, popular >= 60 / hidden < 40), but deezer writes its rank value into that column (often six-digit integers) and itunes writes nothing meaningful. for deezer-primary users this meant popular picks pulled essentially everything and hidden gems pulled nothing. new `_get_popularity_thresholds(source)` returns per-source values: spotify (60, 40), deezer (500_000, 100_000) ballpark, itunes/other (None, None) which skips the popularity filter entirely and falls back to random + diversity. (3) `get_genre_playlist` used to load up to 1M discovery_pool rows into python and run a substring keyword filter on the json column. now the keyword OR chain pushes down into sql via `(artist_genres LIKE ? OR ...)` placeholders, fetch_limit drops to limit*10. parent-genre expansion via GENRE_MAPPING preserved. (4) discovery selectors now exclude tracks the user already owns — `_select_discovery_tracks` gained `exclude_owned: bool = True` (default on) which adds a `NOT EXISTS (SELECT FROM tracks WHERE source_id matches)` correlated subquery covering the spotify/itunes/deezer-id columns (with the deezer-column-name asymmetry handled inline: discovery_pool.deezer_track_id vs tracks.deezer_id). hidden gems / shuffle / popular picks / decade / genre browser all benefit automatically. 12 new tests (27 total in the file): diversity caps, source-aware threshold values, threshold-skip behavior, sql-pushed genre filter, parent-genre expansion, owned-track exclusion, opt-out flag, and the deezer-column-name asymmetry trap. 2232/2232 full suite green.', page: 'discover' }, { title: 'Discover: Stop Showing Undownloadable Tracks (+Lift +Cleanup)', desc: 'audit found multiple discover-page sections (hidden gems / discovery shuffle / popular picks / decade browser / genre browser) had no `WHERE (spotify_track_id IS NOT NULL OR itunes_track_id IS NOT NULL OR ...)` gate on their selection sql. tracks with no source ids in the discovery pool were getting displayed, the user would click download, and the download would silently fail because there was nothing to look up. fix: lifted all five discovery_pool selection methods into shared private helpers (`_select_discovery_tracks`, `_apply_diversity_filter`, `_compute_adaptive_diversity_limits`) on `PersonalizedPlaylistsService`. mandatory id-validity gate is hard-coded into the selector — no opt-out flag, every public method inherits it for free. behavior preserved: same diversity tiers, same over-fetch multipliers, same popularity thresholds, same blacklist filter. ~314 lines of repeated select/diversity boilerplate collapsed across the 5 methods (-55% on those methods\' business logic). also deleted four sections that had been stubbed returning [] for ages (recently added / top tracks / forgotten favorites / familiar favorites) — frontend, backend endpoints, html blocks, helper docs, all gone. on the frontend, lifted the duplicated decade-browser + genre-browser tab management (~314 lines of identical fetch-tabs / render-tabstrip / fetch-content / render-tracklist / wire-sync-button code) into one shared `createTabbedBrowserSection(config)` helper. each browser is now a thin wrapper: ~3 lines per public function. 14 new tests pin the gate (every selector filters null-id rows), the diversity caps, the adaptive limit tiers, the source filter, and the blacklist filter.', page: 'discover' }, { title: 'Internal: Discover Controller — Cin Pre-Review Polish', desc: 'tightened the controller before opening the PR. (1) dropped the magic `extractItems` defaults — controller used to auto-pull `data.items` / `data.albums` / `data.artists` / `data.tracks` / `data.results` if no extractor was provided. removed the fallback chain. each section now MUST supply its own `extractItems(data) => array` callback. cin standard: explicit > implicit; the auto-fallback could silently grab the wrong key on endpoints that return multiple arrays. validated at register-time so misuse fails immediately. all 10 existing call sites already had explicit extractors so no migration churn. (2) replaced the `renderItems` returning null convention (used by Your Albums + manualDom-style sections) with an explicit `manualDom: true` config flag. clearer intent at the call site, less likely to be confused with a renderer error. (3) added a minimal node `--test` JS test file at `tests/static/test_discover_section_controller.mjs` — 32 tests pin the lifecycle contract: config validation (every required field), happy-path fetch+render, empty/stale/error states, no-fetch `data:` mode, manualDom mode, callable `fetchUrl`, load coalescing, refresh bypass, hook error containment, error toasts. runs via `node --test tests/static/` directly, OR via the regular pytest sweep (`tests/test_discover_section_controller_js.py` shells out to node and asserts a clean exit). skipped gracefully when node isn\'t available or is < 22. closes the "controller is a contract, pin it at the test boundary" gap that cin would have flagged. 2205/2205 full suite green (was 2204 + 1 new pytest wrapper); 32/32 node --test pass; ruff clean; js parses clean.', page: 'discover' }, { title: 'Internal: Discover Cleanup Round — Toast Errors, Stale State, Skipped Sections', desc: 'follow-up to the controller migration. extended `createDiscoverSectionController` with the hooks the per-section migrations surfaced as needed: callable `fetchUrl` (resolves the seasonal-playlist recreate-on-key-change hack), no-fetch `data:` mode (lets render-only sections like seasonal albums use the controller without inventing a fake endpoint), `beforeLoad` hook (lets dynamically-inserted sections like because-you-listen-to ensure their container exists before the spinner shows), `onSuccess(data)` hook (cleaner home for sibling header / subtitle / button updates than folding them into renderItems), and an `isStale` / `onStale` / `renderStale` triple for the third render state (data is empty BUT upstream is still discovering — show updating UI + start a poller, instead of the bare empty-state copy). turned on `showErrorToast: true` for every migrated section — section load failures now surface a global toast instead of silently spinning forever or swallowing into console.debug. that\'s the JohnBaumb #369 pattern applied at the UI layer. migrated the two sections that didn\'t fit the original controller contract: `loadYourAlbums` (uses isStale/onStale for stale-fetch UI + onSuccess for subtitle/filters/download-button side-effects + renderItems returning null since it delegates to the existing grid renderer) and `loadSeasonalAlbums` (uses no-fetch data mode since the parent `loadSeasonalContent` already fetched the season payload). also lifted the duplicated decade-tab + genre-tab sync-status block (✓/⏳/✗/percentage) into a `_renderSyncStatusBlock(idPrefix)` helper — two call sites now share one implementation. listenbrainz playlists keep their own block because the semantics differ (matching progress vs download progress). audit found the 13 supposedly-dead hidden sections aren\'t dead at all — they\'re gated on user data (discovery pool, library content, metadata cache) and self-surface when their data exists. removed one orphaned `loadPersonalizedDailyMixes()` call from `blockDiscoveryArtist` — daily mixes is intentionally paused, refreshing it from there was a no-op.', page: 'discover' },