diff --git a/flutter_client/lib/app.dart b/flutter_client/lib/app.dart index a456dcd3..58073049 100644 --- a/flutter_client/lib/app.dart +++ b/flutter_client/lib/app.dart @@ -1,6 +1,7 @@ import 'package:flutter/material.dart'; import 'package:flutter_riverpod/flutter_riverpod.dart'; +import 'cache/metadata_prefetcher.dart'; import 'cache/prefetcher.dart'; import 'cache/sync_controller.dart'; import 'shared/routing.dart'; @@ -27,6 +28,11 @@ class _MinstrelAppState extends ConsumerState { // ignore: unawaited_futures ref.read(syncControllerProvider.notifier).sync(); ref.read(prefetcherProvider); + // Metadata prefetcher: when /api/home returns, fire background + // albumProvider/artistProvider reads for the top-N items in + // each section so subsequent taps are drift hits, not network + // round trips. + ref.read(metadataPrefetcherProvider); }); } diff --git a/flutter_client/lib/cache/adapters.dart b/flutter_client/lib/cache/adapters.dart index ba19be86..0785284f 100644 --- a/flutter_client/lib/cache/adapters.dart +++ b/flutter_client/lib/cache/adapters.dart @@ -88,6 +88,12 @@ extension TrackRefDriftWrite on TrackRef { extension CachedPlaylistAdapter on CachedPlaylist { /// `ownerUsername` is server-derived; cache stores the userId only. /// Pass empty unless a join supplies it. + /// `coverUrl` is reconstructed deterministically from the playlist + /// id — the server serves the cached collage at this path + /// (handleGetPlaylistCover). Mirrors the album-cover trick. When + /// the server hasn't built a collage yet (system playlists with no + /// tracks at build time), the endpoint 404s and PlaylistCard's + /// ServerImage falls back to its slate placeholder. Playlist toRef({String ownerUsername = ''}) => Playlist( id: id, userId: userId, @@ -96,7 +102,7 @@ extension CachedPlaylistAdapter on CachedPlaylist { isPublic: isPublic, systemVariant: systemVariant, trackCount: trackCount, - coverUrl: '', // server-derived; cache doesn't persist + coverUrl: '/api/playlists/$id/cover', ownerUsername: ownerUsername, createdAt: '', updatedAt: '', diff --git a/flutter_client/lib/cache/audio_cache_manager.dart b/flutter_client/lib/cache/audio_cache_manager.dart index 9733eba9..5c4afecb 100644 --- a/flutter_client/lib/cache/audio_cache_manager.dart +++ b/flutter_client/lib/cache/audio_cache_manager.dart @@ -83,6 +83,27 @@ class AudioCacheManager { return path; } + /// Registers a file already on disk in the cache index. Intended for + /// the streaming path (LockCachingAudioSource) which writes the file + /// itself; we need an index row so eviction can find and delete it + /// when usage exceeds the cap. `source` defaults to incidental so + /// stream-cached tracks are first to be evicted. + Future registerStreamCache( + String trackId, + String path, + int sizeBytes, { + CacheSource source = CacheSource.incidental, + }) async { + await _db.into(_db.audioCacheIndex).insertOnConflictUpdate( + AudioCacheIndexCompanion.insert( + trackId: trackId, + path: path, + sizeBytes: sizeBytes, + source: source, + ), + ); + } + /// Removes a track from the index AND deletes the file. Future unpin(String trackId) async { final row = await (_db.select(_db.audioCacheIndex) @@ -96,13 +117,25 @@ class AudioCacheManager { .go(); } - /// Total bytes used by the cache. + /// Total bytes used by the cache. Walks the cache directory directly + /// instead of summing the index, because the streaming-as-you-play + /// path (audio_handler's LockCachingAudioSource) writes files + /// without registering an index row. The index sum would always + /// understate (often to zero) for users who only stream. Future usageBytes() async { - final result = await _db.customSelect( - 'SELECT COALESCE(SUM(size_bytes), 0) AS total FROM audio_cache_index', - readsFrom: {_db.audioCacheIndex}, - ).getSingle(); - return result.read('total'); + final dir = Directory(await _tracksDir()); + if (!await dir.exists()) return 0; + var total = 0; + await for (final entity in dir.list(followLinks: false)) { + if (entity is File) { + try { + total += await entity.length(); + } catch (_) { + // Race against concurrent writes / deletes — just skip. + } + } + } + return total; } /// Evicts files until usage ≤ targetBytes. Eviction order: diff --git a/flutter_client/lib/cache/metadata_prefetcher.dart b/flutter_client/lib/cache/metadata_prefetcher.dart new file mode 100644 index 00000000..2eef60cb --- /dev/null +++ b/flutter_client/lib/cache/metadata_prefetcher.dart @@ -0,0 +1,76 @@ +import 'package:flutter/foundation.dart' show debugPrint; +import 'package:flutter_riverpod/flutter_riverpod.dart'; + +import '../library/library_providers.dart'; +import '../models/home_data.dart'; + +/// Pre-warms the drift cache for likely-tap targets. Conservative on +/// purpose: only warms artistProvider rows (single row, single round +/// trip per id) and only ever fires once per id per session. Album +/// detail is NOT prewarmed — the albumProvider auto-fetches its track +/// list when missing, and pre-warming N albums fans out N parallel +/// "fetch tracks" round trips that compete with the user's actual +/// playback request for bandwidth. +class MetadataPrefetcher { + MetadataPrefetcher(this._ref) { + _ref.listen>(homeProvider, (_, next) { + next.whenData(_warmHome); + }); + } + + final Ref _ref; + + /// Per-session dedupe so re-rendering a screen (every UI rebuild + /// fires the data: callback) doesn't trigger N more fetches for + /// ids we've already warmed. + final Set _warmedArtists = {}; + + /// Cap per section. Covers what fits on screen without scrolling. + static const _topN = 8; + + /// Pre-warm artists. Albums are intentionally not pre-warmed — + /// see class comment. + void warmArtists(Iterable ids) { + var n = 0; + for (final id in ids) { + if (id.isEmpty) continue; + if (!_warmedArtists.add(id)) continue; // already warmed + if (n++ >= _topN) break; + _swallow(_ref.read(artistProvider(id).future)); + } + if (n > 0) debugPrint('metadataPrefetcher: warming $n artists'); + } + + void _warmHome(HomeData h) { + final artistIds = {}; + for (final a in h.recentlyAddedAlbums.take(_topN)) { + if (a.artistId.isNotEmpty) artistIds.add(a.artistId); + } + for (final a in h.rediscoverAlbums.take(_topN)) { + if (a.artistId.isNotEmpty) artistIds.add(a.artistId); + } + for (final ar in h.rediscoverArtists.take(_topN)) { + if (ar.id.isNotEmpty) artistIds.add(ar.id); + } + for (final ar in h.lastPlayedArtists.take(_topN)) { + if (ar.id.isNotEmpty) artistIds.add(ar.id); + } + for (final t in h.mostPlayedTracks.take(_topN)) { + if (t.artistId.isNotEmpty) artistIds.add(t.artistId); + } + warmArtists(artistIds); + } + + /// Discards the return value and any error from a fire-and-forget + /// provider read. We don't care about the value here — we only want + /// the side effect of writing drift. + void _swallow(Future f) { + f.then((_) {}).onError((_, __) {}); + } +} + +/// Read once at app start to activate the prefetcher (e.g. wire it +/// from a top-level Consumer or main.dart container override). +final metadataPrefetcherProvider = Provider((ref) { + return MetadataPrefetcher(ref); +}); diff --git a/flutter_client/lib/library/artist_detail_screen.dart b/flutter_client/lib/library/artist_detail_screen.dart index e13d276d..3f624418 100644 --- a/flutter_client/lib/library/artist_detail_screen.dart +++ b/flutter_client/lib/library/artist_detail_screen.dart @@ -92,10 +92,33 @@ class ArtistDetailScreen extends ConsumerWidget { child: IconButton( icon: Icon(Icons.play_arrow, color: fs.parchment), onPressed: () async { - final tracks = await ref.read(artistTracksProvider(id).future); - if (tracks.isEmpty) return; - final shuffled = [...tracks]..shuffle(); - ref.read(playerActionsProvider).playTracks(shuffled); + try { + final tracks = + await ref.read(artistTracksProvider(id).future); + debugPrint( + 'artist_detail: play tapped — ${tracks.length} tracks for $id'); + if (tracks.isEmpty) { + if (context.mounted) { + ScaffoldMessenger.of(context).showSnackBar( + const SnackBar( + content: Text( + 'No tracks found for this artist yet.')), + ); + } + return; + } + final shuffled = [...tracks]..shuffle(); + await ref + .read(playerActionsProvider) + .playTracks(shuffled); + } catch (e) { + debugPrint('artist_detail: play failed: $e'); + if (context.mounted) { + ScaffoldMessenger.of(context).showSnackBar( + SnackBar(content: Text("Couldn't start playback: $e")), + ); + } + } }, ), ), @@ -109,7 +132,8 @@ class ArtistDetailScreen extends ConsumerWidget { albums.when( error: (e, _) => Center(child: Text('$e', style: TextStyle(color: fs.error))), loading: () => const Padding(padding: EdgeInsets.all(16), child: Center(child: CircularProgressIndicator())), - data: (list) => LayoutBuilder(builder: (ctx, constraints) { + data: (list) { + return LayoutBuilder(builder: (ctx, constraints) { const cols = 3; const sidePad = 8.0; const gap = 8.0; @@ -117,10 +141,11 @@ class ArtistDetailScreen extends ConsumerWidget { (constraints.maxWidth - sidePad * 2 - gap * (cols - 1)) / cols; // Card content: cover (cellW - 16) + 8 + title (≤2 lines - // ≈ 36) + small fudge. Artist line is suppressed in this + // ≈ 36) + slack. Artist line is suppressed in this // grid (showArtist: false) since the page header already - // names the artist. - final cellH = (cellW - 16) + 8 + 36 + 4; + // names the artist. Slack is generous on purpose — line- + // height variations would otherwise overflow by 1px. + final cellH = (cellW - 16) + 8 + 36 + 8; return GridView.builder( shrinkWrap: true, physics: const NeverScrollableScrollPhysics(), @@ -144,7 +169,8 @@ class ArtistDetailScreen extends ConsumerWidget { ); }, ); - }), + }); + }, ), ]); } diff --git a/flutter_client/lib/library/library_providers.dart b/flutter_client/lib/library/library_providers.dart index 202b5bf4..28e0a296 100644 --- a/flutter_client/lib/library/library_providers.dart +++ b/flutter_client/lib/library/library_providers.dart @@ -66,9 +66,10 @@ final artistProvider = isOnline: () async => (await ref .read(connectivityProvider.future) .timeout(const Duration(seconds: 3), onTimeout: () => true)), - // SWR: yield cache instantly on hit, refresh in the background so - // the row catches up to server state without making the user wait. - alwaysRefresh: true, + // No alwaysRefresh: artist rows don't change frequently, and the + // metadata prefetcher creates many subscriptions in parallel — + // each silently re-fetching once would saturate the request + // pipeline behind the user's actual playback request. tag: 'artist($id)', ); }); @@ -100,7 +101,6 @@ final artistAlbumsProvider = isOnline: () async => (await ref .read(connectivityProvider.future) .timeout(const Duration(seconds: 3), onTimeout: () => true)), - alwaysRefresh: true, tag: 'artistAlbums($artistId)', ); }); @@ -137,7 +137,6 @@ final artistTracksProvider = isOnline: () async => (await ref .read(connectivityProvider.future) .timeout(const Duration(seconds: 3), onTimeout: () => true)), - alwaysRefresh: true, tag: 'artistTracks($artistId)', ); }); @@ -169,10 +168,7 @@ final albumProvider = StreamProvider.family< // Once-per-subscription guard so we don't re-fetch in a loop if the // server genuinely returns zero tracks (or if the fetch fails). var fetchAttempted = false; - // SWR revalidation guard — fire one background refresh per - // subscription on the first complete cache hit, so the displayed - // data catches up to server state without making the user wait. - var revalidated = false; + // (revalidated flag removed; see SWR note below the yield.) Future isOnline() async { try { @@ -303,15 +299,13 @@ final albumProvider = StreamProvider.family< ); }).toList(); - // SWR: first complete cache hit triggers one background refresh - // so we don't show stale data indefinitely. Drift watch re-emits - // on success and the next iteration yields the fresh data. - if (!revalidated && trackRows.isNotEmpty) { - revalidated = true; - if (await isOnline()) { - unawaited(fetchAndPopulate()); - } - } + // Note: NO SWR here on purpose. Prior code kicked a background + // refresh on every first cache hit, which combined with the + // metadata prefetcher meant every prewarmed album id triggered + // an extra fetch even when drift was already populated. Tracks + // and album metadata don't change on the same timescale as + // playlists; a stale read is fine until the user invalidates + // (pull-to-refresh) or the album is genuinely re-fetched. yield (album: album, tracks: tracks); } diff --git a/flutter_client/lib/library/library_screen.dart b/flutter_client/lib/library/library_screen.dart index 79f47e6e..b3e85380 100644 --- a/flutter_client/lib/library/library_screen.dart +++ b/flutter_client/lib/library/library_screen.dart @@ -5,6 +5,7 @@ import 'package:go_router/go_router.dart'; import '../api/endpoints/library_lists.dart'; import '../api/endpoints/likes.dart'; import '../api/endpoints/me.dart'; +import '../cache/metadata_prefetcher.dart'; import '../library/library_providers.dart' show dioProvider; import '../models/album.dart'; import '../models/artist.dart'; @@ -207,7 +208,12 @@ class _ArtistsTab extends ConsumerWidget { return ref.watch(_libraryArtistsProvider).when( loading: () => const Center(child: CircularProgressIndicator()), error: (e, _) => Center(child: Text('$e', style: TextStyle(color: fs.error))), - data: (page) => page.items.isEmpty + data: (page) { + // Warm details for the first screenful so taps are instant. + ref + .read(metadataPrefetcherProvider) + .warmArtists(page.items.map((a) => a.id)); + return page.items.isEmpty ? Center(child: Text("No artists yet — scan a library folder via the server's config.", style: TextStyle(color: fs.ash), textAlign: TextAlign.center)) : RefreshIndicator( onRefresh: () async { @@ -247,7 +253,8 @@ class _ArtistsTab extends ConsumerWidget { }, ), ), - ), + ); + }, ); } } @@ -261,7 +268,8 @@ class _AlbumsTab extends ConsumerWidget { return ref.watch(_libraryAlbumsProvider).when( loading: () => const Center(child: CircularProgressIndicator()), error: (e, _) => Center(child: Text('$e', style: TextStyle(color: fs.error))), - data: (page) => page.items.isEmpty + data: (page) { + return page.items.isEmpty ? Center(child: Text("No albums yet — scan a library folder via the server's config.", style: TextStyle(color: fs.ash), textAlign: TextAlign.center)) : RefreshIndicator( onRefresh: () async { @@ -280,8 +288,11 @@ class _AlbumsTab extends ConsumerWidget { gap * (cols - 1)) / cols; // cover (cellW - 16) + gap (8) + 2-line title (~36) - // + artist line (~16) + small fudge. - final cellH = (cellW - 16) + 8 + 36 + 16 + 4; + // + artist line (~16) + slack. Slack is generous on + // purpose — line-height + font scaling variations + // would otherwise overflow the cell by a pixel + // (logged as a noisy RenderFlex warning). + final cellH = (cellW - 16) + 8 + 36 + 16 + 8; return NotificationListener( onNotification: (n) { if (n.metrics.pixels >= @@ -315,7 +326,8 @@ class _AlbumsTab extends ConsumerWidget { ), ); }), - ), + ); + }, ); } } diff --git a/flutter_client/lib/player/audio_handler.dart b/flutter_client/lib/player/audio_handler.dart index 132d9296..3bade142 100644 --- a/flutter_client/lib/player/audio_handler.dart +++ b/flutter_client/lib/player/audio_handler.dart @@ -12,12 +12,36 @@ import 'album_cover_cache.dart'; class MinstrelAudioHandler extends BaseAudioHandler with QueueHandler, SeekHandler { MinstrelAudioHandler() { - _player.playbackEventStream.listen(_broadcastState); + _player.playbackEventStream.listen( + _broadcastState, + // ExoPlayer surfaces stream errors (404, premature EOS, decoder + // failure, network drop) here. Without an error sink, the + // player just goes quiet — exactly the "starts then stops" + // symptom we hit. + onError: (Object e, StackTrace st) { + debugPrint('audio_handler: playbackEventStream error: $e\n$st'); + }, + ); _player.currentIndexStream.listen(_onCurrentIndexChanged); // Re-broadcast on shuffle/repeat changes so the PlaybackState's // shuffleMode + repeatMode fields stay current for UI subscribers. _player.shuffleModeEnabledStream.listen((_) => _broadcastState(null)); _player.loopModeStream.listen((_) => _broadcastState(null)); + // Watch buffered-position so we can register stream-cached files + // in the audio cache index once they're fully downloaded. Without + // this, files written by LockCachingAudioSource never appear in + // the index and the eviction loop can't reclaim them. + _player.bufferedPositionStream + .listen((_) => unawaited(_maybeRegisterStreamCache())); + // Diagnostic: log every player-state transition so silent stops + // surface as something we can read in the log. + _player.playerStateStream.listen((s) { + debugPrint( + 'audio_handler: state playing=${s.playing} processing=${s.processingState}'); + }); + _player.processingStateStream.listen((ps) { + debugPrint('audio_handler: processingState=$ps'); + }); } final AudioPlayer _player = AudioPlayer(); @@ -26,6 +50,16 @@ class MinstrelAudioHandler extends BaseAudioHandler with QueueHandler, SeekHandl AlbumCoverCache? _coverCache; AudioCacheManager? _audioCacheManager; + /// Trackers to dedupe registration — once we've inserted an index row + /// for a trackId, don't repeat the work on every buffered-position + /// emit. Cleared on dispose only; surviving across queue rebuilds is + /// fine because the index is itself the source of truth. + final Set _streamCacheRegistered = {}; + + /// Cached on first use so we don't hit the platform channel every + /// time the buffered-position stream emits (~200ms cadence). + String? _cacheDirPath; + /// Volume stream for UI subscribers. Mirrors the just_audio player's /// volume directly; set via setVolume(double). Stream get volumeStream => _player.volumeStream; @@ -64,12 +98,18 @@ class MinstrelAudioHandler extends BaseAudioHandler with QueueHandler, SeekHandl debugPrint('audio_handler.setQueueFromTracks: ' '_baseUrl="$_baseUrl" trackCount=${tracks.length}'); + final sw = Stopwatch()..start(); final sources = await Future.wait(tracks.map(_buildAudioSource)); + debugPrint('audio_handler: built ${sources.length} sources in ' + '${sw.elapsedMilliseconds}ms'); + sw.reset(); + sw.start(); await _player.setAudioSources( sources, initialIndex: initialIndex, ); + debugPrint('audio_handler: setAudioSources ${sw.elapsedMilliseconds}ms'); // Kick the cover fetch for the initial item — async, doesn't block // playback. Subsequent track changes are handled by the @@ -127,8 +167,8 @@ class MinstrelAudioHandler extends BaseAudioHandler with QueueHandler, SeekHandl // pin / Download buttons cover the index path; LockCaching handles // the network optimization. if (mgr != null) { - final cacheDir = await getApplicationCacheDirectory(); - final cacheFile = File('${cacheDir.path}/audio_cache/${t.id}.mp3'); + _cacheDirPath ??= (await getApplicationCacheDirectory()).path; + final cacheFile = File('${_cacheDirPath!}/audio_cache/${t.id}.mp3'); debugPrint('audio_handler: cache miss for track.id=${t.id}, ' 'using LockCachingAudioSource → ${cacheFile.path}'); // ignore: experimental_member_use @@ -184,6 +224,39 @@ class MinstrelAudioHandler extends BaseAudioHandler with QueueHandler, SeekHandl ); } + /// Once a track is fully buffered (LockCaching has written the whole + /// file to disk), insert an audio_cache_index row so the file shows + /// up to AudioCacheManager.evict() and clearAll(). No-op if the + /// cache manager isn't configured, no current track, the file isn't + /// fully buffered yet, the on-disk file is missing, or we already + /// registered this trackId during this subscription. + Future _maybeRegisterStreamCache() async { + final mgr = _audioCacheManager; + if (mgr == null) return; + final current = mediaItem.value; + if (current == null) return; + final trackId = current.id; + if (_streamCacheRegistered.contains(trackId)) return; + + final dur = _player.duration; + if (dur == null) return; + final buf = _player.bufferedPosition; + // 200ms slack for header bytes / encoding rounding. + if (buf < dur - const Duration(milliseconds: 200)) return; + + _cacheDirPath ??= (await getApplicationCacheDirectory()).path; + final path = '${_cacheDirPath!}/audio_cache/$trackId.mp3'; + final file = File(path); + if (!await file.exists()) return; + final size = await file.length(); + if (size <= 0) return; + + _streamCacheRegistered.add(trackId); + await mgr.registerStreamCache(trackId, path, size); + debugPrint( + 'audio_handler: registered stream cache for $trackId ($size bytes)'); + } + void _onCurrentIndexChanged(int? idx) { if (idx == null) return; // Push the new track's MediaItem onto the mediaItem stream so diff --git a/flutter_client/lib/player/now_playing_screen.dart b/flutter_client/lib/player/now_playing_screen.dart index ff518406..804c4ef3 100644 --- a/flutter_client/lib/player/now_playing_screen.dart +++ b/flutter_client/lib/player/now_playing_screen.dart @@ -113,20 +113,21 @@ class _NowPlayingScreenState extends ConsumerState { overflow: TextOverflow.ellipsis, ), ], - const SizedBox(height: 32), - _SeekRow(position: pos, duration: dur, fs: fs, ref: ref), - const SizedBox(height: 24), - _PrimaryControls( - fs: fs, - ref: ref, - isPlaying: isPlaying, - ), const SizedBox(height: 24), _SecondaryControls( fs: fs, actions: actions, shuffleOn: shuffleOn, repeatMode: repeatMode, + media: media, + ), + const SizedBox(height: 8), + _SeekRow(position: pos, duration: dur, fs: fs, ref: ref), + const SizedBox(height: 24), + _PrimaryControls( + fs: fs, + ref: ref, + isPlaying: isPlaying, ), const SizedBox(height: 24), ], @@ -219,48 +220,14 @@ class _TitleRow extends StatelessWidget { @override Widget build(BuildContext context) { - // Stack: title centered absolutely in the row; like + kebab pinned - // to the right edge. Padding on the title equals the actions' width - // so it stays optically centered without colliding with them. - return SizedBox( - height: 32, - child: Stack( - alignment: Alignment.center, - children: [ - Padding( - padding: const EdgeInsets.symmetric(horizontal: 64), - child: Text( - media.title, - style: TextStyle(color: fs.parchment, fontSize: 22), - maxLines: 1, - overflow: TextOverflow.ellipsis, - textAlign: TextAlign.center, - ), - ), - Positioned( - right: 0, - child: Row( - mainAxisSize: MainAxisSize.min, - children: [ - LikeButton(kind: LikeKind.track, id: media.id, size: 22), - TrackActionsButton( - track: TrackRef( - id: media.id, - title: media.title, - albumId: (media.extras?['album_id'] as String?) ?? '', - albumTitle: media.album ?? '', - artistId: (media.extras?['artist_id'] as String?) ?? '', - artistName: media.artist ?? '', - durationSec: media.duration?.inSeconds ?? 0, - streamUrl: '', - ), - hideQueueActions: true, - ), - ], - ), - ), - ], - ), + // Title-only — like + kebab moved into _SecondaryControls above + // the seek bar so they share the same row as shuffle/repeat/queue. + return Text( + media.title, + style: TextStyle(color: fs.parchment, fontSize: 22), + maxLines: 1, + overflow: TextOverflow.ellipsis, + textAlign: TextAlign.center, ); } } @@ -365,17 +332,23 @@ class _PrimaryControls extends StatelessWidget { } } +/// Action row sitting just above the seek bar. Holds shuffle / repeat +/// / queue plus the like + kebab that used to live in the title row, +/// so the title can sit truly centered above and this row carries +/// every per-track action in one place. class _SecondaryControls extends StatelessWidget { const _SecondaryControls({ required this.fs, required this.actions, required this.shuffleOn, required this.repeatMode, + required this.media, }); final FabledSwordTheme fs; final PlayerActions actions; final bool shuffleOn; final AudioServiceRepeatMode repeatMode; + final MediaItem media; @override Widget build(BuildContext context) { @@ -411,6 +384,20 @@ class _SecondaryControls extends StatelessWidget { icon: Icon(Icons.queue_music, color: fs.ash), onPressed: () => GoRouter.of(context).push('/queue'), ), + LikeButton(kind: LikeKind.track, id: media.id, size: 22), + TrackActionsButton( + track: TrackRef( + id: media.id, + title: media.title, + albumId: (media.extras?['album_id'] as String?) ?? '', + albumTitle: media.album ?? '', + artistId: (media.extras?['artist_id'] as String?) ?? '', + artistName: media.artist ?? '', + durationSec: media.duration?.inSeconds ?? 0, + streamUrl: '', + ), + hideQueueActions: true, + ), ], ); } diff --git a/flutter_client/lib/player/player_provider.dart b/flutter_client/lib/player/player_provider.dart index baca1af3..47e51879 100644 --- a/flutter_client/lib/player/player_provider.dart +++ b/flutter_client/lib/player/player_provider.dart @@ -1,4 +1,5 @@ import 'package:audio_service/audio_service.dart'; +import 'package:flutter/foundation.dart' show debugPrint; import 'package:flutter_riverpod/flutter_riverpod.dart'; import '../api/endpoints/radio.dart'; @@ -53,8 +54,22 @@ class PlayerActions { final Ref _ref; Future playTracks(List tracks, {int initialIndex = 0}) async { + // Stage-by-stage timing so we can see exactly where the + // "tap → audio" delay lives. Look for "playTracks: ..." lines + // in the next log capture; each stage label is followed by its + // elapsed millis since the previous stage. + final sw = Stopwatch()..start(); + int lap() { + final ms = sw.elapsedMilliseconds; + sw.reset(); + sw.start(); + return ms; + } + final url = await _ref.read(serverUrlProvider.future); + debugPrint('playTracks: serverUrl ${lap()}ms'); final token = await _ref.read(secureStorageProvider).read(key: 'session_token'); + debugPrint('playTracks: token ${lap()}ms'); final cache = _ref.read(albumCoverCacheProvider); final audioCache = _ref.read(audioCacheManagerProvider); final h = _ref.read(audioHandlerProvider) @@ -64,8 +79,11 @@ class PlayerActions { coverCache: cache, audioCacheManager: audioCache, ); + debugPrint('playTracks: configure ${lap()}ms'); await h.setQueueFromTracks(tracks, initialIndex: initialIndex); + debugPrint('playTracks: setQueue (${tracks.length} tracks) ${lap()}ms'); await h.play(); + debugPrint('playTracks: play() returned ${lap()}ms'); } Future playNext(TrackRef track) async { diff --git a/flutter_client/lib/playlists/playlists_list_screen.dart b/flutter_client/lib/playlists/playlists_list_screen.dart index 217599b6..7cf4172a 100644 --- a/flutter_client/lib/playlists/playlists_list_screen.dart +++ b/flutter_client/lib/playlists/playlists_list_screen.dart @@ -83,7 +83,11 @@ class _PlaylistTile extends StatelessWidget { color: fs.slate, child: playlist.coverUrl.isEmpty ? Icon(Icons.queue_music, color: fs.ash) - : ServerImage(url: playlist.coverUrl, fit: BoxFit.cover), + : ServerImage( + url: playlist.coverUrl, + fit: BoxFit.cover, + fallback: Icon(Icons.queue_music, color: fs.ash), + ), ), ), const SizedBox(width: 12), diff --git a/flutter_client/lib/playlists/playlists_provider.dart b/flutter_client/lib/playlists/playlists_provider.dart index c0164b5b..dc4105d0 100644 --- a/flutter_client/lib/playlists/playlists_provider.dart +++ b/flutter_client/lib/playlists/playlists_provider.dart @@ -1,5 +1,6 @@ import 'dart:async'; +import 'package:dio/dio.dart'; import 'package:drift/drift.dart' as drift; import 'package:flutter/foundation.dart' show debugPrint; import 'package:flutter_riverpod/flutter_riverpod.dart'; @@ -44,7 +45,21 @@ final playlistsListProvider = debugPrint( 'playlistsListProvider($kind): wire returned owned=${fresh.owned.length} ' '(system=$ownedSysCount) public=${fresh.public.length}'); + + // Reconcile: BuildSystemPlaylists rotates system-playlist UUIDs + // every rebuild, so insertOrReplace alone leaves stale rows in + // drift. Tapping one of those stale tiles 404s. Delete every + // owned drift row whose id isn't in the fresh response, then + // upsert the fresh set. + final freshOwnedIds = + fresh.owned.map((p) => p.id).toSet(); await db.batch((b) { + if (user != null) { + b.deleteWhere(db.cachedPlaylists, (t) { + return t.userId.equals(user.id) & + t.id.isNotIn(freshOwnedIds); + }); + } for (final p in fresh.all) { b.insert(db.cachedPlaylists, p.toDrift(), mode: drift.InsertMode.insertOrReplace); @@ -143,13 +158,29 @@ final playlistDetailProvider = debugPrint( 'playlistDetailProvider($id): got ${fresh.tracks.length} tracks, writing'); - // Collect the artist + album rows referenced by these tracks so - // the JOINs that build artistName/albumTitle in the row view - // have something to bind to. Without this, system-playlist tracks - // surface with empty artist/album columns. + // Collect the track + artist + album rows referenced by these + // playlist entries. Without writing the cachedTracks rows + // themselves, the detail-screen JOIN against cachedTracks + // returns null on every row (only the playlist_tracks join + // succeeds), so the UI shows a list with empty titles and + // unplayable tracks. + final tracks = {}; final artists = {}; final albums = {}; for (final t in fresh.tracks) { + final tId = t.trackId; + if (tId != null && tId.isNotEmpty) { + tracks.putIfAbsent( + tId, + () => _TrackRefRow( + id: tId, + title: t.title, + albumId: t.albumId ?? '', + artistId: t.artistId ?? '', + durationSec: t.durationSec, + ), + ); + } final aId = t.artistId; final aName = t.artistName; if (aId != null && aId.isNotEmpty && aName.isNotEmpty) { @@ -198,6 +229,25 @@ final playlistDetailProvider = .toList(), ); } + if (tracks.isNotEmpty) { + // Insert/update cachedTracks so the detail screen's JOIN + // produces real titles + durations. We don't have + // track_number / disc_number on the wire (PlaylistTrack + // omits them), so they default to 0 — UI doesn't surface + // them on this screen so it's fine. + b.insertAllOnConflictUpdate( + db.cachedTracks, + tracks.values + .map((t) => CachedTracksCompanion.insert( + id: t.id, + albumId: t.albumId, + artistId: t.artistId, + title: t.title, + durationMs: drift.Value(t.durationSec * 1000), + )) + .toList(), + ); + } if (albums.isNotEmpty) { b.insertAllOnConflictUpdate( db.cachedAlbums, @@ -216,6 +266,21 @@ final playlistDetailProvider = 'playlistDetailProvider($id): drift write done; awaiting watch re-emit'); return true; } catch (e, st) { + // 404 = the playlist row in drift is stale (BuildSystemPlaylists + // rotates UUIDs on each rebuild, so old For-You / Songs-Like + // tiles can outlive the actual server-side playlist). Wipe the + // stale row + its track positions so the home tile disappears + // on next render and we don't keep trying. + if (e is DioException && e.response?.statusCode == 404) { + debugPrint( + 'playlistDetailProvider($id): server says 404, evicting stale drift rows'); + await db.batch((b) { + b.deleteWhere( + db.cachedPlaylistTracks, (t) => t.playlistId.equals(id)); + b.deleteWhere(db.cachedPlaylists, (t) => t.id.equals(id)); + }); + return false; + } debugPrint('playlistDetailProvider($id): fetch failed: $e\n$st'); return false; } @@ -224,7 +289,6 @@ final playlistDetailProvider = // Once-per-subscription guard so an empty server response doesn't // cause repeated re-fetches. var fetchAttempted = false; - var revalidated = false; await for (final playlistRows in playlistQuery.watch()) { if (playlistRows.isEmpty) { @@ -283,13 +347,12 @@ final playlistDetailProvider = yield PlaylistDetail(playlist: playlist, tracks: tracks); - // SWR: first complete cache hit kicks one background refresh. - if (!revalidated && trackRows.isNotEmpty) { - revalidated = true; - if (await isOnline()) { - unawaited(fetchAndPopulate()); - } - } + // No SWR refresh here. The aggregate playlistsListProvider does + // alwaysRefresh (system playlists rotate UUIDs), but per-detail + // refresh on every visit was multiplying with the prefetcher's + // parallel fetches and starving user-initiated playback. Pull- + // to-refresh on the detail page invalidates the provider, which + // is the right path for an explicit refresh. } }); @@ -309,6 +372,21 @@ class AlbumRefRow { final String artistId; } +class _TrackRefRow { + _TrackRefRow({ + required this.id, + required this.title, + required this.albumId, + required this.artistId, + required this.durationSec, + }); + final String id; + final String title; + final String albumId; + final String artistId; + final int durationSec; +} + final systemPlaylistsStatusProvider = FutureProvider((ref) async { final dio = await ref.watch(dioProvider.future); diff --git a/flutter_client/lib/playlists/widgets/playlist_card.dart b/flutter_client/lib/playlists/widgets/playlist_card.dart index e6781f37..060351a4 100644 --- a/flutter_client/lib/playlists/widgets/playlist_card.dart +++ b/flutter_client/lib/playlists/widgets/playlist_card.dart @@ -31,9 +31,19 @@ class PlaylistCard extends StatelessWidget { width: 144, height: 144, color: fs.slate, + // coverUrl is deterministic per playlist (see + // CachedPlaylistAdapter.toRef). When the server's + // collage isn't built yet, ServerImage's + // errorBuilder shows the queue_music icon over the + // slate background. child: playlist.coverUrl.isEmpty ? Icon(Icons.queue_music, color: fs.ash, size: 56) - : ServerImage(url: playlist.coverUrl, fit: BoxFit.cover), + : ServerImage( + url: playlist.coverUrl, + fit: BoxFit.cover, + fallback: + Icon(Icons.queue_music, color: fs.ash, size: 56), + ), ), ), const SizedBox(height: 8), diff --git a/flutter_client/lib/shared/routing.dart b/flutter_client/lib/shared/routing.dart index 532cf1e3..73c3eac5 100644 --- a/flutter_client/lib/shared/routing.dart +++ b/flutter_client/lib/shared/routing.dart @@ -85,6 +85,14 @@ GoRouter buildRouter(Ref ref) { }, ), ), + // /queue lives outside the ShellRoute too. Why: pushing /queue + // from /now-playing (which is also outside the shell) used to + // cause go_router to mount a second ShellRoute instance under + // the existing one, producing a duplicate page-key assertion + // (NavigatorState._debugCheckDuplicatedPageKeys). Top-level + // routes can stack on each other freely; shell-children can't + // when something on top of the shell is already routing. + GoRoute(path: '/queue', builder: (_, __) => const QueueScreen()), ShellRoute( builder: (ctx, state, child) => VersionGate(child: _ShellWithPlayerBar(child: child)), routes: [ @@ -105,7 +113,6 @@ GoRouter buildRouter(Ref ref) { seed: s.extra is AlbumRef ? s.extra as AlbumRef : null, ), ), - GoRoute(path: '/queue', builder: (_, __) => const QueueScreen()), GoRoute(path: '/search', builder: (_, __) => const SearchScreen()), GoRoute(path: '/library', builder: (_, __) => const LibraryScreen()), GoRoute(path: '/discover', builder: (_, __) => const DiscoverScreen()), diff --git a/internal/api/library.go b/internal/api/library.go index 3d94da35..206c5549 100644 --- a/internal/api/library.go +++ b/internal/api/library.go @@ -40,19 +40,28 @@ func (h *handlers) handleGetTrack(w http.ResponseWriter, r *http.Request) { // handleGetAlbum implements GET /api/albums/{id}. Returns the album plus its // tracks (ordered by disc/track number via the underlying query) with // duration summed from the track list — keeps one source of truth. +// +// Two DB round trips: the album+artist join and the per-user tracks +// list. Down from three (separate album, artist, tracks) before +// GetAlbumWithArtist landed. func (h *handlers) handleGetAlbum(w http.ResponseWriter, r *http.Request) { q := dbq.New(h.pool) - album, apiErr := resolveByID(r, "id", q.GetAlbumByID, "album") - if apiErr != nil { - writeErr(w, apiErr) + id, ok := requireURLUUID(w, r, "id") + if !ok { return } - artist, err := q.GetArtistByID(r.Context(), album.ArtistID) + row, err := q.GetAlbumWithArtist(r.Context(), id) if err != nil { - h.logger.Error("api: get album artist failed", "err", err) + if errors.Is(err, pgx.ErrNoRows) { + writeErr(w, apierror.NotFound("album")) + return + } + h.logger.Error("api: get album+artist failed", "err", err) writeErr(w, apierror.InternalMsg("lookup failed", err)) return } + album := row.Album + artistName := row.ArtistName var userID pgtype.UUID if user, ok := auth.UserFromContext(r.Context()); ok { userID = user.ID @@ -68,20 +77,24 @@ func (h *handlers) handleGetAlbum(w http.ResponseWriter, r *http.Request) { refs := make([]TrackRef, 0, len(tracks)) durSec := 0 for _, t := range tracks { - ref := trackRefFrom(t, album.Title, artist.Name) + ref := trackRefFrom(t, album.Title, artistName) refs = append(refs, ref) durSec += ref.DurationSec } detail := AlbumDetail{ - AlbumRef: albumRefFrom(album, artist.Name, len(tracks), durSec), + AlbumRef: albumRefFrom(album, artistName, len(tracks), durSec), Tracks: refs, } writeJSON(w, http.StatusOK, detail) } // handleGetArtist implements GET /api/artists/{id}. Returns artist + albums; -// each album carries its own track_count (one count query per album, same -// pattern as subsonic). At M1 sizes (albums-per-artist << 100) this is fine. +// each album carries its own track_count. +// +// Down from 1 + 1 + N queries (artist, albums, per-album CountTracksByAlbum) +// to 1 + 1 (artist, albums-with-track-count via correlated subquery). +// On a 30-album artist that's ~32 round trips collapsed to 2 — the +// difference between "feels slow" and "feels instant" on detail nav. func (h *handlers) handleGetArtist(w http.ResponseWriter, r *http.Request) { q := dbq.New(h.pool) artist, apiErr := resolveByID(r, "id", q.GetArtistByID, "artist") @@ -89,25 +102,19 @@ func (h *handlers) handleGetArtist(w http.ResponseWriter, r *http.Request) { writeErr(w, apiErr) return } - albums, err := q.ListAlbumsByArtist(r.Context(), artist.ID) + rows, err := q.ListAlbumsByArtistWithTrackCount(r.Context(), artist.ID) if err != nil { h.logger.Error("api: list albums by artist failed", "err", err) writeErr(w, apierror.InternalMsg("lookup failed", err)) return } - refs := make([]AlbumRef, 0, len(albums)) - for _, a := range albums { - count, cerr := q.CountTracksByAlbum(r.Context(), a.ID) - if cerr != nil { - h.logger.Error("api: count tracks failed", "err", cerr) - writeErr(w, apierror.InternalMsg("lookup failed", cerr)) - return - } + refs := make([]AlbumRef, 0, len(rows)) + for _, row := range rows { // durationSec=0: not aggregated for nested album lists per spec data flow. - refs = append(refs, albumRefFrom(a, artist.Name, int(count), 0)) + refs = append(refs, albumRefFrom(row.Album, artist.Name, int(row.TrackCount), 0)) } detail := ArtistDetail{ - ArtistRef: artistRefFrom(artist, len(albums)), + ArtistRef: artistRefFrom(artist, len(rows)), Albums: refs, } writeJSON(w, http.StatusOK, detail) diff --git a/internal/db/dbq/albums.sql.go b/internal/db/dbq/albums.sql.go index 5168f686..ca990688 100644 --- a/internal/db/dbq/albums.sql.go +++ b/internal/db/dbq/albums.sql.go @@ -168,6 +168,41 @@ func (q *Queries) GetAlbumCoverageRollup(ctx context.Context) (GetAlbumCoverageR return i, err } +const getAlbumWithArtist = `-- name: GetAlbumWithArtist :one +SELECT albums.id, albums.title, albums.sort_title, albums.artist_id, albums.release_date, albums.mbid, albums.cover_art_path, albums.created_at, albums.updated_at, albums.cover_art_source, albums.cover_art_sources_version, artists.name AS artist_name +FROM albums +JOIN artists ON artists.id = albums.artist_id +WHERE albums.id = $1 +` + +type GetAlbumWithArtistRow struct { + Album Album + ArtistName string +} + +// Combined fetch for /api/albums/{id}: returns the album row + the +// joined artist name in a single round trip. Replaces a sequential +// GetAlbumByID + GetArtistByID pair on the hot detail-page path. +func (q *Queries) GetAlbumWithArtist(ctx context.Context, id pgtype.UUID) (GetAlbumWithArtistRow, error) { + row := q.db.QueryRow(ctx, getAlbumWithArtist, id) + var i GetAlbumWithArtistRow + err := row.Scan( + &i.Album.ID, + &i.Album.Title, + &i.Album.SortTitle, + &i.Album.ArtistID, + &i.Album.ReleaseDate, + &i.Album.Mbid, + &i.Album.CoverArtPath, + &i.Album.CreatedAt, + &i.Album.UpdatedAt, + &i.Album.CoverArtSource, + &i.Album.CoverArtSourcesVersion, + &i.ArtistName, + ) + return i, err +} + const getAlbumsByIDs = `-- name: GetAlbumsByIDs :many SELECT id, title, sort_title, artist_id, release_date, mbid, cover_art_path, created_at, updated_at, cover_art_source, cover_art_sources_version FROM albums WHERE id = ANY($1::uuid[]) ` @@ -389,6 +424,56 @@ func (q *Queries) ListAlbumsByArtist(ctx context.Context, artistID pgtype.UUID) return items, nil } +const listAlbumsByArtistWithTrackCount = `-- name: ListAlbumsByArtistWithTrackCount :many +SELECT albums.id, albums.title, albums.sort_title, albums.artist_id, albums.release_date, albums.mbid, albums.cover_art_path, albums.created_at, albums.updated_at, albums.cover_art_source, albums.cover_art_sources_version, + (SELECT count(*) FROM tracks t WHERE t.album_id = albums.id)::bigint + AS track_count +FROM albums +WHERE albums.artist_id = $1 +ORDER BY release_date NULLS LAST, sort_title +` + +type ListAlbumsByArtistWithTrackCountRow struct { + Album Album + TrackCount int64 +} + +// Replaces the N+1 pattern in handleGetArtist (1 + N CountTracksByAlbum +// per album). Returns each album joined with its track count via a +// correlated subquery — single round trip regardless of album count. +func (q *Queries) ListAlbumsByArtistWithTrackCount(ctx context.Context, artistID pgtype.UUID) ([]ListAlbumsByArtistWithTrackCountRow, error) { + rows, err := q.db.Query(ctx, listAlbumsByArtistWithTrackCount, artistID) + if err != nil { + return nil, err + } + defer rows.Close() + var items []ListAlbumsByArtistWithTrackCountRow + for rows.Next() { + var i ListAlbumsByArtistWithTrackCountRow + if err := rows.Scan( + &i.Album.ID, + &i.Album.Title, + &i.Album.SortTitle, + &i.Album.ArtistID, + &i.Album.ReleaseDate, + &i.Album.Mbid, + &i.Album.CoverArtPath, + &i.Album.CreatedAt, + &i.Album.UpdatedAt, + &i.Album.CoverArtSource, + &i.Album.CoverArtSourcesVersion, + &i.TrackCount, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const listAlbumsByGenre = `-- name: ListAlbumsByGenre :many SELECT DISTINCT ON (albums.id) albums.id, albums.title, albums.sort_title, albums.artist_id, albums.release_date, albums.mbid, albums.cover_art_path, albums.created_at, albums.updated_at, albums.cover_art_source, albums.cover_art_sources_version FROM albums diff --git a/internal/db/dbq/recommendation.sql.go b/internal/db/dbq/recommendation.sql.go index e5ca6ba6..fc653a78 100644 --- a/internal/db/dbq/recommendation.sql.go +++ b/internal/db/dbq/recommendation.sql.go @@ -12,17 +12,19 @@ import ( ) const listLastPlayedArtistsForUser = `-- name: ListLastPlayedArtistsForUser :many -SELECT a.id, a.name, a.sort_name, a.mbid, a.created_at, a.updated_at, a.artist_thumb_path, a.artist_fanart_path, a.artist_art_source, a.artist_art_sources_version, - cov.id AS cover_album_id, - cnt.album_count::bigint AS album_count, - max_started.started_at::timestamptz AS last_played_at -FROM artists a -JOIN LATERAL ( - SELECT max(pe.started_at) AS started_at +WITH user_plays AS ( + SELECT t.artist_id, max(pe.started_at) AS last_started FROM play_events pe JOIN tracks t ON t.id = pe.track_id - WHERE pe.user_id = $1 AND t.artist_id = a.id -) max_started ON max_started.started_at IS NOT NULL + WHERE pe.user_id = $1 + GROUP BY t.artist_id +) +SELECT a.id, a.name, a.sort_name, a.mbid, a.created_at, a.updated_at, a.artist_thumb_path, a.artist_fanart_path, a.artist_art_source, a.artist_art_sources_version, + cov.id AS cover_album_id, + cnt.album_count::bigint AS album_count, + up.last_started::timestamptz AS last_played_at +FROM user_plays up +JOIN artists a ON a.id = up.artist_id LEFT JOIN LATERAL ( SELECT id FROM albums WHERE artist_id = a.id AND cover_art_path IS NOT NULL @@ -32,7 +34,7 @@ LEFT JOIN LATERAL ( SELECT count(*) AS album_count FROM albums WHERE artist_id = a.id ) cnt ON true -ORDER BY max_started.started_at DESC, a.id +ORDER BY up.last_started DESC, a.id LIMIT $2 ` @@ -52,6 +54,15 @@ type ListLastPlayedArtistsForUserRow struct { // a derived cover_album_id via a representative-album lateral join (most // recent album that has cover_art_path set). album_count joined for the // ArtistRef wire shape. +// +// Earlier shape iterated `FROM artists a` and ran a LATERAL play_events +// subquery per artist — O(total_artists) plan even for users with a +// handful of plays. New shape aggregates the user's plays by artist +// via the play_events → tracks join up front (uses +// play_events_user_track_idx + tracks pkey lookups), then attaches +// the artist row and lateral cover/count subqueries only for the +// artists that actually appear. Distinct-artists set is small for a +// typical user, so cost is bounded by play history not library size. func (q *Queries) ListLastPlayedArtistsForUser(ctx context.Context, arg ListLastPlayedArtistsForUserParams) ([]ListLastPlayedArtistsForUserRow, error) { rows, err := q.db.Query(ctx, listLastPlayedArtistsForUser, arg.UserID, arg.Limit) if err != nil { @@ -87,24 +98,24 @@ func (q *Queries) ListLastPlayedArtistsForUser(ctx context.Context, arg ListLast } const listMostPlayedTracksForUser = `-- name: ListMostPlayedTracksForUser :many +WITH plays AS ( + SELECT track_id, count(*) AS cnt + FROM play_events + WHERE user_id = $1 AND was_skipped = false + GROUP BY track_id +) SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, albums.title AS album_title, artists.name AS artist_name -FROM tracks t -JOIN albums ON albums.id = t.album_id -JOIN artists ON artists.id = t.artist_id -JOIN play_events pe ON pe.track_id = t.id -WHERE pe.user_id = $1 - AND pe.was_skipped = false - AND NOT EXISTS ( - SELECT 1 FROM lidarr_quarantine q - WHERE q.user_id = $1 AND q.track_id = t.id - ) -GROUP BY t.id, t.title, t.album_id, t.artist_id, t.duration_ms, t.file_path, - t.file_format, t.file_size, t.bitrate, t.track_number, t.disc_number, - t.mbid, t.genre, t.added_at, t.updated_at, - albums.title, artists.name -ORDER BY count(*) DESC, t.id +FROM plays p +JOIN tracks t ON t.id = p.track_id +JOIN albums ON albums.id = t.album_id +JOIN artists ON artists.id = t.artist_id +WHERE NOT EXISTS ( + SELECT 1 FROM lidarr_quarantine q + WHERE q.user_id = $1 AND q.track_id = t.id +) +ORDER BY p.cnt DESC, t.id LIMIT $2 ` @@ -122,6 +133,11 @@ type ListMostPlayedTracksForUserRow struct { // M6a: top-N tracks by completed-play count for the user. was_skipped // excludes skips so a user spamming next doesn't fabricate a top track. // Quarantined tracks (per-user soft-hide from M5b) are filtered out. +// +// Aggregate play_events first (uses play_events_user_track_idx) and +// join through tracks/albums/artists only for the survivors. Earlier +// shape did the join across every play_event row before grouping — +// O(plays) instead of O(distinct tracks). func (q *Queries) ListMostPlayedTracksForUser(ctx context.Context, arg ListMostPlayedTracksForUserParams) ([]ListMostPlayedTracksForUserRow, error) { rows, err := q.db.Query(ctx, listMostPlayedTracksForUser, arg.UserID, arg.Limit) if err != nil { diff --git a/internal/db/queries/albums.sql b/internal/db/queries/albums.sql index 38f3c34a..9811a083 100644 --- a/internal/db/queries/albums.sql +++ b/internal/db/queries/albums.sql @@ -14,6 +14,26 @@ RETURNING *; -- name: GetAlbumByID :one SELECT * FROM albums WHERE id = $1; +-- name: GetAlbumWithArtist :one +-- Combined fetch for /api/albums/{id}: returns the album row + the +-- joined artist name in a single round trip. Replaces a sequential +-- GetAlbumByID + GetArtistByID pair on the hot detail-page path. +SELECT sqlc.embed(albums), artists.name AS artist_name +FROM albums +JOIN artists ON artists.id = albums.artist_id +WHERE albums.id = $1; + +-- name: ListAlbumsByArtistWithTrackCount :many +-- Replaces the N+1 pattern in handleGetArtist (1 + N CountTracksByAlbum +-- per album). Returns each album joined with its track count via a +-- correlated subquery — single round trip regardless of album count. +SELECT sqlc.embed(albums), + (SELECT count(*) FROM tracks t WHERE t.album_id = albums.id)::bigint + AS track_count +FROM albums +WHERE albums.artist_id = $1 +ORDER BY release_date NULLS LAST, sort_title; + -- name: GetAlbumByArtistAndTitle :one -- Scanner uses this for the no-mbid dedupe path: resolve-or-create. SELECT * FROM albums WHERE artist_id = $1 AND title = $2 LIMIT 1; diff --git a/internal/db/queries/recommendation.sql b/internal/db/queries/recommendation.sql index 34e3cd4e..b4cec84b 100644 --- a/internal/db/queries/recommendation.sql +++ b/internal/db/queries/recommendation.sql @@ -206,24 +206,29 @@ LIMIT $3; -- M6a: top-N tracks by completed-play count for the user. was_skipped -- excludes skips so a user spamming next doesn't fabricate a top track. -- Quarantined tracks (per-user soft-hide from M5b) are filtered out. +-- +-- Aggregate play_events first (uses play_events_user_track_idx) and +-- join through tracks/albums/artists only for the survivors. Earlier +-- shape did the join across every play_event row before grouping — +-- O(plays) instead of O(distinct tracks). +WITH plays AS ( + SELECT track_id, count(*) AS cnt + FROM play_events + WHERE user_id = $1 AND was_skipped = false + GROUP BY track_id +) SELECT sqlc.embed(t), albums.title AS album_title, artists.name AS artist_name -FROM tracks t -JOIN albums ON albums.id = t.album_id -JOIN artists ON artists.id = t.artist_id -JOIN play_events pe ON pe.track_id = t.id -WHERE pe.user_id = $1 - AND pe.was_skipped = false - AND NOT EXISTS ( - SELECT 1 FROM lidarr_quarantine q - WHERE q.user_id = $1 AND q.track_id = t.id - ) -GROUP BY t.id, t.title, t.album_id, t.artist_id, t.duration_ms, t.file_path, - t.file_format, t.file_size, t.bitrate, t.track_number, t.disc_number, - t.mbid, t.genre, t.added_at, t.updated_at, - albums.title, artists.name -ORDER BY count(*) DESC, t.id +FROM plays p +JOIN tracks t ON t.id = p.track_id +JOIN albums ON albums.id = t.album_id +JOIN artists ON artists.id = t.artist_id +WHERE NOT EXISTS ( + SELECT 1 FROM lidarr_quarantine q + WHERE q.user_id = $1 AND q.track_id = t.id +) +ORDER BY p.cnt DESC, t.id LIMIT $2; -- name: ListLastPlayedArtistsForUser :many @@ -231,17 +236,28 @@ LIMIT $2; -- a derived cover_album_id via a representative-album lateral join (most -- recent album that has cover_art_path set). album_count joined for the -- ArtistRef wire shape. -SELECT sqlc.embed(a), - cov.id AS cover_album_id, - cnt.album_count::bigint AS album_count, - max_started.started_at::timestamptz AS last_played_at -FROM artists a -JOIN LATERAL ( - SELECT max(pe.started_at) AS started_at +-- +-- Earlier shape iterated `FROM artists a` and ran a LATERAL play_events +-- subquery per artist — O(total_artists) plan even for users with a +-- handful of plays. New shape aggregates the user's plays by artist +-- via the play_events → tracks join up front (uses +-- play_events_user_track_idx + tracks pkey lookups), then attaches +-- the artist row and lateral cover/count subqueries only for the +-- artists that actually appear. Distinct-artists set is small for a +-- typical user, so cost is bounded by play history not library size. +WITH user_plays AS ( + SELECT t.artist_id, max(pe.started_at) AS last_started FROM play_events pe JOIN tracks t ON t.id = pe.track_id - WHERE pe.user_id = $1 AND t.artist_id = a.id -) max_started ON max_started.started_at IS NOT NULL + WHERE pe.user_id = $1 + GROUP BY t.artist_id +) +SELECT sqlc.embed(a), + cov.id AS cover_album_id, + cnt.album_count::bigint AS album_count, + up.last_started::timestamptz AS last_played_at +FROM user_plays up +JOIN artists a ON a.id = up.artist_id LEFT JOIN LATERAL ( SELECT id FROM albums WHERE artist_id = a.id AND cover_art_path IS NOT NULL @@ -251,7 +267,7 @@ LEFT JOIN LATERAL ( SELECT count(*) AS album_count FROM albums WHERE artist_id = a.id ) cnt ON true -ORDER BY max_started.started_at DESC, a.id +ORDER BY up.last_started DESC, a.id LIMIT $2; -- name: ListRediscoverAlbumsForUser :many