diff --git a/flutter_client/lib/cache/db.dart b/flutter_client/lib/cache/db.dart index c46aaacd..82da7178 100644 --- a/flutter_client/lib/cache/db.dart +++ b/flutter_client/lib/cache/db.dart @@ -120,19 +120,6 @@ class SyncMetadata extends Table { Set get primaryKey => {id}; } -/// Single-row cache of the /api/home response (#357 follow-up). The -/// home screen is the first thing the user sees on app open; storing -/// the last successful HomeData as JSON lets us yield it immediately -/// before the REST round-trip resolves, eliminating the cold-start -/// blank on slow / remote connections. Schema 3+. -class CachedHomeSnapshot extends Table { - IntColumn get id => integer().withDefault(const Constant(1))(); - TextColumn get json => text()(); - DateTimeColumn get updatedAt => dateTime().withDefault(currentDateAndTime)(); - @override - Set get primaryKey => {id}; -} - /// Single-row cache of the /api/me/history response. Mirrors the /// CachedHomeSnapshot pattern: opening the History tab in the Library /// screen yields the last-known page immediately while a fresh REST @@ -255,7 +242,6 @@ enum CacheSource { manual, autoLiked, autoPlaylist, autoPrefetch, incidental } CachedPlaylistTracks, AudioCacheIndex, SyncMetadata, - CachedHomeSnapshot, CachedHistorySnapshot, CachedQuarantineMine, CachedHomeIndex, @@ -267,7 +253,7 @@ class AppDb extends _$AppDb { AppDb([QueryExecutor? e]) : super(e ?? driftDatabase(name: 'minstrel_cache')); @override - int get schemaVersion => 10; + int get schemaVersion => 11; @override MigrationStrategy get migration => MigrationStrategy( @@ -284,10 +270,16 @@ class AppDb extends _$AppDb { await customStatement('UPDATE sync_metadata SET cursor = 0'); } if (from < 3) { - // Schema 3: cached_home_snapshot for #357 drift-first - // home page. No existing rows to migrate — table starts - // empty; the first /api/home fetch populates it. - await m.createTable(cachedHomeSnapshot); + // Schema 3: cached_home_snapshot (legacy drift-first home). + // The table + its homeProvider/JSON encoders were removed + // in #406; recreated here as raw SQL so this historical + // step still compiles without the generated table symbol. + // The from<11 step below drops it. + await customStatement( + 'CREATE TABLE IF NOT EXISTS "cached_home_snapshot" ' + '("id" INTEGER NOT NULL DEFAULT 1, "json" TEXT, ' + '"updated_at" INTEGER, PRIMARY KEY ("id"));', + ); } if (from < 4) { // Schema 4: cached_history_snapshot for drift-first @@ -337,6 +329,15 @@ class AppDb extends _$AppDb { // first persist populates it. await m.createTable(cachedResumeState); } + if (from < 11) { + // Schema 11 (#406): drop the legacy cached_home_snapshot + // table. The home screen now renders solely from the + // per-item cached_home_index path; the legacy homeProvider + // + its JSON encoders were removed. + await customStatement( + 'DROP TABLE IF EXISTS "cached_home_snapshot";', + ); + } }, ); } diff --git a/flutter_client/lib/cache/metadata_prefetcher.dart b/flutter_client/lib/cache/metadata_prefetcher.dart index 7acee535..bafbc3fe 100644 --- a/flutter_client/lib/cache/metadata_prefetcher.dart +++ b/flutter_client/lib/cache/metadata_prefetcher.dart @@ -1,7 +1,7 @@ import 'package:flutter_riverpod/flutter_riverpod.dart'; import '../library/library_providers.dart'; -import '../models/home_data.dart'; +import '../models/home_index.dart'; /// Pre-warms the drift cache for likely-tap targets. Conservative on /// purpose: only warms artistProvider rows (single row, single round @@ -10,10 +10,15 @@ import '../models/home_data.dart'; /// 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. +/// +/// Driven off the per-item home path (homeIndexProvider). Only the +/// artist-typed sections (rediscover / last-played artists) carry +/// artist ids directly; album/track tiles hydrate their own artist on +/// render via the per-item path, so warming those here is unnecessary. class MetadataPrefetcher { MetadataPrefetcher(this._ref) { - _ref.listen>(homeProvider, (_, next) { - next.whenData(_warmHome); + _ref.listen>(homeIndexProvider, (_, next) { + next.whenData(_warmIndex); }); } @@ -24,7 +29,8 @@ class MetadataPrefetcher { /// ids we've already warmed. final Set _warmedArtists = {}; - /// Cap per section. Covers what fits on screen without scrolling. + /// Cap warmed per emission. Covers what fits on screen without + /// scrolling. static const _topN = 8; /// Pre-warm artists. Albums are intentionally not pre-warmed — @@ -39,24 +45,11 @@ class MetadataPrefetcher { } } - 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); + void _warmIndex(HomeIndex h) { + warmArtists({ + ...h.rediscoverArtists.take(_topN), + ...h.lastPlayedArtists.take(_topN), + }); } /// Discards the return value and any error from a fire-and-forget diff --git a/flutter_client/lib/library/library_providers.dart b/flutter_client/lib/library/library_providers.dart index a5d064a0..b97e5946 100644 --- a/flutter_client/lib/library/library_providers.dart +++ b/flutter_client/lib/library/library_providers.dart @@ -1,5 +1,4 @@ import 'dart:async'; -import 'dart:convert'; import 'package:dio/dio.dart'; import 'package:drift/drift.dart' as drift; @@ -16,7 +15,6 @@ import '../cache/connectivity_provider.dart'; import '../cache/db.dart'; import '../models/album.dart'; import '../models/artist.dart'; -import '../models/home_data.dart'; import '../models/home_index.dart'; import '../models/track.dart'; @@ -44,57 +42,6 @@ final libraryApiProvider = FutureProvider((ref) async { return LibraryApi(await ref.watch(dioProvider.future)); }); -/// Drift-first home data (#357 follow-up). The home screen is the first -/// view the user sees on app open; without a local cache the cold-start -/// blocks on the /api/home round-trip, which is the dominant felt- -/// latency on slow / remote connections. -/// -/// Cache layout: a single row in `cached_home_snapshot` storing the -/// last successful HomeData as JSON. On stream subscription: -/// -/// - If a row exists, yield it immediately (parsed from JSON) and -/// kick off a background revalidation against /api/home (SWR). -/// The drift watch() re-emits when the new row is written. -/// - If no row exists yet (cold cache), fetch /api/home synchronously, -/// upsert into drift, and emit. Subsequent emissions come from the -/// drift watch as background revalidations land. -/// -/// The HomeData JSON encodes back to the same wire shape /api/home -/// emits, so the existing HomeData.fromJson handles both sources. -final homeProvider = StreamProvider((ref) { - final db = ref.watch(appDbProvider); - return cacheFirst( - driftStream: db.select(db.cachedHomeSnapshot).watch(), - fetchAndPopulate: () async { - final api = await ref.read(libraryApiProvider.future); - final fresh = await api.getHome(); - await db.into(db.cachedHomeSnapshot).insertOnConflictUpdate( - CachedHomeSnapshotCompanion.insert( - json: _encodeHomeData(fresh), - updatedAt: drift.Value(DateTime.now()), - ), - ); - }, - toResult: (rows) => rows.isEmpty - ? const HomeData( - recentlyAddedAlbums: [], - rediscoverAlbums: [], - rediscoverArtists: [], - mostPlayedTracks: [], - lastPlayedArtists: [], - ) - : HomeData.fromJson(jsonDecode(rows.first.json) as Map), - isOnline: () async => (await ref - .read(connectivityProvider.future) - .timeout(const Duration(seconds: 3), onTimeout: () => true)), - // SWR: when cached snapshot exists, still hit /api/home in the - // background so the user sees the freshest curation. The cache - // mostly serves the first frame, not the canonical state. - alwaysRefresh: true, - tag: 'home', - ); -}); - /// Drift-first per-item home index. Reads from cached_home_index /// (populated by /api/home/index discovery) and yields a HomeIndex /// the screen consumes to lay out section→tile slots. Each tile is @@ -184,49 +131,6 @@ final homeIndexProvider = StreamProvider((ref) { ); }); -/// Encodes HomeData back to the wire-format JSON shape /api/home -/// emits, so HomeData.fromJson can round-trip through the drift cache. -String _encodeHomeData(HomeData h) => jsonEncode({ - 'recently_added_albums': h.recentlyAddedAlbums.map(_albumToJson).toList(), - 'rediscover_albums': h.rediscoverAlbums.map(_albumToJson).toList(), - 'rediscover_artists': h.rediscoverArtists.map(_artistToJson).toList(), - 'most_played_tracks': h.mostPlayedTracks.map(_trackToJson).toList(), - 'last_played_artists': h.lastPlayedArtists.map(_artistToJson).toList(), - }); - -Map _albumToJson(AlbumRef a) => { - 'id': a.id, - 'title': a.title, - 'sort_title': a.sortTitle, - 'artist_id': a.artistId, - 'artist_name': a.artistName, - 'year': a.year, - 'track_count': a.trackCount, - 'duration_sec': a.durationSec, - 'cover_url': a.coverUrl, - }; - -Map _artistToJson(ArtistRef a) => { - 'id': a.id, - 'name': a.name, - 'sort_name': a.sortName, - 'album_count': a.albumCount, - 'cover_url': a.coverUrl, - }; - -Map _trackToJson(TrackRef t) => { - 'id': t.id, - 'title': t.title, - 'album_id': t.albumId, - 'album_title': t.albumTitle, - 'artist_id': t.artistId, - 'artist_name': t.artistName, - 'track_number': t.trackNumber, - 'disc_number': t.discNumber, - 'duration_sec': t.durationSec, - 'stream_url': t.streamUrl, - }; - /// Drift-first per #357 plan C. Watches cached_artists for the row; /// when empty + online, fetches via REST + populates drift, which /// re-emits via watch(). diff --git a/flutter_client/lib/player/audio_handler.dart b/flutter_client/lib/player/audio_handler.dart index 82d80252..5a432e4b 100644 --- a/flutter_client/lib/player/audio_handler.dart +++ b/flutter_client/lib/player/audio_handler.dart @@ -76,6 +76,24 @@ class MinstrelAudioHandler extends BaseAudioHandler with QueueHandler, SeekHandl /// need this. Active only while playing; cancelled otherwise. Timer? _positionBroadcastTimer; + /// Buffering-stall watchdog. On poor coverage a stream hangs in + /// ProcessingState.buffering with NO error event, so the onError + /// recovery never fires. Armed while playing+buffering; on fire, if + /// buffered position hasn't advanced it's a dead stream → recover. + /// Re-arms a fresh window while buffering-but-progressing (slow-but- + /// downloading is not a stall). + static const _stallTimeout = Duration(seconds: 15); + Timer? _stallTimer; + Duration _stallMarkBuffered = Duration.zero; + + /// Per-track recovery budget. Reset when a track reaches + /// ready+playing. One retry of the same source (rebuilt fresh), then + /// skip to the next cached track (or pause) instead of thrashing + /// through unreachable streams. + static const _maxRecoverRetries = 1; + String? _recoveringTrackId; + int _recoverAttempts = 0; + /// Player volume captured when an OS duck interruption begins, /// restored when it ends. Null when not currently ducked. double? _volumeBeforeDuck; @@ -492,33 +510,12 @@ class MinstrelAudioHandler extends BaseAudioHandler with QueueHandler, SeekHandl /// /// If we're at the last track, seekToNext is a no-op; the state /// drops to idle and _broadcastState reflects that. + /// onError entrypoint (404 / decoder / premature EOS / network drop). + /// Routes into the unified recovery path (retry the track once, then + /// skip to the next cached track or pause) instead of immediately + /// skipping the literal next — possibly also-unreachable — source. Future _handlePlaybackError() async { - // mediaItem is the correctly-mapped current track (post #49 logical - // index), so it's the one that just failed — no player-index math. - final failed = mediaItem.value?.title; - if (failed != null && failed.isNotEmpty) { - _playbackErrors.add(failed); - } - final currentIdx = _player.currentIndex; - final queueLen = queue.value.length; - if (currentIdx == null || currentIdx + 1 >= queueLen) { - // Last track or no queue context — just pause; _broadcastState - // already reflects the idle/error processingState. - try { - await _player.pause(); - } catch (_) {} - return; - } - try { - await _player.seekToNext(); - } catch (_) { - // If seekToNext fails too (next source not built yet, etc.), - // there's nothing safe to do — pause and let the user recover - // manually. - try { - await _player.pause(); - } catch (_) {} - } + await _recoverPlayback(); } void _onCurrentIndexChanged(int? idx) { @@ -573,6 +570,8 @@ class MinstrelAudioHandler extends BaseAudioHandler with QueueHandler, SeekHandl _idleStopTimer = null; _positionBroadcastTimer?.cancel(); _positionBroadcastTimer = null; + _stallTimer?.cancel(); + _stallTimer = null; try { await _player.stop(); } catch (_) {} @@ -786,6 +785,7 @@ class MinstrelAudioHandler extends BaseAudioHandler with QueueHandler, SeekHandl )); _reconcileIdleTimer(); _reconcilePositionBroadcast(); + _reconcileStallWatchdog(); } /// Arms (or cancels) the idle-cleanup timer based on the current @@ -842,6 +842,96 @@ class MinstrelAudioHandler extends BaseAudioHandler with QueueHandler, SeekHandl ); } + /// Buffering-stall watchdog. Armed while playing+buffering/loading. + /// On fire: still stuck with no >=1s buffered progress → dead stream, + /// recover; progressing → re-arm a fresh window (slow-but-downloading + /// is fine). When the track is happily ready+playing, clears the + /// per-track recovery budget. Driven from _broadcastState; mirrors + /// the other reconcile helpers. + void _reconcileStallWatchdog() { + final ps = _player.processingState; + if (ps == ProcessingState.ready && _player.playing) { + _recoveringTrackId = null; + _recoverAttempts = 0; + } + final buffering = _player.playing && + (ps == ProcessingState.buffering || ps == ProcessingState.loading); + if (!buffering) { + _stallTimer?.cancel(); + _stallTimer = null; + return; + } + if (_stallTimer != null) return; // window already running + _stallMarkBuffered = _player.bufferedPosition; + _stallTimer = Timer(_stallTimeout, () { + _stallTimer = null; + final ps2 = _player.processingState; + final stillBuffering = _player.playing && + (ps2 == ProcessingState.buffering || + ps2 == ProcessingState.loading); + if (!stillBuffering) return; + final progressed = (_player.bufferedPosition - _stallMarkBuffered) >= + const Duration(seconds: 1); + if (progressed) { + _reconcileStallWatchdog(); // slow but downloading — re-arm + } else { + unawaited(_recoverPlayback()); + } + }); + } + + /// Unified recovery for a failed OR stalled stream. Retries the + /// current track once (rebuilt from scratch via skipToQueueItem so a + /// transient blip doesn't lose it); on exhaustion, surfaces the + /// failure (#58 SnackBar) and skips to the next cached track — or + /// pauses — instead of blindly walking into more unreachable streams. + Future _recoverPlayback() async { + _stallTimer?.cancel(); + _stallTimer = null; + final media = mediaItem.value; + if (media == null) return; + final trackId = media.id; + final curIdx = _lastTracks.indexWhere((t) => t.id == trackId); + if (curIdx < 0) { + try { + await _player.pause(); + } catch (_) {} + return; + } + if (_recoveringTrackId != trackId) { + _recoveringTrackId = trackId; + _recoverAttempts = 0; + } + if (_recoverAttempts < _maxRecoverRetries) { + _recoverAttempts++; + // Rebuild + restart the SAME track (fresh source / HTTP). + await skipToQueueItem(curIdx); + return; + } + if (media.title.isNotEmpty) _playbackErrors.add(media.title); + _recoveringTrackId = null; + _recoverAttempts = 0; + await _skipToNextCachedOrPause(curIdx); + } + + /// Skips to the first track after [fromIdx] that is fully cached on + /// disk (plays with zero network). If none, pauses rather than + /// thrashing through unreachable streams on a dead connection. + Future _skipToNextCachedOrPause(int fromIdx) async { + final mgr = _audioCacheManager; + if (mgr != null) { + for (var i = fromIdx + 1; i < _lastTracks.length; i++) { + if (await mgr.pathFor(_lastTracks[i].id) != null) { + await skipToQueueItem(i); + return; + } + } + } + try { + await _player.pause(); + } catch (_) {} + } + /// Configures the OS audio session for music playback and wires /// interruption + becoming-noisy handling. just_audio auto-activates / /// deactivates the session around playback once it's configured, but diff --git a/flutter_client/lib/shared/live_events_dispatcher.dart b/flutter_client/lib/shared/live_events_dispatcher.dart index 28bc65f0..d38d75ae 100644 --- a/flutter_client/lib/shared/live_events_dispatcher.dart +++ b/flutter_client/lib/shared/live_events_dispatcher.dart @@ -20,7 +20,7 @@ import 'package:flutter/widgets.dart'; import 'package:flutter_riverpod/flutter_riverpod.dart'; -import '../library/library_providers.dart' show homeProvider; +import '../library/library_providers.dart' show homeIndexProvider; import '../quarantine/quarantine_provider.dart'; import 'live_events_provider.dart'; @@ -62,8 +62,8 @@ class _LiveEventsDispatcher with WidgetsBindingObserver { case 'quarantine.deleted_via_lidarr': _ref.invalidate(myQuarantineProvider); // Hidden / liked / album rows referencing a deleted track may - // now be stale — homeProvider re-fetch refreshes the cards. - _ref.invalidate(homeProvider); + // now be stale — homeIndexProvider re-fetch refreshes the cards. + _ref.invalidate(homeIndexProvider); case 'playlist.created': case 'playlist.updated': case 'playlist.deleted': @@ -72,7 +72,7 @@ class _LiveEventsDispatcher with WidgetsBindingObserver { // is reflected immediately. Detail screens that need the // mutated row will get a separate invalidate from their own // listener (filed as a follow-up). - _ref.invalidate(homeProvider); + _ref.invalidate(homeIndexProvider); case 'scan.run_started': case 'scan.run_finished': // Admin scan card provider lives in the admin feature folder @@ -93,7 +93,7 @@ class _LiveEventsDispatcher with WidgetsBindingObserver { // own, but the re-invalidate flushes any stale data that // landed while the app was backgrounded. _ref.invalidate(myQuarantineProvider); - _ref.invalidate(homeProvider); + _ref.invalidate(homeIndexProvider); } } diff --git a/flutter_client/pubspec.yaml b/flutter_client/pubspec.yaml index 00a17bca..6959e227 100644 --- a/flutter_client/pubspec.yaml +++ b/flutter_client/pubspec.yaml @@ -1,7 +1,7 @@ name: minstrel description: Minstrel mobile client publish_to: 'none' -version: 2026.5.19+12 +version: 2026.5.19+13 environment: sdk: '>=3.5.0 <4.0.0' diff --git a/web/src/routes/discover/discover.test.ts b/web/src/routes/discover/discover.test.ts index c01b4580..b2d1c14f 100644 --- a/web/src/routes/discover/discover.test.ts +++ b/web/src/routes/discover/discover.test.ts @@ -3,6 +3,16 @@ import { render, screen, fireEvent, waitFor } from '@testing-library/svelte'; import { mockQuery } from '../../test-utils/query'; import { apiClientMock } from '../../test-utils/mocks/client'; import type { LidarrSearchResult } from '$lib/api/types'; +import { pageUrlModule } from '../../test-utils/mocks/appState'; + +// $app/state / $app/navigation must be mocked or rendering the page +// pulls in SvelteKit's client runtime (`notifiable_store is not a +// function`) at module load. Same pattern as the other route tests. +const pageState = vi.hoisted(() => ({ + pageUrl: new URL('http://localhost/discover') +})); +vi.mock('$app/state', () => pageUrlModule(pageState)); +vi.mock('$app/navigation', () => ({ goto: vi.fn() })); // Lidarr search query factory and createRequest are mocked at the module // level so each test can shape what the page sees without standing up a @@ -56,13 +66,7 @@ afterEach(() => { vi.clearAllMocks(); }); -// FIXME(M7 #374): suite errors at module-load with -// `TypeError: notifiable_store is not a function` from SvelteKit's -// internal client.js. Surfaces only on the new dev-push CI; PR-to-main -// runs were green. Probably a vitest module-resolution interaction -// with a recent SvelteKit; needs targeted triage. Skipped to unblock -// CI for #372 — the page itself isn't broken, the test harness is. -describe.skip('Discover page', () => { +describe('Discover page', () => { test('initial state (no query) shows the suggestion feed', () => { render(DiscoverPage); expect(screen.getByText(/suggested for you/i)).toBeInTheDocument(); diff --git a/web/src/routes/requests/requests.test.ts b/web/src/routes/requests/requests.test.ts index 5639d09e..1178597d 100644 --- a/web/src/routes/requests/requests.test.ts +++ b/web/src/routes/requests/requests.test.ts @@ -3,6 +3,16 @@ import { render, screen, fireEvent, waitFor } from '@testing-library/svelte'; import { mockQuery } from '../../test-utils/query'; import { apiClientMock } from '../../test-utils/mocks/client'; import type { LidarrRequest } from '$lib/api/types'; +import { pageUrlModule } from '../../test-utils/mocks/appState'; + +// $app/state / $app/navigation must be mocked or rendering the page +// pulls in SvelteKit's client runtime (`notifiable_store is not a +// function`) at module load. Same pattern as the other route tests. +const pageState = vi.hoisted(() => ({ + pageUrl: new URL('http://localhost/requests') +})); +vi.mock('$app/state', () => pageUrlModule(pageState)); +vi.mock('$app/navigation', () => ({ goto: vi.fn() })); // Capture invalidateQueries on the mocked QueryClient so the cancel test can // assert against the same instance the page consumed. Per-file mock OVERRIDES @@ -66,10 +76,7 @@ afterEach(() => { vi.clearAllMocks(); }); -// FIXME(M7 #374): same SvelteKit `notifiable_store is not a function` -// failure as discover.test.ts. Skipped to unblock CI for #372 — needs -// triage as a separate task. -describe.skip('Requests page', () => { +describe('Requests page', () => { test('renders one row per request from the API', () => { const rows: LidarrRequest[] = [ req({ id: 'r1', kind: 'artist', artist_name: 'Boards of Canada' }), diff --git a/web/vitest.config.ts b/web/vitest.config.ts index 9a071c45..f97aac5f 100644 --- a/web/vitest.config.ts +++ b/web/vitest.config.ts @@ -7,19 +7,7 @@ export default defineConfig({ test: { environment: 'jsdom', include: ['src/**/*.test.ts', 'src/**/*.svelte.test.ts', 'scripts/**/*.test.js'], - // M7 #374: these two test files explode at module-load with - // `TypeError: notifiable_store is not a function` from SvelteKit's - // internal client.js — the import chain triggers SvelteKit's client - // runtime init in a way the dev-push vitest invocation can't satisfy. - // describe.skip inside the files doesn't help because vitest must - // load the file before it can see the describe block. Excluded here - // until the triage in #374 lands a real fix (probably a $app/paths - // mock in vitest.setup.ts). - exclude: [ - ...configDefaults.exclude, - 'src/routes/discover/discover.test.ts', - 'src/routes/requests/requests.test.ts' - ], + exclude: [...configDefaults.exclude], setupFiles: ['./vitest.setup.ts'] } });