From fa8da33ec84cedbe604262b3d17ee7bf4d8b3abf Mon Sep 17 00:00:00 2001 From: edde746 <86283021+edde746@users.noreply.github.com> Date: Thu, 3 Sep 2026 23:54:09 +0200 Subject: [PATCH] fix(tv): drop a stale Discover rail-focus claim, log swallowed tvOS Menu presses, and unblock shelf test resets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings from the #2239 investigation. Discover armed "focus the browse rail when it has hubs" on its first load and kept it armed indefinitely while the rail had nothing to show. Hubs landing minutes later — reconnect, push refresh — then yanked the remote off a sidebar item the user had since moved to. The claim now lapses once focus sits on a control outside the screen; a bare content scope (the startup state) still lets the rail take its initial focus. On Apple TV a Menu press that reaches MainScreen at the home root is consumed silently by design (the engine normally hands root Menu to UIKit). That branch is what turned a focus slip into a "dead remote" report, so it now logs passthrough state, picker state, and the primary focus label. SystemShelfService.debugReset awaited the mutation tail queued by the previous widget test, whose FakeAsync zone ends without flushing the microtasks that settle the chain, so any second test in a file using it hung in setUp. It now drops the queue synchronously; the #2239 session regression test moves back beside the profile-switch test it belongs with. --- lib/screens/discover_screen.dart | 22 ++- lib/screens/main_screen.dart | 8 +- lib/services/system_shelf_service.dart | 9 +- .../profile_session_focus_boundary_test.dart | 157 ----------------- .../profile_session_screen_test.dart | 105 +++++++++++- test/screens/discover_screen_test.dart | 158 ++++++++++++++++++ 6 files changed, 296 insertions(+), 163 deletions(-) delete mode 100644 test/navigation/profile_session_focus_boundary_test.dart diff --git a/lib/screens/discover_screen.dart b/lib/screens/discover_screen.dart index c06da5804..0cec8e3fd 100644 --- a/lib/screens/discover_screen.dart +++ b/lib/screens/discover_screen.dart @@ -266,7 +266,7 @@ class _DiscoverScreenState extends State WidgetsBinding.instance.addPostFrameCallback((_) { if (!mounted) return; - if (!_isTabVisible || !(ModalRoute.of(context)?.isCurrent ?? false)) { + if (!_isTabVisible || !(ModalRoute.of(context)?.isCurrent ?? false) || _focusHasLeftScreen) { _pendingTvBrowseRailFocus = false; return; } @@ -278,8 +278,26 @@ class _DiscoverScreenState extends State }); } + /// A rail-focus request stays armed while the rail has no hubs to focus + /// (empty first load), so hubs landing later still receive it. It must not + /// outlive the user's own navigation: once focus sits on a control off this + /// screen (a sidebar item), hubs arriving minutes later would yank the + /// remote back. A bare scope — MainScreen's content scope before any child + /// has focus — is "nowhere yet", not a destination, and keeps the request. + bool get _focusHasLeftScreen { + final node = FocusManager.instance.primaryFocus; + final focusContext = node?.context; + if (node == null || node is FocusScopeNode || focusContext == null) return false; + return !identical(focusContext.findAncestorStateOfType<_DiscoverScreenState>(), this); + } + void _applyPendingTvBrowseRailFocus() { - if (_pendingTvBrowseRailFocus) _focusTvBrowseRailWhenReady(); + if (!_pendingTvBrowseRailFocus) return; + if (_focusHasLeftScreen) { + _pendingTvBrowseRailFocus = false; + return; + } + _focusTvBrowseRailWhenReady(); } /// Handle vertical navigation between hubs diff --git a/lib/screens/main_screen.dart b/lib/screens/main_screen.dart index bdd1b1fbb..0044e0ed9 100644 --- a/lib/screens/main_screen.dart +++ b/lib/screens/main_screen.dart @@ -1493,9 +1493,15 @@ class _MainScreenState extends State // The tvOS engine normally passes root Menu presses through to UIKit. If a // stale event still reaches Flutter, avoid showing an exit prompt that - // cannot be honored app-side. + // cannot be honored app-side. Log it: a Menu swallowed here with the + // picker up or focus off this route is how #2239 read as a dead remote. if (PlatformDetector.isAppleTV()) { _lastBackPressAt = null; + appLogger.d( + 'tvOS Menu reached MainScreen at the home root: ' + 'passthrough=$_shouldPassTvosMenuToSystem picker=$_isShowingProfileSelection ' + 'focus=${FocusManager.instance.primaryFocus?.debugLabel}', + ); return KeyEventResult.handled; } diff --git a/lib/services/system_shelf_service.dart b/lib/services/system_shelf_service.dart index 120f782b2..fff84bb84 100644 --- a/lib/services/system_shelf_service.dart +++ b/lib/services/system_shelf_service.dart @@ -62,9 +62,14 @@ class SystemShelfService { @visibleForTesting int get debugGeneration => _generation; + /// Forgets the owner and drops the mutation queue. Deliberately does not + /// await the old tail: a widget test's FakeAsync zone ends without + /// flushing the microtasks that settle a chained future, so the previous + /// test's final clear would leave a tail that never completes and hang the + /// next test's setUp. Tests own their channel fakes, so nothing native is + /// lost by abandoning the chain. @visibleForTesting - Future debugReset() async { - await _mutationTail; + void debugReset() { _activeOwner = null; _generation = 0; _mutationTail = Future.value(); diff --git a/test/navigation/profile_session_focus_boundary_test.dart b/test/navigation/profile_session_focus_boundary_test.dart deleted file mode 100644 index 15c88d1af..000000000 --- a/test/navigation/profile_session_focus_boundary_test.dart +++ /dev/null @@ -1,157 +0,0 @@ -import 'package:drift/native.dart'; -import 'package:flutter/material.dart'; -import 'package:flutter_test/flutter_test.dart'; -import 'package:plezy/connection/connection_registry.dart'; -import 'package:plezy/database/app_database.dart'; -import 'package:plezy/models/plex/plex_home_user.dart'; -import 'package:plezy/navigation/profile_session_screen.dart'; -import 'package:plezy/profiles/active_profile_provider.dart'; -import 'package:plezy/profiles/plex_home_service.dart'; -import 'package:plezy/profiles/profile.dart'; -import 'package:plezy/profiles/profile_connection_registry.dart'; -import 'package:plezy/profiles/profile_registry.dart'; -import 'package:plezy/providers/multi_server_provider.dart'; -import 'package:plezy/services/multi_server_manager.dart'; -import 'package:plezy/services/offline_watch_sync_service.dart'; -import 'package:plezy/services/storage_service.dart'; -import 'package:plezy/services/system_shelf_service.dart'; -import 'package:provider/provider.dart'; - -import '../test_helpers/io_fakes.dart'; -import '../test_helpers/multi_server_fixtures.dart'; -import '../test_helpers/prefs.dart'; - -// Own file: SystemShelfService.debugReset awaits the mutation tail queued by -// the previous test's profile switch, which never settles outside that test's -// zone, so a second testWidgets in profile_session_screen_test.dart hangs. -void main() { - TestWidgetsFlutterBinding.ensureInitialized(); - - setUp(() async { - resetSharedPreferencesForTest(); - await SystemShelfService().debugReset(); - }); - - testWidgets( - 'a root-navigator route over the session blocks focus steals below it and hands focus back on pop (#2239)', - (tester) async { - final db = AppDatabase.forTesting(NativeDatabase.memory()); - final profileRegistry = ProfileRegistry(db); - final connectionRegistry = ConnectionRegistry(db); - final profileConnectionRegistry = ProfileConnectionRegistry(db); - final storage = await StorageService.getInstance(); - final plexHome = _FakePlexHomeService( - connections: connectionRegistry, - profileConnections: profileConnectionRegistry, - storage: storage, - ); - final activeProfile = ActiveProfileProvider( - registry: profileRegistry, - plexHome: plexHome, - connections: connectionRegistry, - profileConnections: profileConnectionRegistry, - storage: storage, - ); - final serverManager = MultiServerManager(); - final multiServer = testMultiServerProvider(serverManager); - final offlineWatch = OfflineWatchSyncService(database: db, serverManager: serverManager); - final rootNavigator = GlobalKey(); - final content = FocusNode(debugLabel: 'SessionContent'); - final sidebar = FocusNode(debugLabel: 'SessionSidebar'); - final picker = FocusNode(debugLabel: 'RootPicker'); - - addTearDown(() async { - await tester.pumpWidget(const SizedBox.shrink()); - await tester.pumpAndSettle(); - content.dispose(); - sidebar.dispose(); - picker.dispose(); - await activeProfile.resetForTesting(); - activeProfile.dispose(); - multiServer.dispose(); - serverManager.dispose(); - await plexHome.dispose(); - offlineWatch.dispose(); - await db.close(); - }); - - final owner = Profile.local(id: 'local-owner', displayName: 'Owner', createdAt: DateTime(2026, 1, 1)); - await profileRegistry.upsert(owner); - await storage.setActiveProfileId(owner.id); - await activeProfile.initialize(); - - await tester.pumpWidget( - MultiProvider( - providers: [ - Provider.value(value: storage), - Provider.value(value: db), - Provider.value(value: connectionRegistry), - Provider.value(value: profileConnectionRegistry), - Provider.value(value: plexHome), - ChangeNotifierProvider.value(value: activeProfile), - ChangeNotifierProvider.value(value: multiServer), - ChangeNotifierProvider.value(value: offlineWatch), - ], - child: MaterialApp( - navigatorKey: rootNavigator, - home: ProfileSessionScreen.forTesting( - initialPromptHandled: true, - httpClientFactory: () => FakeHttpClient(200, const []), - profileShellBuilder: (context) => Column( - children: [ - Focus(focusNode: sidebar, child: const SizedBox(height: 10, width: 10)), - Focus(focusNode: content, autofocus: true, child: const SizedBox(height: 10, width: 10)), - ], - ), - ), - ), - ), - ); - await tester.pump(); - await tester.pump(const Duration(seconds: 1)); - expect(content.hasPrimaryFocus, isTrue); - - // The picker/PIN shape: pushed on the root navigator, above the whole - // nested profile-session navigator. - rootNavigator.currentState!.push( - MaterialPageRoute( - builder: (_) => Focus(focusNode: picker, autofocus: true, child: const SizedBox.expand()), - ), - ); - await tester.pump(); - await tester.pump(const Duration(seconds: 1)); - expect(picker.hasPrimaryFocus, isTrue); - - // A resume-time self-heal under MainScreen (sidebar reveal, grid load). - sidebar.requestFocus(); - await tester.pump(); - expect(sidebar.hasPrimaryFocus, isFalse, reason: 'covered session must not take focus'); - expect(picker.hasPrimaryFocus, isTrue); - - rootNavigator.currentState!.pop(); - await tester.pump(); - await tester.pump(const Duration(seconds: 1)); - expect(content.hasPrimaryFocus, isTrue, reason: 'focus returns to the session leaf that had it'); - }, - ); -} - -class _FakePlexHomeService extends PlexHomeService { - _FakePlexHomeService({required super.connections, required super.profileConnections, required StorageService storage}) - : super(storage: storage, plexHomeUserFetcher: (_) async => const []); - - @override - Map> get current => const {}; - - @override - Stream>> get stream => Stream.value(const {}); - - @override - Future start() async {} - - @override - Future reloadFromStorage() async {} - - @override - Future dispose() async {} -} diff --git a/test/navigation/profile_session_screen_test.dart b/test/navigation/profile_session_screen_test.dart index 673a17ff4..5a4fed979 100644 --- a/test/navigation/profile_session_screen_test.dart +++ b/test/navigation/profile_session_screen_test.dart @@ -31,7 +31,7 @@ void main() { setUp(() async { resetSharedPreferencesForTest(); - await SystemShelfService().debugReset(); + SystemShelfService().debugReset(); }); testWidgets('profile switch disposes the profile navigator, routes, and providers', (tester) async { @@ -186,6 +186,109 @@ void main() { expect(trackerHttpClients.toSet(), hasLength(trackerAuthClientsPerProfile * 3)); _expectCloseCount(trackerHttpClients, 1); }); + + testWidgets( + 'a root-navigator route over the session blocks focus steals below it and hands focus back on pop (#2239)', + (tester) async { + final db = AppDatabase.forTesting(NativeDatabase.memory()); + final profileRegistry = ProfileRegistry(db); + final connectionRegistry = ConnectionRegistry(db); + final profileConnectionRegistry = ProfileConnectionRegistry(db); + final storage = await StorageService.getInstance(); + final plexHome = _FakePlexHomeService( + connections: connectionRegistry, + profileConnections: profileConnectionRegistry, + storage: storage, + ); + final activeProfile = ActiveProfileProvider( + registry: profileRegistry, + plexHome: plexHome, + connections: connectionRegistry, + profileConnections: profileConnectionRegistry, + storage: storage, + ); + final serverManager = MultiServerManager(); + final multiServer = testMultiServerProvider(serverManager); + final offlineWatch = OfflineWatchSyncService(database: db, serverManager: serverManager); + final rootNavigator = GlobalKey(); + final content = FocusNode(debugLabel: 'SessionContent'); + final sidebar = FocusNode(debugLabel: 'SessionSidebar'); + final picker = FocusNode(debugLabel: 'RootPicker'); + + addTearDown(() async { + await tester.pumpWidget(const SizedBox.shrink()); + await tester.pumpAndSettle(); + content.dispose(); + sidebar.dispose(); + picker.dispose(); + await activeProfile.resetForTesting(); + activeProfile.dispose(); + multiServer.dispose(); + serverManager.dispose(); + await plexHome.dispose(); + offlineWatch.dispose(); + await db.close(); + }); + + final owner = Profile.local(id: 'local-owner', displayName: 'Owner', createdAt: DateTime(2026, 1, 1)); + await profileRegistry.upsert(owner); + await storage.setActiveProfileId(owner.id); + await activeProfile.initialize(); + + await tester.pumpWidget( + MultiProvider( + providers: [ + Provider.value(value: storage), + Provider.value(value: db), + Provider.value(value: connectionRegistry), + Provider.value(value: profileConnectionRegistry), + Provider.value(value: plexHome), + ChangeNotifierProvider.value(value: activeProfile), + ChangeNotifierProvider.value(value: multiServer), + ChangeNotifierProvider.value(value: offlineWatch), + ], + child: MaterialApp( + navigatorKey: rootNavigator, + home: ProfileSessionScreen.forTesting( + initialPromptHandled: true, + httpClientFactory: () => FakeHttpClient(200, const []), + profileShellBuilder: (context) => Column( + children: [ + Focus(focusNode: sidebar, child: const SizedBox(height: 10, width: 10)), + Focus(focusNode: content, autofocus: true, child: const SizedBox(height: 10, width: 10)), + ], + ), + ), + ), + ), + ); + await tester.pump(); + await tester.pump(const Duration(seconds: 1)); + expect(content.hasPrimaryFocus, isTrue); + + // The picker/PIN shape: pushed on the root navigator, above the whole + // nested profile-session navigator. + rootNavigator.currentState!.push( + MaterialPageRoute( + builder: (_) => Focus(focusNode: picker, autofocus: true, child: const SizedBox.expand()), + ), + ); + await tester.pump(); + await tester.pump(const Duration(seconds: 1)); + expect(picker.hasPrimaryFocus, isTrue); + + // A resume-time self-heal under MainScreen (sidebar reveal, grid load). + sidebar.requestFocus(); + await tester.pump(); + expect(sidebar.hasPrimaryFocus, isFalse, reason: 'covered session must not take focus'); + expect(picker.hasPrimaryFocus, isTrue); + + rootNavigator.currentState!.pop(); + await tester.pump(); + await tester.pump(const Duration(seconds: 1)); + expect(content.hasPrimaryFocus, isTrue, reason: 'focus returns to the session leaf that had it'); + }, + ); } void _expectCloseCount(Iterable clients, int expected) { diff --git a/test/screens/discover_screen_test.dart b/test/screens/discover_screen_test.dart index 737a19435..0150ae92b 100644 --- a/test/screens/discover_screen_test.dart +++ b/test/screens/discover_screen_test.dart @@ -750,6 +750,164 @@ void main() { expect(find.byType(SystemClock), findsOneWidget, reason: 'a fullscreen leanback app hides the system clock'); await tester.pumpWidget(const SizedBox()); }); + + testWidgets('a pending TV rail focus is dropped once the user has moved to the sidebar', (tester) async { + // The first load arms "focus the rail when it has hubs"; an empty-items + // hub keeps it armed because the rail has nothing to render yet. Items + // landing on a later pass must not pull focus off a sidebar item the + // user has since navigated to. + final gated = await _pumpGatedDiscover(tester); + gated.sidebarItem.requestFocus(); + await tester.pump(); + + await gated.landItems(tester); + + expect(find.byType(TvBrowseRail), findsOneWidget); + expect( + FocusManager.instance.primaryFocus, + same(gated.sidebarItem), + reason: 'items landing must not steal the sidebar', + ); + }); + + testWidgets('a pending TV rail focus still claims the rail while focus sits on a bare scope', (tester) async { + // Startup shape: MainScreen has focused its content scope but nothing + // below it yet. That is "nowhere", not a destination the user chose, so + // the rail keeps its initial-focus claim when hubs finally arrive. + final gated = await _pumpGatedDiscover(tester); + expect(FocusManager.instance.primaryFocus, isA()); + + await gated.landItems(tester); + + expect(FocusManager.instance.primaryFocus?.debugLabel, 'tv_browse_rail'); + }); +} + +class _GatedDiscover { + _GatedDiscover({required this.client, required this.sidebarItem}); + + final _GatedHubsFakeClient client; + final FocusNode sidebarItem; + + /// Replace the empty-items hub with a populated one and run a full pass. + Future landItems(WidgetTester tester) async { + client.hubs + ..clear() + ..add( + MediaHub( + id: 'hub_1', + title: 'Recommended', + type: 'movie', + items: [testMediaItem(id: 'movie_1', backend: MediaBackend.plex, kind: MediaKind.movie, title: 'Movie 1')], + size: 1, + ), + ); + (tester.state(find.byType(DiscoverScreen)) as FullRefreshable).fullRefresh(); + await tester.pump(); + client.release(); + await tester.pump(); + await tester.pump(); + } +} + +/// Mounts a TV [DiscoverScreen] beside a sidebar stand-in and completes its +/// first load with a hub that has no items, so the rail has nothing to render +/// and the screen's "focus the rail when ready" request stays armed. +Future<_GatedDiscover> _pumpGatedDiscover(WidgetTester tester) async { + await SettingsService.getInstance(); + tester.view.devicePixelRatio = 1.0; + tester.view.physicalSize = const Size(1280, 720); + addTearDown(() { + tester.view.resetDevicePixelRatio(); + tester.view.resetPhysicalSize(); + }); + + final client = _GatedHubsFakeClient( + hubs: [MediaHub(id: 'hub_1', title: 'Recommended', type: 'movie', items: const [], size: 0)], + ); + final multiServerProvider = testMultiServer(clients: [client]).provider; + final hiddenLibrariesProvider = HiddenLibrariesProvider(); + final librariesProvider = LibrariesProvider(); + final watchTogetherProvider = WatchTogetherProvider(); + final companionRemoteProvider = CompanionRemoteProvider(); + final db = AppDatabase.forTesting(NativeDatabase.memory()); + final connectionRegistry = _FakeConnectionRegistry(db); + final profileConnectionRegistry = _FakeProfileConnectionRegistry(db); + final storage = await StorageService.getInstance(); + final plexHome = PlexHomeService( + connections: connectionRegistry, + profileConnections: profileConnectionRegistry, + storage: storage, + plexHomeUserFetcher: (_) async => const [], + ); + final activeProfileProvider = ActiveProfileProvider( + registry: _FakeProfileRegistry(db), + plexHome: plexHome, + connections: connectionRegistry, + profileConnections: profileConnectionRegistry, + storage: storage, + ); + final discoverProvider = DiscoverProvider( + multiServerProvider, + hiddenLibrariesProvider, + librariesProvider, + profileId: null, + isProfileBinding: () => false, + ); + final sidebarItem = FocusNode(debugLabel: 'sidebar_item'); + addTearDown(() async { + sidebarItem.dispose(); + discoverProvider.dispose(); + activeProfileProvider.dispose(); + companionRemoteProvider.dispose(); + watchTogetherProvider.dispose(); + librariesProvider.dispose(); + hiddenLibrariesProvider.dispose(); + // multiServerProvider + its manager are torn down by testMultiServer. + await plexHome.dispose(); + await db.close(); + }); + + await tester.pumpWidget( + TranslationProvider( + child: MultiProvider( + providers: [ + ChangeNotifierProvider.value(value: multiServerProvider), + ChangeNotifierProvider.value(value: hiddenLibrariesProvider), + ChangeNotifierProvider.value(value: librariesProvider), + ChangeNotifierProvider.value(value: discoverProvider), + ChangeNotifierProvider.value(value: watchTogetherProvider), + ChangeNotifierProvider.value(value: companionRemoteProvider), + ChangeNotifierProvider.value(value: activeProfileProvider), + ], + child: InputModeTracker( + child: MaterialApp( + theme: monoTheme(dark: true), + home: Row( + children: [ + Focus(focusNode: sidebarItem, child: const SizedBox(width: 80, height: 720)), + MainScreenFocusScope( + focusSidebar: () {}, + sideNavigationWidth: SideNavigationRailState.expandedWidth, + reservedSideNavigationWidth: SideNavigationRailState.tvCollapsedWidth, + foregroundLeft: 80, + foregroundWidth: 1200, + viewportWidth: 1280, + child: const SizedBox(width: 1200, height: 720, child: DiscoverScreen()), + ), + ], + ), + ), + ), + ), + ), + ); + await tester.pump(); + client.release(); + await tester.pump(); + await tester.pump(); + expect(find.byType(TvBrowseRail), findsNothing, reason: 'an empty-items hub gives the rail nothing to show'); + return _GatedDiscover(client: client, sidebarItem: sidebarItem); } /// Mounts [DiscoverScreen] in the smallest graph both layout branches need, so