Skip to content
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

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

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
13 changes: 13 additions & 0 deletions 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 @@ -502,6 +503,18 @@ class NotificationDisplayManager {
final route = routeForNotification(context: context, url: url);
if (route == null) return; // TODO(log)

Route<dynamic>? topRoute;
navigator.popUntil((r) {
topRoute = r;
return true;
});

if (topRoute == null || topRoute is! AccountRoute
|| (topRoute as AccountRoute).accountId != route.accountId) {
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!


// TODO(nav): Better interact with existing nav stack on notif open
unawaited(navigator.push(route));
}
Expand Down
43 changes: 43 additions & 0 deletions test/notifications/display_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1176,6 +1176,49 @@ void main() {
takeStartingRoutes(account: accountB);
matchesNavigation(check(pushedRoutes).single, accountB, message);
});

testWidgets('back navigation after opening a notification will preserve target account context', (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);

Route<dynamic>? topRoute;
final navigator = await ZulipApp.navigator;
navigator.popUntil((r) {
topRoute = r;
return true;
});
check(topRoute).isA<AccountRoute>().accountId.equals(accountA.id);
await openNotification(tester, accountB, message);

navigator.popUntil((r) {
topRoute = r;
return true;
});
check(topRoute).isA<MaterialAccountWidgetRoute>()
..accountId.equals(accountB.id)
..page.isA<MessageListPage>();

navigator.pop();
await tester.pumpAndSettle();

navigator.popUntil((r) {
topRoute = r;
return true;
});
check(topRoute).isA<MaterialAccountWidgetRoute>()
..accountId.equals(accountB.id)
..page.isA<HomePage>();
});
});

group('NotificationOpenPayload', () {
Expand Down