From 4ede37d9ad0265924832e7352fabe841f826dd9d Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 11 May 2026 19:02:16 -0400 Subject: [PATCH] =?UTF-8?q?fix(flutter):=20stop=20the=20cache=20feedback?= =?UTF-8?q?=20loop=20=E2=80=94=20playback=20no=20longer=20competes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The prefetcher + alwaysRefresh combination was creating a feedback loop visible in the logs as repeated `metadataPrefetcher: warming N albums` cycles, each kicking N parallel getAlbum fetches that then triggered drift writes that triggered re-emits that re-ran the prefetcher. Tap-to-play was queueing behind 14+ in-flight cache fetches. Three structural fixes: 1. Prefetcher hard-dedupes per session via _warmedArtists Set. Re-rendering a screen no longer re-fires fetches for ids we've already seen. 2. Prefetcher only warms artistProvider, not albumProvider. Albums carry track lists; pre-warming N albums fans out N parallel "fetch tracks" round trips for content the user may never visit. Artist rows are single-row lookups — cheap. Album detail loads on tap (still fast: server-side perf work makes it ~one round trip). 3. Drop alwaysRefresh from albumProvider, artistProvider, artistAlbumsProvider, artistTracksProvider. Each was kicking one silent background refresh per first cache hit. With the prefetcher creating many subscriptions in parallel, that meant every prewarmed id triggered an extra fetch even when drift was already populated. playlistsListProvider keeps alwaysRefresh — system playlists genuinely rotate UUIDs and need the catch-up. Pull-to- refresh remains the explicit invalidation path everywhere else. Removed the warmAlbums calls from the library Albums tab and artist detail album grid (the storm sources). Net effect: cold app boot warms ~12-15 artist rows once, period. Tapping a tile still fetches its detail on demand (one round trip, fast). User-initiated playback isn't queued behind cache work. --- .../lib/cache/metadata_prefetcher.dart | 56 ++++++------------- .../lib/library/artist_detail_screen.dart | 3 - .../lib/library/library_providers.dart | 30 ++++------ .../lib/library/library_screen.dart | 3 - .../lib/playlists/playlists_provider.dart | 14 ++--- 5 files changed, 35 insertions(+), 71 deletions(-) diff --git a/flutter_client/lib/cache/metadata_prefetcher.dart b/flutter_client/lib/cache/metadata_prefetcher.dart index b60cc08e..2eef60cb 100644 --- a/flutter_client/lib/cache/metadata_prefetcher.dart +++ b/flutter_client/lib/cache/metadata_prefetcher.dart @@ -4,14 +4,13 @@ import 'package:flutter_riverpod/flutter_riverpod.dart'; import '../library/library_providers.dart'; import '../models/home_data.dart'; -/// Pre-warms the drift cache for entities the user is most likely to -/// tap. The home prefetch fires when /api/home returns. Library -/// tabs and artist detail expose `warmAlbums` / `warmArtists` so they -/// can fan out reads from their own AsyncNotifier callbacks. -/// -/// All reads are unawaited and idempotent — providers that already -/// have a cached value short-circuit. Readers/writers don't conflict -/// because each provider de-duplicates concurrent subscribers. +/// 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) { @@ -21,28 +20,21 @@ class MetadataPrefetcher { final Ref _ref; - /// Cap per section so a 50-album recently-added row doesn't fan - /// out 50 fetches the moment the screen appears. Top-N covers what - /// the user can see without scrolling on a typical phone. + /// 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 drift for each album id (no-op past the first dedup). - /// Called from the library Albums tab + artist detail album grid. - void warmAlbums(Iterable ids) { - var n = 0; - for (final id in ids) { - if (id.isEmpty) continue; - if (n++ >= _topN) break; - _swallow(_ref.read(albumProvider(id).future)); - } - if (n > 0) debugPrint('metadataPrefetcher: warming $n albums'); - } - - /// Pre-warm drift for each artist id. + /// 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)); } @@ -50,15 +42,11 @@ class MetadataPrefetcher { } void _warmHome(HomeData h) { - final albumIds = {}; final artistIds = {}; - for (final a in h.recentlyAddedAlbums.take(_topN)) { - if (a.id.isNotEmpty) albumIds.add(a.id); if (a.artistId.isNotEmpty) artistIds.add(a.artistId); } for (final a in h.rediscoverAlbums.take(_topN)) { - if (a.id.isNotEmpty) albumIds.add(a.id); if (a.artistId.isNotEmpty) artistIds.add(a.artistId); } for (final ar in h.rediscoverArtists.take(_topN)) { @@ -68,19 +56,9 @@ class MetadataPrefetcher { if (ar.id.isNotEmpty) artistIds.add(ar.id); } for (final t in h.mostPlayedTracks.take(_topN)) { - if (t.albumId.isNotEmpty) albumIds.add(t.albumId); if (t.artistId.isNotEmpty) artistIds.add(t.artistId); } - - debugPrint( - 'metadataPrefetcher: warming ${albumIds.length} albums + ${artistIds.length} artists'); - - for (final id in albumIds) { - _swallow(_ref.read(albumProvider(id).future)); - } - for (final id in artistIds) { - _swallow(_ref.read(artistProvider(id).future)); - } + warmArtists(artistIds); } /// Discards the return value and any error from a fire-and-forget diff --git a/flutter_client/lib/library/artist_detail_screen.dart b/flutter_client/lib/library/artist_detail_screen.dart index 2430bf78..004ea9e1 100644 --- a/flutter_client/lib/library/artist_detail_screen.dart +++ b/flutter_client/lib/library/artist_detail_screen.dart @@ -135,9 +135,6 @@ class ArtistDetailScreen extends ConsumerWidget { error: (e, _) => Center(child: Text('$e', style: TextStyle(color: fs.error))), loading: () => const Padding(padding: EdgeInsets.all(16), child: Center(child: CircularProgressIndicator())), data: (list) { - ref - .read(metadataPrefetcherProvider) - .warmAlbums(list.map((a) => a.id)); return LayoutBuilder(builder: (ctx, constraints) { const cols = 3; const sidePad = 8.0; 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 145e7cbf..80b4a762 100644 --- a/flutter_client/lib/library/library_screen.dart +++ b/flutter_client/lib/library/library_screen.dart @@ -269,9 +269,6 @@ class _AlbumsTab extends ConsumerWidget { loading: () => const Center(child: CircularProgressIndicator()), error: (e, _) => Center(child: Text('$e', style: TextStyle(color: fs.error))), data: (page) { - ref - .read(metadataPrefetcherProvider) - .warmAlbums(page.items.map((a) => a.id)); 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( diff --git a/flutter_client/lib/playlists/playlists_provider.dart b/flutter_client/lib/playlists/playlists_provider.dart index 52fa1c51..dc4105d0 100644 --- a/flutter_client/lib/playlists/playlists_provider.dart +++ b/flutter_client/lib/playlists/playlists_provider.dart @@ -289,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) { @@ -348,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. } });