From aa7a921943ec971b8d22877d9dea1d9a2c3d9af1 Mon Sep 17 00:00:00 2001 From: Michael Goodness Date: Fri, 24 Jul 2026 10:36:20 -0500 Subject: [PATCH] fix(buddies): show Add New Buddy screen in front of the bulk-edit dialog Bulk-editing dives opens the buddy picker inside a showDialog (root navigator by default); the buddy-selection bottom sheet resolves to that same root navigator. The newBuddy route, however, is nested under the app's ShellRoute with no parentNavigatorKey override, so it mounted on the shell's own nested navigator instead -- rendering underneath the still-open dialog/sheet on the root navigator. Users had to back out of both to find the new-buddy screen was open the whole time. Give newBuddy a parentNavigatorKey pointing at the root navigator so it always escapes to wherever the dialog/sheet actually are. Fixes #707 Co-Authored-By: Claude Sonnet 5 --- lib/core/router/app_router.dart | 7 + test/core/router/app_router_test.dart | 29 +++ .../buddy_picker_navigation_render_test.dart | 241 ++++++++++++++++++ 3 files changed, 277 insertions(+) create mode 100644 test/features/buddies/presentation/widgets/buddy_picker_navigation_render_test.dart diff --git a/lib/core/router/app_router.dart b/lib/core/router/app_router.dart index de94190f3..9a1941ac1 100644 --- a/lib/core/router/app_router.dart +++ b/lib/core/router/app_router.dart @@ -540,6 +540,13 @@ final appRouterProvider = Provider((ref) { GoRoute( path: 'new', name: 'newBuddy', + // Bulk dive editing opens the buddy picker inside a + // showDialog (root navigator by default); "Add New Buddy" + // must land on that same root navigator or it renders + // underneath the still-open dialog/bottom sheet instead of + // in the foreground (see app_router_test.dart and + // buddy_picker_navigation_render_test.dart). + parentNavigatorKey: rootNavigatorKey, builder: (context, state) { final extra = state.extra as Map?; return BuddyEditPage( diff --git a/test/core/router/app_router_test.dart b/test/core/router/app_router_test.dart index 6a8ac157d..9ec8a94d7 100644 --- a/test/core/router/app_router_test.dart +++ b/test/core/router/app_router_test.dart @@ -659,6 +659,35 @@ void main() { }); }); + group('newBuddy route escapes to the root navigator', () { + // Regression: bulk-editing dives opens "Add buddies" via showDialog + // (useRootNavigator: true by default), then BuddyPicker opens a + // showModalBottomSheet (which resolves to that same root navigator, + // since its default useRootNavigator: false picks the nearest + // navigator ancestor -- the dialog's). Tapping "Add New Buddy" pushes + // 'newBuddy', which -- absent a parentNavigatorKey -- mounts on the + // ShellRoute's nested navigator instead. That nested navigator's + // overlay paints underneath the root navigator's, so the new-buddy + // screen renders hidden behind the still-open dialog and bottom sheet + // until both are dismissed. See buddy_picker_navigation_render_test.dart + // for the full render-level reproduction, including confirmation that + // the dialog/sheet correctly reappear (with state intact) once the + // new-buddy page is popped -- pushing onto the root navigator does not + // evict them, it only elides their (harmless, Flutter-standard) + // rendering while a fully opaque route covers them. + test('newBuddy has parentNavigatorKey set to the root navigator', () { + final route = _findRouteByName(router.configuration.routes, 'newBuddy'); + expect(route, isNotNull); + expect( + route!.parentNavigatorKey, + same(rootNavigatorKey), + reason: + 'Without this, "Add New Buddy" pushed from the bulk-edit ' + 'dialog renders underneath it instead of in the foreground.', + ); + }); + }); + group('app_router lightroom route (pending Adobe review)', () { test('lightroom route stays defined so navigation degrades gracefully', () { // The route is intentionally kept (not removed) while the UI is hidden so diff --git a/test/features/buddies/presentation/widgets/buddy_picker_navigation_render_test.dart b/test/features/buddies/presentation/widgets/buddy_picker_navigation_render_test.dart new file mode 100644 index 000000000..9feadbc40 --- /dev/null +++ b/test/features/buddies/presentation/widgets/buddy_picker_navigation_render_test.dart @@ -0,0 +1,241 @@ +import 'package:flutter/material.dart'; +import 'package:flutter_riverpod/flutter_riverpod.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:go_router/go_router.dart'; +import 'package:shared_preferences/shared_preferences.dart'; + +import 'package:submersion/core/providers/provider.dart'; +import 'package:submersion/features/buddies/data/repositories/buddy_repository.dart'; +import 'package:submersion/features/buddies/domain/entities/buddy.dart'; +import 'package:submersion/features/buddies/presentation/pages/buddy_edit_page.dart'; +import 'package:submersion/features/buddies/presentation/widgets/buddy_picker.dart'; +import 'package:submersion/features/divers/data/repositories/diver_repository.dart'; +import 'package:submersion/features/divers/domain/entities/diver.dart'; +import 'package:submersion/features/divers/presentation/providers/diver_providers.dart'; +import 'package:submersion/features/settings/presentation/providers/settings_providers.dart'; +import 'package:submersion/l10n/arb/app_localizations.dart'; + +import '../../../../helpers/mock_providers.dart'; +import '../../../../helpers/test_database.dart'; + +// Regression coverage for the bug where "Add New Buddy" during bulk dive +// editing pushed a new-buddy screen that rendered underneath the still-open +// dialog/bottom sheet -- the user had to back out twice to see it. +// +// Root cause: bulk-edit opens BuddyPicker inside showDialog (root navigator +// by default); the buddy-selection bottom sheet then also resolves to that +// root navigator. But 'newBuddy' is a GoRoute nested under the app's +// ShellRoute, which -- absent a parentNavigatorKey override -- mounts pages +// on the shell's own nested navigator instead of root. That nested +// navigator's overlay paints underneath the root navigator's, so the new +// page rendered, but hidden behind the still-open dialog/sheet. +// +// The fix (app_router.dart) gives 'newBuddy' `parentNavigatorKey: +// rootNavigatorKey` so it always mounts on the root navigator, matching +// wherever the dialog/sheet actually are. +// +// A go_router push does temporarily elide the dialog/sheet from the render +// tree while BuddyEditPage (fully opaque) covers them -- confirmed via a +// NavigatorObserver to be Flutter's standard "don't paint/build routes +// fully covered by an opaque route" behavior, NOT a pop: the DialogRoute is +// never popped or removed, and reappears with its state intact as soon as +// the covering route is popped. These tests assert on that full round +// trip -- the real proof the fix works, rather than just checking router +// config. + +/// Mirrors DiveEditPage._bulkAddBuddies: an AlertDialog hosting BuddyPicker, +/// opened via showDialog's default useRootNavigator: true. +class _BulkAddBuddiesHost extends StatelessWidget { + const _BulkAddBuddiesHost(); + + @override + Widget build(BuildContext context) { + return Scaffold( + body: Center( + child: ElevatedButton( + onPressed: () { + var picked = []; + showDialog( + context: context, + builder: (ctx) => AlertDialog( + content: SizedBox( + width: 400, + height: 500, + child: StatefulBuilder( + builder: (ctx, setSt) => BuddyPicker( + selectedBuddies: picked, + onChanged: (b) => setSt(() => picked = b), + ), + ), + ), + ), + ); + }, + child: const Text('Open bulk add buddies'), + ), + ), + ); + } +} + +/// Mirrors the single-dive-edit flow: BuddyPicker embedded directly in the +/// page, with no dialog wrapper. +class _EmbeddedPickerHost extends StatefulWidget { + const _EmbeddedPickerHost(); + + @override + State<_EmbeddedPickerHost> createState() => _EmbeddedPickerHostState(); +} + +class _EmbeddedPickerHostState extends State<_EmbeddedPickerHost> { + var _selected = []; + + @override + Widget build(BuildContext context) { + return Scaffold( + body: BuddyPicker( + selectedBuddies: _selected, + onChanged: (b) => setState(() => _selected = b), + ), + ); + } +} + +void main() { + late BuddyRepository buddyRepo; + late SharedPreferences prefs; + + setUp(() async { + await setUpTestDatabase(); + SharedPreferences.setMockInitialValues({}); + prefs = await SharedPreferences.getInstance(); + buddyRepo = BuddyRepository(); + + final diver = await DiverRepository().createDiver( + Diver( + id: '', + name: 'D', + isDefault: true, + createdAt: DateTime(2024), + updatedAt: DateTime(2024), + ), + ); + await prefs.setString(currentDiverIdKey, diver.id); + }); + + tearDown(() async { + await tearDownTestDatabase(); + }); + + // Mirrors the app's real router shape (see app_router.dart): a ShellRoute + // with a nested navigator, and 'newBuddy' escaping to the root navigator + // via parentNavigatorKey. + Widget buildApp(Widget host) { + final rootKey = GlobalKey(debugLabel: 'root'); + final router = GoRouter( + navigatorKey: rootKey, + initialLocation: '/host', + routes: [ + ShellRoute( + builder: (context, state, child) => child, + routes: [ + GoRoute(path: '/host', builder: (context, state) => host), + GoRoute( + path: '/buddies', + builder: (context, state) => const Text('Buddies list'), + routes: [ + GoRoute( + path: 'new', + name: 'newBuddy', + parentNavigatorKey: rootKey, + builder: (context, state) => const BuddyEditPage(), + ), + ], + ), + ], + ), + ], + ); + return ProviderScope( + overrides: [ + sharedPreferencesProvider.overrideWithValue(prefs), + settingsProvider.overrideWith((ref) => MockSettingsNotifier()), + ], + child: MaterialApp.router( + routerConfig: router, + localizationsDelegates: AppLocalizations.localizationsDelegates, + supportedLocales: AppLocalizations.supportedLocales, + ), + ); + } + + testWidgets( + 'Add New Buddy pushed from inside the bulk-edit dialog renders the real ' + 'BuddyEditPage on top, and the dialog/sheet reappear intact once it is ' + 'saved and popped', + (tester) async { + await tester.pumpWidget(buildApp(const _BulkAddBuddiesHost())); + await tester.pumpAndSettle(); + + // Mirrors DiveEditPage._bulkAddBuddies opening the buddies dialog. + await tester.tap(find.text('Open bulk add buddies')); + await tester.pumpAndSettle(); + expect(find.byType(AlertDialog), findsOneWidget); + + // Open the buddy-selection bottom sheet from inside the dialog. + await tester.tap(find.byIcon(Icons.add)); + await tester.pumpAndSettle(); + expect(find.byType(DraggableScrollableSheet), findsOneWidget); + + // Tap "Add New Buddy" -- BuddyPicker pushes the real BuddyEditPage. + await tester.tap(find.byIcon(Icons.person_add)); + await tester.pumpAndSettle(); + + // The real proof of the fix: BuddyEditPage is on top and interactive. + // (The dialog/sheet are elided from the tree while it's on top -- a + // harmless Flutter optimization for fully-opaque covering routes, not + // a dismissal -- so they are deliberately not asserted on here.) + expect(find.byType(BuddyEditPage), findsOneWidget); + + // Complete the round trip: save a new buddy so it pops back via + // context.pop(savedBuddy), and confirm the dialog/sheet reappear with + // their original state intact -- proving nothing was lost while + // covered. + await tester.enterText(find.byType(TextFormField).first, 'New Diver'); + await tester.tap(find.text('Save')); + await tester.pumpAndSettle(); + + expect(find.byType(BuddyEditPage), findsNothing); + expect(find.byType(AlertDialog), findsOneWidget); + expect(find.byType(DraggableScrollableSheet), findsOneWidget); + expect(find.text('New Diver'), findsOneWidget); + expect((await buddyRepo.getAllBuddies()).single.name, 'New Diver'); + }, + ); + + testWidgets( + 'Add New Buddy pushed with no dialog involved (single-dive-edit style) ' + 'still shows the real BuddyEditPage on top -- the root-navigator fix ' + 'does not regress this flow', + (tester) async { + await tester.pumpWidget(buildApp(const _EmbeddedPickerHost())); + await tester.pumpAndSettle(); + + await tester.tap(find.byIcon(Icons.add)); + await tester.pumpAndSettle(); + expect(find.byType(DraggableScrollableSheet), findsOneWidget); + + await tester.tap(find.byIcon(Icons.person_add)); + await tester.pumpAndSettle(); + + expect(find.byType(BuddyEditPage), findsOneWidget); + + await tester.enterText(find.byType(TextFormField).first, 'Another One'); + await tester.tap(find.text('Save')); + await tester.pumpAndSettle(); + + expect(find.byType(BuddyEditPage), findsNothing); + expect((await buddyRepo.getAllBuddies()).single.name, 'Another One'); + }, + ); +}