diff --git a/lib/core/router/app_router.dart b/lib/core/router/app_router.dart index 5947b4ace..192e43d80 100644 --- a/lib/core/router/app_router.dart +++ b/lib/core/router/app_router.dart @@ -894,6 +894,21 @@ final appRouterProvider = Provider((ref) { child: const SettingsPage(), ), routes: [ + // Shared detail route for settings sections that have no + // dedicated page of their own (About, Units, Data, ...). + // Deliberately a child route with a plain builder: go_router + // then wraps it in the platform-adaptive MaterialPage, so these + // sections slide in exactly like '/settings/appearance'. + // Rendering them by re-matching '/settings?selected=' + // instead reused this route's NoTransitionPage and made them + // appear instantly. + GoRoute( + path: 'section/:sectionId', + name: 'settingsSection', + builder: (context, state) => SettingsSectionDetailPage( + sectionId: state.pathParameters['sectionId']!, + ), + ), GoRoute( path: 'storage', name: 'storageSettings', diff --git a/lib/features/settings/presentation/pages/settings_page.dart b/lib/features/settings/presentation/pages/settings_page.dart index 49d1fa44a..36a8007da 100644 --- a/lib/features/settings/presentation/pages/settings_page.dart +++ b/lib/features/settings/presentation/pages/settings_page.dart @@ -127,7 +127,7 @@ class SettingsPage extends ConsumerWidget { if (selectedSection != null) { // Show section detail page - return _SettingsSectionDetailPage(sectionId: selectedSection, ref: ref); + return SettingsSectionDetailPage(sectionId: selectedSection); } // Mobile: Show section list @@ -222,15 +222,16 @@ class SettingsMobileContent extends ConsumerWidget { } } -/// Mobile detail page for settings sections accessed via query params. -class _SettingsSectionDetailPage extends ConsumerWidget { +/// Mobile detail page for a single settings section. +/// +/// Reached by pushing the '/settings/section/:sectionId' child route, which +/// go_router wraps in a platform-adaptive page so the section slides in like +/// every other sub-page. Also rendered directly by [SettingsPage] for legacy +/// `/settings?selected=` deep links. +class SettingsSectionDetailPage extends ConsumerWidget { final String sectionId; - final WidgetRef ref; - const _SettingsSectionDetailPage({ - required this.sectionId, - required this.ref, - }); + const SettingsSectionDetailPage({super.key, required this.sectionId}); @override Widget build(BuildContext context, WidgetRef ref) { @@ -392,13 +393,16 @@ class _MobileSettingsTile extends StatelessWidget { context.push('/settings/appearance'); break; default: - // For sections that don't have dedicated pages, show them in a - // detail page using query params. PUSH (not go): go() replaces the - // location in place, leaving nothing on the stack for the system - // back gesture to pop, so Android closed the whole app (#647). - final state = GoRouterState.of(context); - final currentPath = state.uri.path; - context.push('$currentPath?selected=$sectionId'); + // Sections without a dedicated page get the shared section route. + // PUSH (not go): go() replaces the location in place, leaving nothing + // on the stack for the system back gesture to pop, so Android closed + // the whole app (#647). + // + // This pushes a child route rather than '/settings?selected='. + // The latter re-matched the '/settings' tab root, whose pageBuilder + // returns a NoTransitionPage so bottom-nav tab switches do not + // animate -- which also robbed every pushed section of its slide-in. + context.push('/settings/section/$sectionId'); } } } diff --git a/test/core/router/app_router_test.dart b/test/core/router/app_router_test.dart index 0f13b9b53..03feb5090 100644 --- a/test/core/router/app_router_test.dart +++ b/test/core/router/app_router_test.dart @@ -777,4 +777,91 @@ void main() { expect(await route.redirect!(capturedContext, state), isNull); }); }); + + group('settings sections are pushed as animated child routes', () { + // Settings sub-sections used to navigate two different ways. Sections + // with a dedicated route (Appearance -> /settings/appearance) push a + // child GoRoute, which go_router wraps in a platform-adaptive + // MaterialPage, so they slide in. Sections without one (About, Units, + // Data, ...) pushed '/settings?selected=', which re-matches the + // '/settings' route itself -- a bottom-nav tab root whose pageBuilder + // returns a NoTransitionPage. Correct for switching tabs, but it made + // those sections snap into place with no animation. + // + // The fix gives them a real child route. It deliberately does not make + // '/settings' itself animate when '?selected=' is present: the desktop + // master-detail pane navigates with go() (a stable pageKey), so swapping + // the page type under the same key would fail Page.canUpdate's + // runtimeType check and slide the whole split view on every click. + test('a section child route exists under /settings', () { + final route = _findRouteByName( + router.configuration.routes, + 'settingsSection', + ); + expect(route, isNotNull); + expect(route!.path, 'section/:sectionId'); + }); + + test('the section route uses builder, so go_router animates it', () { + final route = _findRouteByName( + router.configuration.routes, + 'settingsSection', + ); + expect(route, isNotNull); + expect( + route!.builder, + isNotNull, + reason: + 'builder lets go_router pick the platform-adaptive MaterialPage, ' + 'which is what makes /settings/appearance slide in.', + ); + expect( + route.pageBuilder, + isNull, + reason: + 'a custom pageBuilder here would risk reintroducing the ' + 'NoTransitionPage that suppressed the animation.', + ); + }); + + testWidgets('the settings tab root itself still has no transition', ( + tester, + ) async { + late BuildContext capturedContext; + await tester.pumpWidget( + MaterialApp( + home: Builder( + builder: (context) { + capturedContext = context; + return const SizedBox.shrink(); + }, + ), + ), + ); + + final route = _findRouteByName(router.configuration.routes, 'settings'); + expect(route, isNotNull); + expect(route!.pageBuilder, isNotNull); + + final page = route.pageBuilder!( + capturedContext, + GoRouterState( + router.configuration, + uri: Uri.parse('/settings'), + matchedLocation: '/settings', + fullPath: '/settings', + pathParameters: const {}, + pageKey: const ValueKey('/settings'), + ), + ); + + expect( + page, + isA>(), + reason: + '/settings is a bottom-nav destination; switching tabs must not ' + 'animate, matching every other tab root.', + ); + }); + }); } diff --git a/test/features/settings/presentation/pages/settings_page_test.dart b/test/features/settings/presentation/pages/settings_page_test.dart index b8bebaea3..3b4b34a93 100644 --- a/test/features/settings/presentation/pages/settings_page_test.dart +++ b/test/features/settings/presentation/pages/settings_page_test.dart @@ -1440,12 +1440,26 @@ void main() { await tester.binding.setSurfaceSize(const Size(400, 800)); addTearDown(() => tester.binding.setSurfaceSize(null)); + // Mirror the real route config: '/settings' is a bottom-nav tab root + // that must not animate when tabs are switched, and sections live on + // an animated child route beneath it. final router = GoRouter( initialLocation: initialLocation, routes: [ GoRoute( path: '/settings', - builder: (context, state) => const SettingsPage(), + pageBuilder: (context, state) => NoTransitionPage( + key: state.pageKey, + child: const SettingsPage(), + ), + routes: [ + GoRoute( + path: 'section/:sectionId', + builder: (context, state) => SettingsSectionDetailPage( + sectionId: state.pathParameters['sectionId']!, + ), + ), + ], ), ], ); @@ -1465,7 +1479,44 @@ void main() { return router; } - testWidgets('opening a query-param section pushes a poppable route so the ' + testWidgets('opening a section animates it into place instead of ' + 'snapping', (tester) async { + // Appearance slid in because it pushes a child GoRoute; About, Data and + // the rest re-matched the '/settings' tab root, whose NoTransitionPage + // suppressed the animation. Every section must now animate the same + // way. + final router = await pumpSettingsList(tester); + + await tester.scrollUntilVisible(find.text('Data'), 100); + await tester.tap(find.text('Data')); + await tester.pumpAndSettle(); + + expect( + router.state.uri.toString(), + '/settings/section/data', + reason: + 'the section must be its own child route; re-matching /settings ' + 'reuses that tab root page, which never animates', + ); + + // Assert on the route rather than a frame-by-frame position: the tap + // ripple keeps animations running either way, so only the pushed + // route's own transition duration distinguishes a slide from a snap. + final route = ModalRoute.of( + tester.element(find.byType(SettingsSectionDetailPage)), + ); + expect(route, isNotNull); + expect( + route!.transitionDuration, + greaterThan(Duration.zero), + reason: + 'a NoTransitionPage route has a zero-length transition, which is ' + 'exactly the snap this fixes', + ); + expect(tester.takeException(), isNull); + }); + + testWidgets('opening a section pushes a poppable route so the ' 'system back gesture returns to Settings instead of closing the app', ( tester, ) async {