From e3a370dae35fae354b8e121e2c0eeec6dd5c3087 Mon Sep 17 00:00:00 2001 From: lakshya1goel Date: Sat, 5 Apr 2025 00:18:00 +0530 Subject: [PATCH] notif: Ensure back navigation preserves target account context after opening notification Fixes: #1210 --- lib/notifications/display.dart | 36 +++++++++++- lib/widgets/app.dart | 1 + test/notifications/display_test.dart | 88 +++++++++++++++++++++++++++- 3 files changed, 121 insertions(+), 4 deletions(-) diff --git a/lib/notifications/display.dart b/lib/notifications/display.dart index 1d74db6854..20e80de2db 100644 --- a/lib/notifications/display.dart +++ b/lib/notifications/display.dart @@ -17,6 +17,7 @@ import '../model/narrow.dart'; import '../widgets/app.dart'; import '../widgets/color.dart'; import '../widgets/dialog.dart'; +import '../widgets/home.dart'; import '../widgets/message_list.dart'; import '../widgets/page.dart'; import '../widgets/store.dart'; @@ -214,8 +215,34 @@ class NotificationChannelManager { } } +/// Tracks the currently active account in the navigation stack +class AccountNavigationObserver extends NavigatorObserver { + int? _activeAccountId; + int? get activeAccountId => _activeAccountId; + + @override + void didPush(Route route, Route? previousRoute) { + _updateActiveAccount(route); + } + + @override + void didReplace({Route? newRoute, Route? oldRoute}) { + if (newRoute != null) { + _updateActiveAccount(newRoute); + } + } + + void _updateActiveAccount(Route route) { + if (route is AccountRoute) { + _activeAccountId = route.accountId; + } + } +} + /// Service for managing the notifications shown to the user. class NotificationDisplayManager { + static final accountObserver = AccountNavigationObserver(); + static Future init() async { await NotificationChannelManager.ensureChannel(); } @@ -502,7 +529,14 @@ class NotificationDisplayManager { final route = routeForNotification(context: context, url: url); if (route == null) return; // TODO(log) - // TODO(nav): Better interact with existing nav stack on notif open + final currentAccountId = accountObserver.activeAccountId; + + if (currentAccountId != route.accountId) { + navigator.popUntil((r) => r.isFirst); + HomePage.navigate(context, accountId: route.accountId); + navigator = await ZulipApp.navigator; + } + unawaited(navigator.push(route)); } diff --git a/lib/widgets/app.dart b/lib/widgets/app.dart index d2f5b36e1d..2c5b784265 100644 --- a/lib/widgets/app.dart +++ b/lib/widgets/app.dart @@ -231,6 +231,7 @@ class _ZulipAppState extends State with WidgetsBindingObserver { navigatorKey: ZulipApp.navigatorKey, navigatorObservers: [ + NotificationDisplayManager.accountObserver, if (widget.navigatorObservers != null) ...widget.navigatorObservers!, _PreventEmptyStack(), diff --git a/test/notifications/display_test.dart b/test/notifications/display_test.dart index b1c56b55b1..6237eaec36 100644 --- a/test/notifications/display_test.dart +++ b/test/notifications/display_test.dart @@ -959,6 +959,8 @@ void main() { group('NotificationDisplayManager open', () { late List> pushedRoutes; + late List> poppedRoutes; + late TestNavigatorObserver testNavObserver; void takeStartingRoutes({Account? account, bool withAccount = true}) { account ??= eg.selfAccount; @@ -978,8 +980,14 @@ void main() { {bool early = false, bool withAccount = true}) async { await init(); pushedRoutes = []; - final testNavObserver = TestNavigatorObserver() - ..onPushed = (route, prevRoute) => pushedRoutes.add(route); + poppedRoutes = []; + testNavObserver = TestNavigatorObserver(); + testNavObserver.onPushed = (route, prevRoute) => pushedRoutes.add(route); + testNavObserver.onPopped = (route, prevRoute) => poppedRoutes.add(route); + testNavObserver.onReplaced = (route, prevRoute) { + poppedRoutes.add(prevRoute!); + pushedRoutes.add(route!); + }; // This uses [ZulipApp] instead of [TestZulipApp] because notification // logic uses `await ZulipApp.navigator`. await tester.pumpWidget(ZulipApp(navigatorObservers: [testNavObserver])); @@ -1018,7 +1026,7 @@ void main() { Future checkOpenNotification(WidgetTester tester, Account account, Message message) async { await openNotification(tester, account, message); - matchesNavigation(check(pushedRoutes).single, account, message); + check(pushedRoutes).any((it) => matchesNavigation(it, account, message)); pushedRoutes.clear(); } @@ -1176,6 +1184,80 @@ void main() { takeStartingRoutes(account: accountB); matchesNavigation(check(pushedRoutes).single, accountB, message); }); + + testWidgets('notification switches account only when from different account', (tester) async { + addTearDown(testBinding.reset); + + final accountA = eg.selfAccount; + final accountB = eg.otherAccount; + final message = eg.streamMessage(); + + await testBinding.globalStore.add(accountA, eg.initialSnapshot()); + await testBinding.globalStore.add(accountB, eg.initialSnapshot()); + + await prepare(tester, early: true); + await tester.pump(); + takeStartingRoutes(account: accountA); + + await openNotification(tester, accountB, message); + check(poppedRoutes).any((it) => it.isA() + .accountId.equals(accountA.id)); + check(pushedRoutes.last).isA() + ..accountId.equals(accountB.id) + ..page.isA(); + }); + + testWidgets('notification preserves navigation stack when in same account', (tester) async { + addTearDown(testBinding.reset); + + final account = eg.selfAccount; + final message = eg.streamMessage(); + + await testBinding.globalStore.add(account, eg.initialSnapshot()); + + await prepare(tester, early: true); + await tester.pump(); + takeStartingRoutes(account: account); + + await openNotification(tester, account, message); + check(pushedRoutes.last).isA() + ..accountId.equals(account.id) + ..page.isA(); + + check(poppedRoutes).isEmpty(); + }); + + testWidgets('notification keeps AccountRoute in stack when opened from non-AccountRoute', (tester) async { + addTearDown(testBinding.reset); + + final accountA = eg.selfAccount; + final accountB = eg.otherAccount; + final message = eg.streamMessage(); + + await testBinding.globalStore.add(accountA, eg.initialSnapshot()); + await testBinding.globalStore.add(accountB, eg.initialSnapshot()); + + await prepare(tester, early: true); + await tester.pump(); + takeStartingRoutes(account: accountA); + + final navigator = await ZulipApp.navigator; + unawaited(navigator.push(MaterialWidgetRoute(page: const ChooseAccountPage()))); + await tester.pumpAndSettle(); + + await openNotification(tester, accountA, message); + check(poppedRoutes).isEmpty(); + check(pushedRoutes.last).isA() + ..accountId.equals(accountA.id) + ..page.isA(); + + await openNotification(tester, accountB, message); + check(poppedRoutes).any((it) => it.isA() + .accountId.equals(accountA.id)); + check(pushedRoutes.last).isA() + ..accountId.equals(accountB.id) + ..page.isA(); + }); }); group('NotificationOpenPayload', () {