Skip to content

Commit e3a370d

Browse files
committed
notif: Ensure back navigation preserves target account context after opening notification
Fixes: #1210
1 parent 0494723 commit e3a370d

File tree

3 files changed

+121
-4
lines changed

3 files changed

+121
-4
lines changed

lib/notifications/display.dart

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import '../model/narrow.dart';
1717
import '../widgets/app.dart';
1818
import '../widgets/color.dart';
1919
import '../widgets/dialog.dart';
20+
import '../widgets/home.dart';
2021
import '../widgets/message_list.dart';
2122
import '../widgets/page.dart';
2223
import '../widgets/store.dart';
@@ -214,8 +215,34 @@ class NotificationChannelManager {
214215
}
215216
}
216217

218+
/// Tracks the currently active account in the navigation stack
219+
class AccountNavigationObserver extends NavigatorObserver {
220+
int? _activeAccountId;
221+
int? get activeAccountId => _activeAccountId;
222+
223+
@override
224+
void didPush(Route<dynamic> route, Route<void>? previousRoute) {
225+
_updateActiveAccount(route);
226+
}
227+
228+
@override
229+
void didReplace({Route<dynamic>? newRoute, Route<void>? oldRoute}) {
230+
if (newRoute != null) {
231+
_updateActiveAccount(newRoute);
232+
}
233+
}
234+
235+
void _updateActiveAccount(Route<void> route) {
236+
if (route is AccountRoute) {
237+
_activeAccountId = route.accountId;
238+
}
239+
}
240+
}
241+
217242
/// Service for managing the notifications shown to the user.
218243
class NotificationDisplayManager {
244+
static final accountObserver = AccountNavigationObserver();
245+
219246
static Future<void> init() async {
220247
await NotificationChannelManager.ensureChannel();
221248
}
@@ -502,7 +529,14 @@ class NotificationDisplayManager {
502529
final route = routeForNotification(context: context, url: url);
503530
if (route == null) return; // TODO(log)
504531

505-
// TODO(nav): Better interact with existing nav stack on notif open
532+
final currentAccountId = accountObserver.activeAccountId;
533+
534+
if (currentAccountId != route.accountId) {
535+
navigator.popUntil((r) => r.isFirst);
536+
HomePage.navigate(context, accountId: route.accountId);
537+
navigator = await ZulipApp.navigator;
538+
}
539+
506540
unawaited(navigator.push(route));
507541
}
508542

lib/widgets/app.dart

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -231,6 +231,7 @@ class _ZulipAppState extends State<ZulipApp> with WidgetsBindingObserver {
231231

232232
navigatorKey: ZulipApp.navigatorKey,
233233
navigatorObservers: [
234+
NotificationDisplayManager.accountObserver,
234235
if (widget.navigatorObservers != null)
235236
...widget.navigatorObservers!,
236237
_PreventEmptyStack(),

test/notifications/display_test.dart

Lines changed: 85 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -959,6 +959,8 @@ void main() {
959959

960960
group('NotificationDisplayManager open', () {
961961
late List<Route<void>> pushedRoutes;
962+
late List<Route<void>> poppedRoutes;
963+
late TestNavigatorObserver testNavObserver;
962964

963965
void takeStartingRoutes({Account? account, bool withAccount = true}) {
964966
account ??= eg.selfAccount;
@@ -978,8 +980,14 @@ void main() {
978980
{bool early = false, bool withAccount = true}) async {
979981
await init();
980982
pushedRoutes = [];
981-
final testNavObserver = TestNavigatorObserver()
982-
..onPushed = (route, prevRoute) => pushedRoutes.add(route);
983+
poppedRoutes = [];
984+
testNavObserver = TestNavigatorObserver();
985+
testNavObserver.onPushed = (route, prevRoute) => pushedRoutes.add(route);
986+
testNavObserver.onPopped = (route, prevRoute) => poppedRoutes.add(route);
987+
testNavObserver.onReplaced = (route, prevRoute) {
988+
poppedRoutes.add(prevRoute!);
989+
pushedRoutes.add(route!);
990+
};
983991
// This uses [ZulipApp] instead of [TestZulipApp] because notification
984992
// logic uses `await ZulipApp.navigator`.
985993
await tester.pumpWidget(ZulipApp(navigatorObservers: [testNavObserver]));
@@ -1018,7 +1026,7 @@ void main() {
10181026

10191027
Future<void> checkOpenNotification(WidgetTester tester, Account account, Message message) async {
10201028
await openNotification(tester, account, message);
1021-
matchesNavigation(check(pushedRoutes).single, account, message);
1029+
check(pushedRoutes).any((it) => matchesNavigation(it, account, message));
10221030
pushedRoutes.clear();
10231031
}
10241032

@@ -1176,6 +1184,80 @@ void main() {
11761184
takeStartingRoutes(account: accountB);
11771185
matchesNavigation(check(pushedRoutes).single, accountB, message);
11781186
});
1187+
1188+
testWidgets('notification switches account only when from different account', (tester) async {
1189+
addTearDown(testBinding.reset);
1190+
1191+
final accountA = eg.selfAccount;
1192+
final accountB = eg.otherAccount;
1193+
final message = eg.streamMessage();
1194+
1195+
await testBinding.globalStore.add(accountA, eg.initialSnapshot());
1196+
await testBinding.globalStore.add(accountB, eg.initialSnapshot());
1197+
1198+
await prepare(tester, early: true);
1199+
await tester.pump();
1200+
takeStartingRoutes(account: accountA);
1201+
1202+
await openNotification(tester, accountB, message);
1203+
check(poppedRoutes).any((it) => it.isA<MaterialAccountWidgetRoute>()
1204+
.accountId.equals(accountA.id));
1205+
check(pushedRoutes.last).isA<MaterialAccountWidgetRoute>()
1206+
..accountId.equals(accountB.id)
1207+
..page.isA<MessageListPage>();
1208+
});
1209+
1210+
testWidgets('notification preserves navigation stack when in same account', (tester) async {
1211+
addTearDown(testBinding.reset);
1212+
1213+
final account = eg.selfAccount;
1214+
final message = eg.streamMessage();
1215+
1216+
await testBinding.globalStore.add(account, eg.initialSnapshot());
1217+
1218+
await prepare(tester, early: true);
1219+
await tester.pump();
1220+
takeStartingRoutes(account: account);
1221+
1222+
await openNotification(tester, account, message);
1223+
check(pushedRoutes.last).isA<MaterialAccountWidgetRoute>()
1224+
..accountId.equals(account.id)
1225+
..page.isA<MessageListPage>();
1226+
1227+
check(poppedRoutes).isEmpty();
1228+
});
1229+
1230+
testWidgets('notification keeps AccountRoute in stack when opened from non-AccountRoute', (tester) async {
1231+
addTearDown(testBinding.reset);
1232+
1233+
final accountA = eg.selfAccount;
1234+
final accountB = eg.otherAccount;
1235+
final message = eg.streamMessage();
1236+
1237+
await testBinding.globalStore.add(accountA, eg.initialSnapshot());
1238+
await testBinding.globalStore.add(accountB, eg.initialSnapshot());
1239+
1240+
await prepare(tester, early: true);
1241+
await tester.pump();
1242+
takeStartingRoutes(account: accountA);
1243+
1244+
final navigator = await ZulipApp.navigator;
1245+
unawaited(navigator.push(MaterialWidgetRoute(page: const ChooseAccountPage())));
1246+
await tester.pumpAndSettle();
1247+
1248+
await openNotification(tester, accountA, message);
1249+
check(poppedRoutes).isEmpty();
1250+
check(pushedRoutes.last).isA<MaterialAccountWidgetRoute>()
1251+
..accountId.equals(accountA.id)
1252+
..page.isA<MessageListPage>();
1253+
1254+
await openNotification(tester, accountB, message);
1255+
check(poppedRoutes).any((it) => it.isA<MaterialAccountWidgetRoute>()
1256+
.accountId.equals(accountA.id));
1257+
check(pushedRoutes.last).isA<MaterialAccountWidgetRoute>()
1258+
..accountId.equals(accountB.id)
1259+
..page.isA<MessageListPage>();
1260+
});
11791261
});
11801262

11811263
group('NotificationOpenPayload', () {

0 commit comments

Comments
 (0)