Skip to content

notif: Preserve target account context on back navigation after opening a notification #1373

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

notif: Preserve target account context on back navigation after opening a notification #1373

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 35 additions & 1 deletion lib/notifications/display.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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<dynamic> route, Route<void>? previousRoute) {
_updateActiveAccount(route);
}

@override
void didReplace({Route<dynamic>? newRoute, Route<void>? oldRoute}) {
if (newRoute != null) {
_updateActiveAccount(newRoute);
}
}

void _updateActiveAccount(Route<void> 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<void> init() async {
await NotificationChannelManager.ensureChannel();
}
Expand Down Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When currentAccountId (i.e., account ID associated with the top-most route) is null, routes below the top-most route can still belong to the same account. HomePage.navigate will pop those routes even though they belong to same account that receives this notification.

navigator.popUntil((r) => r.isFirst);
HomePage.navigate(context, accountId: route.accountId);
navigator = await ZulipApp.navigator;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The popUntil above unconditionally clears the navigator stack until there is only one route left. I believe that this is equivalent to HomePage.navigate, which also pops all routes beforehand.

Either way, we should not pop all routes unconditionally, because it doesn't feel like great user experience when the existing routes already belong to the same account.

#F1210 suggests that we

switch to a new nav stack for the notification's account (if different) before pushing the message-list route.

I think what we need here is just a way to keep track of the currently active account, or a way to inspect the navigator stack without actually popping the routes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See discussion here: #mobile > Beta: after open via notification, back opens old server @ 💬. Feel free to ask questions if you have any!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on my current implementation, I have addressed the following:

  1. Inspect the navigation stack without modifying it by using popUntil with return true.
  2. Only switch accounts (via HomePage.navigate) when the notification is for a different account than the current one (currentAccountId != route.accountId).
  3. When the notification is for the current account, preserve the existing navigation stack and simply push the new route on top, providing a better user experience.


unawaited(navigator.push(route));
}

Expand Down
1 change: 1 addition & 0 deletions lib/widgets/app.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,7 @@ class _ZulipAppState extends State<ZulipApp> with WidgetsBindingObserver {

navigatorKey: ZulipApp.navigatorKey,
navigatorObservers: [
NotificationDisplayManager.accountObserver,
if (widget.navigatorObservers != null)
...widget.navigatorObservers!,
_PreventEmptyStack(),
Expand Down
88 changes: 85 additions & 3 deletions test/notifications/display_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -959,6 +959,8 @@ void main() {

group('NotificationDisplayManager open', () {
late List<Route<void>> pushedRoutes;
late List<Route<void>> poppedRoutes;
late TestNavigatorObserver testNavObserver;

void takeStartingRoutes({Account? account, bool withAccount = true}) {
account ??= eg.selfAccount;
Expand All @@ -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]));
Expand Down Expand Up @@ -1018,7 +1026,7 @@ void main() {

Future<void> 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();
}

Expand Down Expand Up @@ -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<MaterialAccountWidgetRoute>()
.accountId.equals(accountA.id));
check(pushedRoutes.last).isA<MaterialAccountWidgetRoute>()
..accountId.equals(accountB.id)
..page.isA<MessageListPage>();
});

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<MaterialAccountWidgetRoute>()
..accountId.equals(account.id)
..page.isA<MessageListPage>();

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<MaterialAccountWidgetRoute>()
..accountId.equals(accountA.id)
..page.isA<MessageListPage>();

await openNotification(tester, accountB, message);
check(poppedRoutes).any((it) => it.isA<MaterialAccountWidgetRoute>()
.accountId.equals(accountA.id));
check(pushedRoutes.last).isA<MaterialAccountWidgetRoute>()
..accountId.equals(accountB.id)
..page.isA<MessageListPage>();
});
});

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also have a test where the top-most route is a non-AccountRoute.

group('NotificationOpenPayload', () {
Expand Down