diff --git a/lib/main.dart b/lib/main.dart index 36fd0105..141b7060 100644 --- a/lib/main.dart +++ b/lib/main.dart @@ -854,7 +854,17 @@ class _MainAppState extends State with WidgetsBindingObserver { ChangeNotifierProvider(create: (context) => TraktAccountProvider()), ChangeNotifierProvider(create: (context) => TrackersProvider()), ChangeNotifierProvider(create: (context) => HiddenLibrariesProvider(), lazy: true), - ChangeNotifierProvider(create: (context) => LibrariesProvider()), + ChangeNotifierProvider( + create: (context) { + final provider = LibrariesProvider(); + // Reload libraries when a new server comes online. Servers bind in + // waves on sign-in / profile switch and slow ones reconnect after + // the initial load; without this they stay missing from the sidebar + // until a profile re-switch or restart. + context.read().onOnlineServersChanged = provider.syncToOnlineServers; + return provider; + }, + ), ChangeNotifierProvider(create: (context) => PlaybackStateProvider()), ChangeNotifierProvider(create: (context) => WatchTogetherProvider()), ChangeNotifierProvider(create: (context) => CompanionRemoteProvider()), diff --git a/lib/providers/libraries_provider.dart b/lib/providers/libraries_provider.dart index a6850f9d..3da3394d 100644 --- a/lib/providers/libraries_provider.dart +++ b/lib/providers/libraries_provider.dart @@ -23,6 +23,20 @@ class LibrariesProvider extends ChangeNotifier with DisposableChangeNotifierMixi /// see the same in-flight result instead of racing two separate fetches. Future? _inFlightLoad; + /// Server ids whose library fetch *succeeded* in the current [_libraries], as + /// reported by [DataAggregationService.getMediaLibrariesFromAllServers]. + /// Keyed on fetch success (not on which servers returned libraries) so a + /// server that genuinely has zero libraries still counts as loaded, while a + /// server whose fetch failed does not — the latter is retried on the next + /// status emission instead of being cached as "loaded" forever. Drives + /// [syncToOnlineServers]. + Set _loadedServerIds = {}; + + /// Set when a (re)load is requested while one is already in flight, so the + /// loop runs another pass: a server that comes online *during* a load would + /// otherwise be lost to the coalesced [_inFlightLoad]. + bool _hasPendingLoad = false; + /// Unmodifiable list of all libraries (filtered for supported types, ordered) List get libraries => List.unmodifiable(_libraries); @@ -47,30 +61,77 @@ class LibrariesProvider extends ChangeNotifier with DisposableChangeNotifierMixi _aggregationService = service; } - /// Load libraries from all connected servers. - /// Filters out music libraries and applies saved ordering. - Future loadLibraries() { - return _inFlightLoad ??= _loadLibrariesInternal().whenComplete(() => _inFlightLoad = null); + /// Reload libraries when the set of online servers has grown since the last + /// load. Servers connect in waves — the owner Plex account, then each + /// borrowed/shared connection, then Jellyfin, plus slow servers that + /// reconnect after timing out — and each wave must surface in the sidebar + /// without a profile re-switch or app restart. + /// + /// No-op when uninitialized, when [onlineServerIds] is empty, or when every + /// id is already represented in the current load. That last guard keeps the + /// many unrelated reasons the server-status stream fires (visibility churn, + /// auth errors, Live TV probes, a server going offline) from causing reload + /// storms. + Future syncToOnlineServers(Set onlineServerIds) { + if (_aggregationService == null || onlineServerIds.isEmpty) return Future.value(); + if (_loadState == LibrariesLoadState.loaded && _loadedServerIds.containsAll(onlineServerIds)) { + return Future.value(); + } + return _load(); } - Future _loadLibrariesInternal() async { + /// Load libraries from all connected servers, unconditionally. Used by + /// pull-to-refresh, inline connection-add, and library reordering. + /// Filters out music libraries and applies saved ordering. + Future loadLibraries() => _load(); + + /// Single entry point for every (re)load. Concurrent callers coalesce onto + /// one in-flight pass; a request that arrives mid-pass is replayed by + /// [_runLoadLoop] so it isn't masked by that coalescing. Each pass fetches + /// whatever is online at fetch time, so no caller needs to specify a target. + Future _load() { + _hasPendingLoad = true; + return _inFlightLoad ??= _runLoadLoop().whenComplete(() => _inFlightLoad = null); + } + + Future _runLoadLoop() async { + while (_hasPendingLoad) { + _hasPendingLoad = false; + final succeeded = await _loadLibrariesInternal(); + // Stop on failure so a persistently failing fetch can't hot-loop; the + // next server-status emission re-drives the sync. + if (!succeeded) break; + } + } + + /// Returns `true` on a successful load, `false` on error. + Future _loadLibrariesInternal() async { if (_aggregationService == null) { appLogger.w('LibrariesProvider: Cannot load libraries - not initialized'); - return; + return false; } - _loadState = LibrariesLoadState.loading; - _errorMessage = null; - safeNotifyListeners(); + // Reloading over an already-loaded list (a reactive server-connect sync, an + // inline connection add, a reorder) must not flip the UI back to a loading + // state: screens such as LibrariesScreen replace their whole body with a + // spinner whenever `isLoading` is true. Keep the current list visible and + // swap in the fuller one when the fetch completes; only the first load (or + // a reload after clear()/error) surfaces the spinner. + final reloadInPlace = _loadState == LibrariesLoadState.loaded; + if (!reloadInPlace) { + _loadState = LibrariesLoadState.loading; + _errorMessage = null; + safeNotifyListeners(); + } try { // Fetch libraries from every connected backend (Plex + Jellyfin). // The aggregation service converts Plex-typed responses to MediaLibrary // internally; Jellyfin clients return MediaLibrary natively. - final allLibraries = await _aggregationService!.getMediaLibrariesFromAllServers(); + final result = await _aggregationService!.getMediaLibrariesFromAllServers(); // Filter out music libraries (not supported) - final filteredLibraries = allLibraries.where((lib) => !ContentTypeHelper.isMusicLibrary(lib)).toList(); + final filteredLibraries = result.libraries.where((lib) => !ContentTypeHelper.isMusicLibrary(lib)).toList(); // Apply saved library order final storage = await StorageService.getInstance(); @@ -78,16 +139,28 @@ class LibrariesProvider extends ChangeNotifier with DisposableChangeNotifierMixi final orderedLibraries = _applyLibraryOrder(filteredLibraries, savedOrder); _libraries = orderedLibraries; + // Track which servers actually responded so [syncToOnlineServers] can tell + // a genuinely new server from one already covered. Keyed on fetch success + // (not on which servers returned libraries) so a zero-library server still + // counts as loaded, while a server whose fetch failed is left out and + // retried on the next status emission. + _loadedServerIds = result.succeededServerIds; _loadState = LibrariesLoadState.loaded; _errorMessage = null; appLogger.i('LibrariesProvider: Loaded ${_libraries.length} libraries'); safeNotifyListeners(); + return true; } catch (e, stackTrace) { appLogger.e('LibrariesProvider: Failed to load libraries', error: e, stackTrace: stackTrace); + // A refresh that fails over an existing list keeps the last good data and + // `loaded` state instead of blanking to an error screen; the next status + // emission re-drives the sync. + if (reloadInPlace) return false; _loadState = LibrariesLoadState.error; _errorMessage = e.toString(); safeNotifyListeners(); + return false; } } @@ -118,6 +191,8 @@ class LibrariesProvider extends ChangeNotifier with DisposableChangeNotifierMixi _libraries = []; _loadState = LibrariesLoadState.initial; _errorMessage = null; + _loadedServerIds = {}; + _hasPendingLoad = false; safeNotifyListeners(); appLogger.d('LibrariesProvider: Cleared library data'); } diff --git a/lib/providers/multi_server_provider.dart b/lib/providers/multi_server_provider.dart index caaeb3e9..319b5ab8 100644 --- a/lib/providers/multi_server_provider.dart +++ b/lib/providers/multi_server_provider.dart @@ -40,6 +40,14 @@ class MultiServerProvider extends ChangeNotifier with DisposableChangeNotifierMi /// Previously-seen set of online server IDs, used to detect new servers Set _previousOnlineServerIds = {}; + /// Invoked with the current visibility-filtered online server ids whenever + /// the manager's status stream fires (a server connects, reconnects, drops, + /// or its auth state changes). Lets `LibrariesProvider` reload when the + /// online set grows — servers bind in waves and slow ones reconnect after + /// the initial load — without coupling the two providers by type. Wired once + /// in `main.dart`. + void Function(Set onlineServerIds)? onOnlineServersChanged; + /// Visibility filter applied by the active app profile. `null` means /// "all servers visible" (no profile restriction); otherwise only server /// ids in the set surface through [serverIds] / [onlineServerIds]. @@ -136,6 +144,12 @@ class MultiServerProvider extends ChangeNotifier with DisposableChangeNotifierMi safeNotifyListeners(); + // Reload libraries when the online set changes. LibrariesProvider owns + // the "is anything actually new to me?" decision (its loaded set can + // differ from _previousOnlineServerIds after a load error or a profile + // switch that cleared it), so notify unconditionally and let it decide. + onOnlineServersChanged?.call(currentOnline); + // Only re-check live TV when a new server came online if (hasNewServer) { checkLiveTvAvailability(); diff --git a/lib/services/data_aggregation_service.dart b/lib/services/data_aggregation_service.dart index 659e8ee7..bc71c821 100644 --- a/lib/services/data_aggregation_service.dart +++ b/lib/services/data_aggregation_service.dart @@ -23,23 +23,34 @@ class DataAggregationService { DataAggregationService(this._serverManager); /// Fetch libraries from all online clients regardless of backend, returning - /// neutral [MediaLibrary]s. - Future> getMediaLibrariesFromAllServers() async { + /// the merged neutral [MediaLibrary]s alongside the ids of the servers whose + /// fetch actually succeeded. + /// + /// A per-server `fetchLibraries()` failure is swallowed (that server simply + /// contributes no libraries) so one unreachable server doesn't sink the whole + /// list. [succeededServerIds] lets callers tell a *failed* fetch apart from a + /// server that genuinely has no libraries — both contribute nothing, so + /// conflating them would let a transient failure be cached as "loaded" and + /// never retried. + Future<({List libraries, Set succeededServerIds})> getMediaLibrariesFromAllServers() async { final clients = _serverManager.onlineClients; if (clients.isEmpty) { appLogger.w('No online servers available for fetching libraries (neutral)'); - return []; + return (libraries: const [], succeededServerIds: const {}); } + final succeededServerIds = {}; final futures = clients.entries.map((entry) async { try { - return await entry.value.fetchLibraries(); + final libraries = await entry.value.fetchLibraries(); + succeededServerIds.add(entry.key); + return libraries; } catch (e, stackTrace) { appLogger.e('Failed neutral library fetch from ${entry.key}', error: e, stackTrace: stackTrace); return []; } }); final results = await Future.wait(futures); - return [for (final list in results) ...list]; + return (libraries: [for (final list in results) ...list], succeededServerIds: succeededServerIds); } /// Fetch "On Deck" (Continue Watching) from all servers and merge by recency. @@ -243,7 +254,9 @@ class DataAggregationService { // Only fallback clients need a library prefetch when home layout is on; // rich-hub backends return the intended home rows directly. final needsLibraryPrefetch = useGlobalHubs && clients.values.any((client) => !client.capabilities.richHubs); - final libraries = needsLibraryPrefetch ? _groupLibrariesByServer(await getMediaLibrariesFromAllServers()) : null; + final libraries = needsLibraryPrefetch + ? _groupLibrariesByServer((await getMediaLibrariesFromAllServers()).libraries) + : null; final futures = clients.entries.map((entry) async { final serverId = entry.key; diff --git a/test/providers/libraries_provider_test.dart b/test/providers/libraries_provider_test.dart index 0d42b3c5..354c3b38 100644 --- a/test/providers/libraries_provider_test.dart +++ b/test/providers/libraries_provider_test.dart @@ -1,8 +1,13 @@ +import 'dart:async'; + import 'package:flutter_test/flutter_test.dart'; import 'package:plezy/media/media_backend.dart'; import 'package:plezy/media/media_kind.dart'; import 'package:plezy/media/media_library.dart'; +import 'package:plezy/media/media_server_client.dart'; import 'package:plezy/providers/libraries_provider.dart'; +import 'package:plezy/services/data_aggregation_service.dart'; +import 'package:plezy/services/multi_server_manager.dart'; import 'package:plezy/services/storage_service.dart'; import '../test_helpers/prefs.dart'; @@ -15,6 +20,46 @@ MediaLibrary _lib(String key, {String type = 'movie', String? serverId, String t serverId: serverId, ); +MediaLibrary _serverLib(String serverId, String id, String title) => + MediaLibrary(id: id, backend: MediaBackend.plex, title: title, kind: MediaKind.movie, serverId: serverId); + +/// Minimal [MediaServerClient] returning canned libraries; only the surface the +/// aggregation service touches is implemented. An optional [gate] lets a test +/// hold `fetchLibraries` open to exercise the mid-load race; setting [error] +/// makes `fetchLibraries` throw, simulating a (possibly transient) failure. +class _FakeClient implements MediaServerClient { + _FakeClient({required this.serverId, this.libraries = const [], this.gate}); + + @override + final String serverId; + @override + final String serverName = 'Server'; + + final List libraries; + final Future? gate; + + /// When non-null, [fetchLibraries] throws this instead of returning. Mutable + /// so a test can fail a fetch once and then let it recover. + Object? error; + + int fetchLibrariesCalls = 0; + + @override + Future> fetchLibraries() async { + fetchLibrariesCalls++; + final pending = gate; + if (pending != null) await pending; + if (error != null) throw error!; + return libraries; + } + + @override + void close() {} + + @override + dynamic noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation); +} + void main() { setUp(resetSharedPreferencesForTest); @@ -115,4 +160,226 @@ void main() { await p.updateLibraryOrder([_lib('1', serverId: 'srv')]); }); }); + + group('LibrariesProvider.syncToOnlineServers', () { + test('loads when a server first comes online', () async { + final manager = MultiServerManager(); + final clientA = _FakeClient(serverId: 'A', libraries: [_serverLib('A', '1', 'Movies A')]); + manager.debugRegisterClientForTesting(clientA); + final p = LibrariesProvider()..initialize(DataAggregationService(manager)); + + await p.syncToOnlineServers({'A'}); + + expect(p.hasLoaded, isTrue); + expect(p.libraries.map((l) => l.title), ['Movies A']); + expect(clientA.fetchLibrariesCalls, 1); + + p.dispose(); + manager.dispose(); + }); + + test('does not reload when the online set is unchanged', () async { + final manager = MultiServerManager(); + final clientA = _FakeClient(serverId: 'A', libraries: [_serverLib('A', '1', 'Movies A')]); + manager.debugRegisterClientForTesting(clientA); + final p = LibrariesProvider()..initialize(DataAggregationService(manager)); + + await p.syncToOnlineServers({'A'}); + await p.syncToOnlineServers({'A'}); // already covered → no-op + + expect(clientA.fetchLibrariesCalls, 1); + + p.dispose(); + manager.dispose(); + }); + + test('reloads and surfaces a server that connects after the first load', () async { + // The bug: a server binding in a later wave (borrowed connection, or a + // slow server reconnecting after timing out) was never picked up because + // the load was one-shot. + final manager = MultiServerManager(); + final clientA = _FakeClient(serverId: 'A', libraries: [_serverLib('A', '1', 'Movies A')]); + manager.debugRegisterClientForTesting(clientA); + final p = LibrariesProvider()..initialize(DataAggregationService(manager)); + + await p.syncToOnlineServers({'A'}); + expect(p.libraries.map((l) => l.title), ['Movies A']); + + final clientB = _FakeClient(serverId: 'B', libraries: [_serverLib('B', '1', 'Shows B')]); + manager.debugRegisterClientForTesting(clientB); + await p.syncToOnlineServers({'A', 'B'}); + + expect(p.libraries.map((l) => l.title), containsAll(['Movies A', 'Shows B'])); + expect(clientA.fetchLibrariesCalls, 2); + expect(clientB.fetchLibrariesCalls, 1); + + p.dispose(); + manager.dispose(); + }); + + test('a background reload over existing data never surfaces a loading state', () async { + final manager = MultiServerManager(); + final clientA = _FakeClient(serverId: 'A', libraries: [_serverLib('A', '1', 'Movies A')]); + manager.debugRegisterClientForTesting(clientA); + final p = LibrariesProvider()..initialize(DataAggregationService(manager)); + + await p.syncToOnlineServers({'A'}); + expect(p.hasLoaded, isTrue); + + // A server connecting later must not flip the provider back to a loading + // state — screens render `isLoading` as a full-screen spinner, so a + // background reload would blank content the user is already viewing. + final sawLoading = []; + p.addListener(() => sawLoading.add(p.isLoading)); + + final clientB = _FakeClient(serverId: 'B', libraries: [_serverLib('B', '1', 'Shows B')]); + manager.debugRegisterClientForTesting(clientB); + await p.syncToOnlineServers({'A', 'B'}); + + expect(sawLoading, isNot(contains(true))); + expect(p.libraries.map((l) => l.title), containsAll(['Movies A', 'Shows B'])); + + p.dispose(); + manager.dispose(); + }); + + test('a server whose fetch fails is retried on the next sync, not cached as loaded', () async { + // Regression: getMediaLibrariesFromAllServers swallows a per-server fetch + // failure and returns no libraries for it — identical to a genuinely empty + // server. Keying loaded-state on fetch *success* keeps a transiently + // failed server out of _loadedServerIds so it reloads instead of staying + // missing until a profile re-switch/restart. + final manager = MultiServerManager(); + final clientA = _FakeClient(serverId: 'A', libraries: [_serverLib('A', '1', 'Movies A')]); + final clientB = _FakeClient(serverId: 'B', libraries: [_serverLib('B', '1', 'Shows B')]) + ..error = Exception('transient'); + manager.debugRegisterClientForTesting(clientA); + manager.debugRegisterClientForTesting(clientB); + final p = LibrariesProvider()..initialize(DataAggregationService(manager)); + + await p.syncToOnlineServers({'A', 'B'}); + // A loaded; B's fetch failed, so it is absent and must not be recorded. + expect(p.libraries.map((l) => l.title), ['Movies A']); + + // B recovers. The same online set must now reload it rather than treating + // B as already covered. + clientB.error = null; + await p.syncToOnlineServers({'A', 'B'}); + + expect(p.libraries.map((l) => l.title), containsAll(['Movies A', 'Shows B'])); + + p.dispose(); + manager.dispose(); + }); + + test('does not reload when the online set shrinks', () async { + final manager = MultiServerManager(); + final clientA = _FakeClient(serverId: 'A', libraries: [_serverLib('A', '1', 'Movies A')]); + final clientB = _FakeClient(serverId: 'B', libraries: [_serverLib('B', '1', 'Shows B')]); + manager.debugRegisterClientForTesting(clientA); + manager.debugRegisterClientForTesting(clientB); + final p = LibrariesProvider()..initialize(DataAggregationService(manager)); + + await p.syncToOnlineServers({'A', 'B'}); + expect(clientA.fetchLibrariesCalls, 1); + + // A drops; the visible online set is now a subset of what we loaded. + await p.syncToOnlineServers({'B'}); + + expect(clientA.fetchLibrariesCalls, 1); + expect(clientB.fetchLibrariesCalls, 1); + + p.dispose(); + manager.dispose(); + }); + + test('a zero-library server is marked loaded and does not retrigger', () async { + final manager = MultiServerManager(); + final clientC = _FakeClient(serverId: 'C', libraries: const []); + manager.debugRegisterClientForTesting(clientC); + final p = LibrariesProvider()..initialize(DataAggregationService(manager)); + + await p.syncToOnlineServers({'C'}); + expect(p.hasLoaded, isTrue); + expect(p.libraries, isEmpty); + expect(clientC.fetchLibrariesCalls, 1); + + // Tracking the requested set (not deriving from loaded libraries) is what + // stops a zero-library server from looking "never loaded" and reloading + // on every status emission. + await p.syncToOnlineServers({'C'}); + expect(clientC.fetchLibrariesCalls, 1); + + p.dispose(); + manager.dispose(); + }); + + test('a server appearing mid-load is still picked up', () async { + final manager = MultiServerManager(); + final gate = Completer(); + final clientA = _FakeClient(serverId: 'A', libraries: [_serverLib('A', '1', 'Movies A')], gate: gate.future); + manager.debugRegisterClientForTesting(clientA); + final p = LibrariesProvider()..initialize(DataAggregationService(manager)); + + // First load starts and suspends on A's gated fetch. + final inFlight = p.syncToOnlineServers({'A'}); + + // B comes online before the first load completes. + final clientB = _FakeClient(serverId: 'B', libraries: [_serverLib('B', '1', 'Shows B')]); + manager.debugRegisterClientForTesting(clientB); + unawaited(p.syncToOnlineServers({'A', 'B'})); // queued behind the in-flight pass + + gate.complete(); + await inFlight; // resolves after the replayed pass covering {A, B} + + expect(p.libraries.map((l) => l.title), containsAll(['Movies A', 'Shows B'])); + expect(clientA.fetchLibrariesCalls, 2, reason: 'a second pass runs for the larger set'); + + p.dispose(); + manager.dispose(); + }); + + test('clear() resets tracking so the next sync reloads', () async { + final manager = MultiServerManager(); + final clientA = _FakeClient(serverId: 'A', libraries: [_serverLib('A', '1', 'Movies A')]); + manager.debugRegisterClientForTesting(clientA); + final p = LibrariesProvider()..initialize(DataAggregationService(manager)); + + await p.syncToOnlineServers({'A'}); + expect(clientA.fetchLibrariesCalls, 1); + + p.clear(); + expect(p.hasLoaded, isFalse); + + await p.syncToOnlineServers({'A'}); + expect(clientA.fetchLibrariesCalls, 2); + + p.dispose(); + manager.dispose(); + }); + + test('is a no-op for an empty set or before initialize', () async { + final manager = MultiServerManager(); + final clientA = _FakeClient(serverId: 'A', libraries: [_serverLib('A', '1', 'Movies A')]); + manager.debugRegisterClientForTesting(clientA); + + // Empty set on an initialized provider. + final p = LibrariesProvider()..initialize(DataAggregationService(manager)); + await p.syncToOnlineServers({}); + expect(p.loadState, LibrariesLoadState.initial); + expect(clientA.fetchLibrariesCalls, 0); + p.dispose(); + + // Non-empty set on an uninitialized provider. + final p2 = LibrariesProvider(); + var notified = 0; + p2.addListener(() => notified++); + await p2.syncToOnlineServers({'A'}); + expect(p2.loadState, LibrariesLoadState.initial); + expect(notified, 0); + p2.dispose(); + + manager.dispose(); + }); + }); } diff --git a/test/providers/multi_server_provider_test.dart b/test/providers/multi_server_provider_test.dart index fb450f4f..7c0fe47b 100644 --- a/test/providers/multi_server_provider_test.dart +++ b/test/providers/multi_server_provider_test.dart @@ -82,6 +82,26 @@ void main() { p.dispose(); }); + test('invokes onOnlineServersChanged with the visibility-filtered online set', () async { + final p = MultiServerProvider(manager, aggregation); + final calls = >[]; + p.onOnlineServersChanged = calls.add; + + manager.updateServerStatus('srv-1', true); + await Future.delayed(Duration.zero); + expect(calls, isNotEmpty); + expect(calls.last, {'srv-1'}); + + // A server that is online in the manager but outside the active profile's + // visibility filter must not appear in the payload. + p.setVisibleServerIds({'srv-1'}); + manager.updateServerStatus('srv-2', true); + await Future.delayed(Duration.zero); + expect(calls.last, {'srv-1'}, reason: 'srv-2 is online but filtered out'); + + p.dispose(); + }); + test('checkServerHealth with no clients completes without error', () async { final p = MultiServerProvider(manager, aggregation); // Empty clients map → no work, but the call must complete. diff --git a/test/services/data_aggregation_bridge_test.dart b/test/services/data_aggregation_bridge_test.dart index fdfb947c..b9925b33 100644 --- a/test/services/data_aggregation_bridge_test.dart +++ b/test/services/data_aggregation_bridge_test.dart @@ -51,7 +51,9 @@ void main() { group('DataAggregationService cross-server aggregation', () { test('getMediaLibrariesFromAllServers returns empty when no clients connected', () async { - expect(await service.getMediaLibrariesFromAllServers(), isEmpty); + final result = await service.getMediaLibrariesFromAllServers(); + expect(result.libraries, isEmpty); + expect(result.succeededServerIds, isEmpty); }); test('searchAcrossServers and getOnDeckFromAllServers return empty when no clients', () async {