From 564e6e9070196226454cc27c4ceba6acb78abae7 Mon Sep 17 00:00:00 2001 From: Strycher Date: Sun, 26 Jul 2026 23:10:31 -0400 Subject: [PATCH] fix(#389): Back pops out of a channel; only top-level tabs background AppShell decided top-level vs detail by `selectedIndex != null`, but pushed detail screens (a channel chat, the LOS map) also set selectedIndex to keep the bottom bar visible. So Back from inside a channel backgrounded/left the app instead of popping to the channel list. Decouple the two concerns: add `isTopLevel` (default true), separate from `selectedIndex`. Detail screens pass `isTopLevel: false` (keep the bar, but Back pops). The decision is extracted into a pure `AppShell.backAction` (drawer -> close; detail with a route below -> pop; else -> background), with an assert that a detail is actually poppable. Unit tests cover the matrix; widget tests exercise the real system-Back -> PopScope -> pop wiring. Follow-up #390 tracks the separate AppBar back-arrow path. Co-Authored-By: Claude Opus 4.8 --- lib/screens/channel_chat_screen.dart | 3 + lib/screens/line_of_sight_map_screen.dart | 3 + lib/widgets/app_shell.dart | 92 ++++++++++++----- test/widgets/app_shell_back_test.dart | 116 ++++++++++++++++++++++ 4 files changed, 187 insertions(+), 27 deletions(-) create mode 100644 test/widgets/app_shell_back_test.dart diff --git a/lib/screens/channel_chat_screen.dart b/lib/screens/channel_chat_screen.dart index 6917817..dfa8959 100644 --- a/lib/screens/channel_chat_screen.dart +++ b/lib/screens/channel_chat_screen.dart @@ -347,6 +347,9 @@ class _ChannelChatScreenState extends State { // a channel on desktop, where there is no system back button and the // hamburger has taken the back arrow's place. selectedIndex: 1, + // Pushed detail: Back pops to the channel list, not to the background + // (#389). The bar above is only for tab highlighting/switching. + isTopLevel: false, onDestinationSelected: _handleQuickSwitch, contactsUnreadCount: context .watch() diff --git a/lib/screens/line_of_sight_map_screen.dart b/lib/screens/line_of_sight_map_screen.dart index 4a68c22..2e83150 100644 --- a/lib/screens/line_of_sight_map_screen.dart +++ b/lib/screens/line_of_sight_map_screen.dart @@ -411,6 +411,9 @@ class _LineOfSightMapScreenState extends State { return AppShell( selectedIndex: 2, + // Pushed detail (LOS analysis over the map): Back pops to the map, not to + // the background (#389). + isTopLevel: false, onDestinationSelected: (index) => _handleQuickSwitch(index, context), contactsUnreadCount: context .watch() diff --git a/lib/widgets/app_shell.dart b/lib/widgets/app_shell.dart index d07c3ec..106255f 100644 --- a/lib/widgets/app_shell.dart +++ b/lib/widgets/app_shell.dart @@ -6,6 +6,9 @@ import '../services/ui_view_state_service.dart'; import '../utils/app_backgrounder.dart'; import 'quick_switch_bar.dart'; +/// What the system Back button should do inside an [AppShell] (#389). +enum AppShellBackAction { closeDrawer, pop, background } + /// Shared shell for the primary views (Contacts / Channels / Map). /// /// Owns the bottom [QuickSwitchBar] that each view previously mounted itself, @@ -17,9 +20,18 @@ class AppShell extends StatefulWidget { static const double wideBreakpoint = 720; static const double _drawerWidth = 300; - /// Bottom bar tab. Null on pushed detail screens (a channel chat), which - /// carry the nav panel but no bottom bar. + /// Bottom bar tab to highlight. A pushed detail screen (a channel chat) still + /// sets this so the bar stays visible; it is NOT what decides Back behavior — + /// [isTopLevel] is. Null renders no bottom bar. final int? selectedIndex; + + /// Whether this is a genuine top-level landing screen — a bottom-bar tab + /// (Contacts/Channels/Map). On a top-level screen, Back sends the app to the + /// background; on a pushed detail screen (a channel chat, the LOS map) Back + /// pops to the list it came from. Kept separate from [selectedIndex] so a + /// detail can keep the bar visible without Back treating it as top-level + /// (#389). Defaults to true. + final bool isTopLevel; final ValueChanged? onDestinationSelected; final int contactsUnreadCount; final int channelsUnreadCount; @@ -48,6 +60,7 @@ class AppShell extends StatefulWidget { super.key, required this.body, this.selectedIndex, + this.isTopLevel = true, this.onDestinationSelected, this.appBar, this.appBarBuilder, @@ -59,6 +72,23 @@ class AppShell extends StatefulWidget { this.channelsUnreadCount = 0, }); + /// Pure back-button decision (#389), extracted so it is testable without the + /// widget tree. An open drawer closes first; a pushed detail ([isTopLevel] + /// false) that has a route below pops to its list; anything else — a + /// top-level tab, or a detail with nothing to pop — backgrounds the app. A + /// top-level tab CAN pop (the scanner sits below it) but must not, or Back + /// would strand the user on the radio-connect screen. + @visibleForTesting + static AppShellBackAction backAction({ + required bool drawerOpen, + required bool isTopLevel, + required bool canPop, + }) { + if (drawerOpen) return AppShellBackAction.closeDrawer; + if (!isTopLevel && canPop) return AppShellBackAction.pop; + return AppShellBackAction.background; + } + @override State createState() => _AppShellState(); } @@ -68,35 +98,43 @@ class _AppShellState extends State { /// System back, in priority order: /// 1. an open drawer closes, - /// 2. on a detail screen, pop back to the list it came from, - /// 3. on a primary view, send the app to the background so Android - /// returns to the home screen or the previous app. - /// - /// Step 3 must NOT pop, even though the route below can be popped. The - /// primary views sit on top of the scanner, so popping would dump a - /// connected user back onto the radio-connect list. Reaching the scanner is - /// what Disconnect is for, not what Back is for. + /// 2. on a pushed detail screen (a channel chat), pop back to the list it + /// came from, + /// 3. on a top-level tab, send the app to the background so Android returns + /// to the home screen or the previous app. /// - /// A primary view is one carrying the bottom bar; a detail screen (a channel - /// chat) has no [selectedIndex] and is genuinely pushed. + /// Top-level is decided by [AppShell.isTopLevel], NOT by whether a route can + /// be popped: a top-level tab sits on top of the scanner, so it CAN pop, but + /// popping would dump a connected user back onto the radio-connect list. + /// Reaching the scanner is what Disconnect is for, not what Back is for. A + /// detail screen keeps the bottom bar ([selectedIndex]) yet is not top-level, + /// so Back pops it (#389). Future _handleBack() async { final scaffold = _scaffoldKey.currentState; - if (scaffold?.isDrawerOpen ?? false) { - scaffold!.closeDrawer(); - return; - } - - final isPrimaryView = widget.selectedIndex != null; - final navigator = Navigator.of(context); - if (!isPrimaryView && navigator.canPop()) { - navigator.pop(); - return; + // Guardrail (#389): a screen marked as a pushed detail must actually be + // poppable, or Back would fall through to backgrounding the app instead of + // returning to its list. Catches a detail wired without a route below it. + assert( + widget.isTopLevel || Navigator.of(context).canPop(), + 'AppShell(isTopLevel: false) must be a pushed route so Back pops to its ' + 'list', + ); + final action = AppShell.backAction( + drawerOpen: scaffold?.isDrawerOpen ?? false, + isTopLevel: widget.isTopLevel, + canPop: Navigator.of(context).canPop(), + ); + switch (action) { + case AppShellBackAction.closeDrawer: + scaffold!.closeDrawer(); + case AppShellBackAction.pop: + Navigator.of(context).pop(); + case AppShellBackAction.background: + // Background, do NOT finish. SystemNavigator.pop() would call finish() + // on the activity, tearing down the Flutter engine and dropping the + // radio connection, so reopening would show a disconnected radio. + await AppBackgrounder.moveToBackground(); } - - // Background, do NOT finish. SystemNavigator.pop() would call finish() on - // the activity, tearing down the Flutter engine and dropping the radio - // connection, so reopening the app would show a disconnected radio. - await AppBackgrounder.moveToBackground(); } @override diff --git a/test/widgets/app_shell_back_test.dart b/test/widgets/app_shell_back_test.dart new file mode 100644 index 0000000..47b2fa8 --- /dev/null +++ b/test/widgets/app_shell_back_test.dart @@ -0,0 +1,116 @@ +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:meshcore_open/l10n/app_localizations.dart'; +import 'package:meshcore_open/services/ui_view_state_service.dart'; +import 'package:meshcore_open/widgets/app_shell.dart'; +import 'package:provider/provider.dart'; + +/// #389: Back must pop out of a pushed detail (a channel chat) to its list, and +/// background the app only from a genuine top-level tab — not the other way +/// round. These pin the full decision matrix. +void main() { + group('AppShell.backAction', () { + test('an open drawer closes first, regardless of anything else', () { + expect( + AppShell.backAction(drawerOpen: true, isTopLevel: false, canPop: true), + AppShellBackAction.closeDrawer, + ); + expect( + AppShell.backAction(drawerOpen: true, isTopLevel: true, canPop: true), + AppShellBackAction.closeDrawer, + ); + }); + + test('a pushed detail (channel chat) pops to its list', () { + expect( + AppShell.backAction(drawerOpen: false, isTopLevel: false, canPop: true), + AppShellBackAction.pop, + ); + }); + + test('a top-level tab backgrounds even though it CAN pop', () { + // The scanner sits below a top-level tab, so canPop is true, but popping + // would strand the user on the radio-connect screen. + expect( + AppShell.backAction(drawerOpen: false, isTopLevel: true, canPop: true), + AppShellBackAction.background, + ); + }); + + test('a detail with nothing left to pop backgrounds', () { + expect( + AppShell.backAction( + drawerOpen: false, + isTopLevel: false, + canPop: false, + ), + AppShellBackAction.background, + ); + }); + + test('a top-level with nothing to pop backgrounds', () { + expect( + AppShell.backAction(drawerOpen: false, isTopLevel: true, canPop: false), + AppShellBackAction.background, + ); + }); + }); + + group('system Back wiring', () { + Widget host(GlobalKey navKey) => MultiProvider( + providers: [ChangeNotifierProvider(create: (_) => UiViewStateService())], + child: MaterialApp( + navigatorKey: navKey, + localizationsDelegates: AppLocalizations.localizationsDelegates, + supportedLocales: AppLocalizations.supportedLocales, + home: const Scaffold(body: Center(child: Text('HOME'))), + ), + ); + + Future pushShell( + WidgetTester tester, + GlobalKey navKey, { + required bool isTopLevel, + }) async { + navKey.currentState!.push( + MaterialPageRoute( + builder: (_) => AppShell( + isTopLevel: isTopLevel, + selectedIndex: 1, + onDestinationSelected: (_) {}, + body: const Text('SHELL'), + ), + ), + ); + await tester.pumpAndSettle(); + } + + testWidgets('a pushed detail pops back to its list on system Back', ( + tester, + ) async { + final navKey = GlobalKey(); + await tester.pumpWidget(host(navKey)); + await pushShell(tester, navKey, isTopLevel: false); + expect(find.text('SHELL'), findsOneWidget); + + await tester.binding.handlePopRoute(); + await tester.pumpAndSettle(); + + expect(find.text('SHELL'), findsNothing, reason: 'popped to the list'); + expect(find.text('HOME'), findsOneWidget); + }); + + testWidgets('a top-level tab does NOT pop on system Back', (tester) async { + final navKey = GlobalKey(); + await tester.pumpWidget(host(navKey)); + await pushShell(tester, navKey, isTopLevel: true); + expect(find.text('SHELL'), findsOneWidget); + + await tester.binding.handlePopRoute(); + await tester.pumpAndSettle(); + + // moveToBackground is a no-op off Android, so the route stays put. + expect(find.text('SHELL'), findsOneWidget, reason: 'top-level stays'); + }); + }); +}