From 439ae1d73386696b57ac26f3d29b928b459537b5 Mon Sep 17 00:00:00 2001 From: edde746 <86283021+edde746@users.noreply.github.com> Date: Tue, 4 Aug 2026 06:58:05 +0200 Subject: [PATCH] perf(detail): paint a show before its on-deck episode is looked up MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Jellyfin has no equivalent of Plex's bundled `?includeOnDeck=1`, so a show detail open chained `/Shows/NextUp` behind the item fetch and the screen sat on a spinner for both round trips. The second one is not needed to paint: everything except the play button's episode label comes from the item. `fetchItemWithOnDeck` now takes an `onItemReady` callback and invokes it as soon as the item is known, when that is strictly before on-deck settles. Plex returns both together and never invokes it. Phone and desktop only. TV keeps its own reveal gate — `_isTvDetailReadyToReveal` holds the foreground at opacity 0 until extras, related hubs, seasons and the first episode page have all loaded, and those still run after the on-deck lookup settles, so TV sees no change. Both halves are pinned by tests. Measured on a remote Jellyfin server, 15 interleaved show-detail opens per version: time to content 1264ms -> 1042ms (-18%), with the rest of the load unchanged. Seasons and extras deliberately still start after the whole lookup settles. Starting them at the early paint measured worse (time to settled +21%) because they contend with the on-deck request instead of overlapping it — the same reason `/Shows/NextUp` is not fired in parallel with the item fetch. That trade-off is also why TV was left alone rather than being unblocked by moving those loads earlier. Two ordering hazards the early paint introduces, both covered by `media_detail_screen_test.dart`: - The early call must not write on-deck. `_loadFullMetadata` runs again after playback, and clearing there would blank the play button for the length of the round trip. `onDeckSettled` marks the authoritative write, so a reload that finds the series finished still clears it. - A settled empty on-deck must not drop the episode-derived fallback that `_ensureFallbackOnDeckEpisode` supplies. close #1784 --- lib/media/media_server_client.dart | 25 ++- lib/screens/media_detail_screen.dart | 85 +++++--- .../jellyfin_client/parts/browse.dart | 17 +- lib/services/plex_client.dart | 10 +- test/screens/media_detail_screen_test.dart | 187 ++++++++++++++++-- 5 files changed, 265 insertions(+), 59 deletions(-) diff --git a/lib/media/media_server_client.dart b/lib/media/media_server_client.dart index 59804107..6cf641d0 100644 --- a/lib/media/media_server_client.dart +++ b/lib/media/media_server_client.dart @@ -167,13 +167,24 @@ abstract class MediaServerClient { Future fetchItem(String id); /// Fetch a single item *and* its on-deck episode (the next unwatched / - /// in-progress episode) in one round-trip when the backend supports it. - /// The item follows [fetchItem]'s error contract: an online HTTP 404 returns - /// both nullable fields as `null`, while every other HTTP status throws. - /// Plex bundles both via `/library/metadata/{id}?includeOnDeck=1`; - /// Jellyfin has no equivalent endpoint and returns `onDeckEpisode: null`, - /// leaving callers to fetch on-deck separately if they need it. - Future<({MediaItem? item, MediaItem? onDeckEpisode})> fetchItemWithOnDeck(String id); + /// in-progress episode). The item follows [fetchItem]'s error contract: an + /// online HTTP 404 returns both nullable fields as `null`, while every other + /// HTTP status throws. + /// + /// Plex bundles both via `/library/metadata/{id}?includeOnDeck=1`. Jellyfin + /// has no equivalent endpoint and needs a second request for on-deck, so it + /// would otherwise hold the item behind a round trip the detail screen does + /// not need in order to paint. + /// + /// [onItemReady] exists for exactly that case: implementations invoke it as + /// soon as the item is known, *if* that is strictly before the on-deck + /// lookup finishes. Backends that return both together never invoke it, and + /// neither does a null item. Callers must therefore treat it as an optional + /// early paint and still handle the returned record. + Future<({MediaItem? item, MediaItem? onDeckEpisode})> fetchItemWithOnDeck( + String id, { + void Function(MediaItem item)? onItemReady, + }); /// Direct children of [parentId] — episodes of a season, seasons of a /// show, tracks of an album, items of a collection. diff --git a/lib/screens/media_detail_screen.dart b/lib/screens/media_detail_screen.dart index aee6041c..5eb69322 100644 --- a/lib/screens/media_detail_screen.dart +++ b/lib/screens/media_detail_screen.dart @@ -207,6 +207,12 @@ class _SeasonEpisodePager { } } +/// Identifies the TV reveal gate's opacity wrapper. The detail tree builds +/// other [AnimatedOpacity] widgets (the scroll-linked app-bar scrim is 0 at +/// rest), so tests must target this one specifically. +@visibleForTesting +const tvDetailRevealGateKey = ValueKey('tvDetailRevealGate'); + class MediaDetailScreen extends StatefulWidget { final MediaItem metadata; final bool isOffline; @@ -837,6 +843,9 @@ class _MediaDetailScreenState extends State child: IgnorePointer( ignoring: !revealed, child: AnimatedOpacity( + // Keyed so tests can assert this specific gate rather than + // whichever AnimatedOpacity happens to be lowest on screen. + key: tvDetailRevealGateKey, opacity: revealed ? 1 : 0, duration: const Duration(milliseconds: 160), curve: Curves.easeOutCubic, @@ -1280,35 +1289,59 @@ class _MediaDetailScreenState extends State return; } - final result = await client.fetchItemWithOnDeck(_metadata.id); + // Normalises a freshly fetched item against the row we navigated from + // (which owns serverId/library) and paints it. Called once from + // [onItemReady] on backends that learn the item before on-deck, and once + // from the settled result. + // + // [onDeckSettled] separates "on-deck not looked up yet" from "on-deck + // looked up and there is none". Only the settled call may write it, so + // the early paint leaves whatever is on screen alone — this method runs + // again after playback, and clearing there would blank the play button + // for the length of the on-deck round trip — while a reload that finds + // the series finished still clears it. + MediaItem publish(MediaItem source, {MediaItem? onDeckEpisode, bool onDeckSettled = false}) { + final serverId = _metadata.serverId; + final serverName = _metadata.serverName; + final base = _withFallbackLibrary( + source.copyWith(serverId: serverId ?? source.serverId, serverName: serverName ?? source.serverName), + _metadata, + ); + final onDeckWithServerId = onDeckEpisode == null + ? null + : _withFallbackLibrary( + onDeckEpisode.copyWith( + serverId: serverId ?? onDeckEpisode.serverId, + serverName: serverName ?? onDeckEpisode.serverName, + ), + base, + ); + + setState(() { + _fullMetadata = base; + if (onDeckSettled) _onDeckEpisode = onDeckWithServerId; + _isLoadingMetadata = false; + }); + return base; + } + + // Jellyfin needs a second round trip for on-deck, which the screen does + // not need in order to paint. Publishing the item as soon as it lands + // takes that round trip off the critical path (#1784). + // + // Seasons/extras deliberately still start after the whole lookup + // settles: starting them at the early paint measured *worse*, because + // they contend with the on-deck request rather than overlapping it. + final result = await client.fetchItemWithOnDeck( + _metadata.id, + onItemReady: (item) { + if (mounted) publish(item); + }, + ); final metadata = result.item; - final onDeckEpisode = result.onDeckEpisode; if (!mounted) return; - - // Preserve serverId from original metadata - final serverId = _metadata.serverId; - final serverName = _metadata.serverName; - final source = metadata ?? _metadata; - final base = _withFallbackLibrary( - source.copyWith(serverId: serverId ?? source.serverId, serverName: serverName ?? source.serverName), - _metadata, - ); - final onDeckWithServerId = onDeckEpisode == null - ? null - : _withFallbackLibrary( - onDeckEpisode.copyWith( - serverId: serverId ?? onDeckEpisode.serverId, - serverName: serverName ?? onDeckEpisode.serverName, - ), - base, - ); - - setState(() { - _fullMetadata = base; - _onDeckEpisode = onDeckWithServerId; - _isLoadingMetadata = false; - }); + final base = publish(metadata ?? _metadata, onDeckEpisode: result.onDeckEpisode, onDeckSettled: true); if (base.isShow) { unawaited(_loadSeasons()); diff --git a/lib/services/jellyfin_client/parts/browse.dart b/lib/services/jellyfin_client/parts/browse.dart index 2cff6ccf..2c1b565f 100644 --- a/lib/services/jellyfin_client/parts/browse.dart +++ b/lib/services/jellyfin_client/parts/browse.dart @@ -613,17 +613,22 @@ mixin _JellyfinBrowseMethods on _JellyfinClientInternals { /// OnDeck semantics: returns the resume episode when one exists, or S1E1 /// when the user hasn't started. Movies and other kinds short-circuit. /// - /// Deliberately still chained rather than fired in parallel off a caller - /// kind hint: measured against a remote server that saved nothing, because - /// the two requests contend rather than overlap (`/Shows/NextUp` went from - /// 380ms alone to 1395ms beside the detail fetch), and it would cost a - /// wasted request on every movie whose hint was absent or wrong. + /// The chain stays sequential — firing both together measured no better, + /// because the requests contend rather than overlap (`/Shows/NextUp` went + /// from 380ms alone to 1395ms beside the detail fetch) and a movie would pay + /// for a request it can never use. Instead the item is handed to + /// [onItemReady] the moment it lands, so the caller can paint without + /// waiting for the on-deck round trip (#1784). @override - Future<({MediaItem? item, MediaItem? onDeckEpisode})> fetchItemWithOnDeck(String id) async { + Future<({MediaItem? item, MediaItem? onDeckEpisode})> fetchItemWithOnDeck( + String id, { + void Function(MediaItem item)? onItemReady, + }) async { final item = await fetchItem(id); if (item == null || item.kind != MediaKind.show) { return (item: item, onDeckEpisode: null); } + onItemReady?.call(item); final nextUp = await _safeFetchItemsArray('/Shows/NextUp', { 'seriesId': id, 'userId': connection.userId, diff --git a/lib/services/plex_client.dart b/lib/services/plex_client.dart index c08cfdc9..ec92176b 100644 --- a/lib/services/plex_client.dart +++ b/lib/services/plex_client.dart @@ -3769,10 +3769,14 @@ class PlexClient } /// Full item with on-deck episode from a single `/library/metadata/{id}` - /// round-trip. Implements [MediaServerClient.fetchItemWithOnDeck]; - /// Jellyfin has no analogous endpoint and returns onDeck=null there. + /// round-trip. Implements [MediaServerClient.fetchItemWithOnDeck]. Both + /// halves arrive together, so there is no window in which the item is known + /// and on-deck is not — `onItemReady` is intentionally never invoked. @override - Future<({MediaItem? item, MediaItem? onDeckEpisode})> fetchItemWithOnDeck(String id) async { + Future<({MediaItem? item, MediaItem? onDeckEpisode})> fetchItemWithOnDeck( + String id, { + void Function(MediaItem item)? onItemReady, + }) async { try { final result = await getMetadataWithImagesAndOnDeck(id, shouldFallback: _shouldFallbackPlexItemLookup); final itemDto = result['metadata'] as PlexMetadataDto?; diff --git a/test/screens/media_detail_screen_test.dart b/test/screens/media_detail_screen_test.dart index dca1f983..8c86383e 100644 --- a/test/screens/media_detail_screen_test.dart +++ b/test/screens/media_detail_screen_test.dart @@ -315,9 +315,7 @@ void main() { }, pendingPlayableDescendants: descendantsCompleter.future, ); - final manager = MultiServerManager()..debugRegisterClientForTesting(client); - final provider = testMultiServerProvider(manager); - addTearDown(provider.dispose); + final provider = testMultiServer(clients: [client]).provider; await tester.pumpWidget( TranslationProvider( @@ -342,6 +340,97 @@ void main() { expect(find.text('Specials'), findsNothing); expect(find.text('S1E1'), findsOneWidget); }); + testWidgets('TV detail reveal still waits for the supplemental sections', (tester) async { + // Counterpart to the test above: the early paint does NOT move the TV + // reveal. `_isTvDetailReadyToReveal` additionally requires extras, related + // hubs, seasons and the first episode page, and those deliberately start + // only once the on-deck lookup settles — starting them at the early paint + // measured worse, because they contend with it rather than overlap. + // Pinned so the phone/desktop win is never restated as an all-platform one. + await SettingsService.getInstance(); + tester.view.physicalSize = const Size(1280, 720); + tester.view.devicePixelRatio = 1; + addTearDown(tester.view.resetPhysicalSize); + addTearDown(tester.view.resetDevicePixelRatio); + + final show = testMediaItem( + id: 'show_1', + backend: MediaBackend.jellyfin, + kind: MediaKind.show, + title: 'The Show', + serverId: 'server_1', + serverName: 'Server', + ); + final season1 = testMediaItem( + id: 'season_1', + backend: MediaBackend.jellyfin, + kind: MediaKind.season, + title: 'Season 1', + index: 1, + parentId: show.id, + serverId: show.serverId, + serverName: show.serverName, + ); + final episode1 = testMediaItem( + id: 'episode_1', + backend: MediaBackend.jellyfin, + kind: MediaKind.episode, + title: 'Episode 1', + index: 1, + parentIndex: season1.index, + parentId: season1.id, + grandparentId: show.id, + serverId: show.serverId, + serverName: show.serverName, + ); + + final client = _FakeMediaServerClient( + show: show, + childrenByParent: { + show.id: [season1], + season1.id: [episode1], + }, + )..onDeckGate = Completer(); + final provider = testMultiServer(clients: [client]).provider; + + await tester.pumpWidget( + TranslationProvider( + child: ChangeNotifierProvider.value( + value: provider, + child: MaterialApp( + theme: monoTheme(dark: true), + home: withProfileNavigationScope( + child: SizedBox(width: 1280, height: 720, child: MediaDetailScreen(metadata: show)), + ), + ), + ), + ), + ); + + for (var i = 0; i < 4; i++) { + await tester.pump(); + } + await tester.pump(const Duration(milliseconds: 200)); + + // Target the reveal gate specifically: the detail tree also builds a + // scroll-linked app-bar scrim whose opacity is 0 at rest, so matching on + // AnimatedOpacity by type would pass no matter what the gate does. + double revealOpacity() => tester.widget(find.byKey(tvDetailRevealGateKey)).opacity; + + // The item was published early and the metadata phase is over... + expect(client.earlyPaints, hasLength(1)); + expect(find.byType(CircularProgressIndicator), findsNothing); + // ...yet TV shows nothing but the backdrop, because the reveal gate also + // waits on extras, related hubs, seasons and the first episode page — none + // of which have started, since they run after the on-deck lookup settles. + expect(revealOpacity(), 0, reason: 'the early paint must not be claimed as a TV win'); + + // Let the held lookup finish so teardown is not left holding a suspended + // future and a client mid-request. + client.onDeckGate!.complete(); + await tester.pump(); + await tester.pump(const Duration(milliseconds: 100)); + }); testWidgets('TV detail summary uses light theme foreground color', (tester) async { await SettingsService.getInstance(); @@ -441,9 +530,7 @@ void main() { season2.id: [episode2], }, ); - final manager = MultiServerManager()..debugRegisterClientForTesting(client); - final provider = testMultiServerProvider(manager); - addTearDown(provider.dispose); + final provider = testMultiServer(clients: [client]).provider; await tester.pumpWidget( TranslationProvider( @@ -538,9 +625,7 @@ void main() { }, childrenPageErrors: {season1.id: Exception('season cache failed')}, ); - final manager = MultiServerManager()..debugRegisterClientForTesting(client); - final provider = testMultiServerProvider(manager); - addTearDown(provider.dispose); + final provider = testMultiServer(clients: [client]).provider; await tester.pumpWidget( TranslationProvider( @@ -629,9 +714,7 @@ void main() { }, childrenPageFutures: {season2.id: season2Completer.future}, ); - final manager = MultiServerManager()..debugRegisterClientForTesting(client); - final provider = testMultiServerProvider(manager); - addTearDown(provider.dispose); + final provider = testMultiServer(clients: [client]).provider; await tester.pumpWidget( TranslationProvider( @@ -839,15 +922,16 @@ void main() { final downloadProvider = DownloadProvider.forTesting(downloadManager: downloadManager, database: db); await downloadProvider.ensureInitialized(); - final manager = MultiServerManager()..debugRegisterClientForTesting(client); - final multiServerProvider = testMultiServerProvider(manager); + // testMultiServer disposes the manager as well as its provider; + // MultiServerProvider does not own the manager, and manager.dispose() is + // what closes its status/progress controllers and the registered client. + final multiServerProvider = testMultiServer(clients: [client]).provider; final watchStateOverlay = WatchStateStore(); addTearDown(() async { watchStateOverlay.dispose(); downloadProvider.dispose(); downloadManager.dispose(); - multiServerProvider.dispose(); await db.close(); }); @@ -912,6 +996,53 @@ void main() { ); } + testWidgets('paints the item before the on-deck lookup settles', (tester) async { + // Jellyfin needs a second round trip for on-deck; the phone/desktop + // layout must not wait for it. Scoped to non-TV deliberately: on TV the + // foreground stays at opacity 0 until `_isTvDetailReadyToReveal` is + // satisfied, which this change does not move (see the TV counterpart). + final show = buildShow(); + final season1 = buildSeason(show, 1); + MediaItem episode(int number, {required int viewCount}) => testMediaItem( + id: 'episode_$number', + backend: MediaBackend.jellyfin, + kind: MediaKind.episode, + title: 'Episode $number', + index: number, + parentIndex: season1.index, + parentId: season1.id, + grandparentId: show.id, + serverId: show.serverId, + serverName: show.serverName, + viewCount: viewCount, + ); + + final client = _FakeMediaServerClient( + show: show, + childrenByParent: { + show.id: [season1], + season1.id: [episode(1, viewCount: 1), episode(2, viewCount: 0)], + }, + )..onDeckGate = Completer(); + + await pumpPhoneDetail(tester, client, show); + + // On-deck is still in flight, but the item has landed. + expect(client.earlyPaints, hasLength(1)); + expect(find.byType(CircularProgressIndicator), findsNothing, reason: 'painted without waiting for on-deck'); + + // Settling with no on-deck must not drop the episode-derived fallback + // that `_ensureFallbackOnDeckEpisode` supplies. + client.onDeckGate!.complete(); + for (var i = 0; i < 6; i++) { + await tester.pump(); + } + await tester.pump(const Duration(milliseconds: 300)); + + expect(find.text('S1E2'), findsOneWidget, reason: 'fallback survives a settled empty on-deck'); + expect(find.text('S1E1'), findsNothing); + }); + testWidgets('shows directors when they are the only additional info', (tester) async { final movie = testMediaItem( id: 'director_only', @@ -1212,6 +1343,17 @@ class _FakeMediaServerClient implements MediaServerClient { final childrenPageCalls = <({String parentId, int? start, int? size})>[]; final thumbnailPaths = []; + /// On-deck episode returned by the next [fetchItemWithOnDeck]; mutate between + /// loads to model the series being finished. + MediaItem? onDeckEpisode; + + /// Held open to keep the on-deck half of a load in flight while the item half + /// has already been published. + Completer? onDeckGate; + + /// Items handed to `onItemReady` — i.e. painted before on-deck settled. + final earlyPaints = []; + _FakeMediaServerClient({ required this.show, required this.childrenByParent, @@ -1233,8 +1375,19 @@ class _FakeMediaServerClient implements MediaServerClient { ServerCapabilities get capabilities => ServerCapabilities.jellyfin; @override - Future<({MediaItem? item, MediaItem? onDeckEpisode})> fetchItemWithOnDeck(String id) async { - return (item: show, onDeckEpisode: null); + Future<({MediaItem? item, MediaItem? onDeckEpisode})> fetchItemWithOnDeck( + String id, { + void Function(MediaItem item)? onItemReady, + }) async { + // Mirrors the Jellyfin shape: the item is known first, on-deck needs a + // second round trip. + if (onItemReady != null) { + earlyPaints.add(show); + onItemReady(show); + } + final gate = onDeckGate; + if (gate != null) await gate.future; + return (item: show, onDeckEpisode: onDeckEpisode); } @override