Skip to content

adds search icon to inbox and combined feed pages #1864

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 2 commits into
base: main
Choose a base branch
from
Open

adds search icon to inbox and combined feed pages #1864

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8159ca7
home: Add search button to Inbox page
Oct 6, 2025
4772ff8
message_list: Add search button to combined feed
Oct 6, 2025
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
21 changes: 20 additions & 1 deletion lib/widgets/home.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,23 @@ class _HomePageState extends State<HomePage> {
}
}

List<Widget>? get _currentTabAppBarActions {
Copy link
Collaborator

Choose a reason for hiding this comment

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

home: Add search button and related tests.

Adds a search icon and tooltip to the icon in the home page app bar
that appears on the inbox tab. Uses the same localized 'Search' text
for consistency with the message list search button.

Also refactors the app bar actions logic by extracting it into a
_currentTabAppBarActions getter.

Updates the existing bottom nav navigation test inside home_test to verify search button presence
across different tabs: present on inbox, absent on channels and
direct messages tabs.

Also updates the existing InboxPage tests to verify the search button navigates to search page when tapped

Let's shorten the commit message down to just what's needed:

  • Tests are always part of the process for building new features, so we don't normally mention them in commit messages.
  • This one should say Fixes-partly: #1854, and the next one (which completes the fix) should say Fixes #1854.
  • Let's mention the "Inbox" page in the summary line. There's room to do that, and it could be helpful for someone who's only reading the summary lines. Then the first sentence in the body is redundant and can be removed.
  • The sentence about _currentTabAppBarActions isn't quite accurate: it's not that we're really changing/refactoring existing logic; rather it's more like we're adding logic that wasn't there before. There isn't anything very remarkable about how we're doing this, so I would just remove the sentence.
  • This sentence doesn't make sense to me: "Uses the same localized 'Search' text
    for consistency with the message list search button." At this commit, "message list search button" doesn't refer to anything that exists, so it's confusing. You could rewrite it to make the context clear, by mentioning the plan to add a similar button the "Combined feed", which is done in the next commit…but you don't need to; the reader can easily look at the code if they're worried abou an inconsistency.

So, putting all that together, it would be something like:

home: Add search button to Inbox page

Fixes-partly: #1854

Copy link
Author

Choose a reason for hiding this comment

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

Updated the commit message as per suggestion.

switch(_tab.value) {
case _HomePageTab.inbox:
return [
IconButton(
icon: const Icon(ZulipIcons.search),
tooltip: ZulipLocalizations.of(context).searchMessagesPageTitle,
onPressed: () => Navigator.of(context).push(MessageListPage.buildRoute(
context: context, narrow: KeywordSearchNarrow(''))),
),
Comment on lines +94 to +96
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
onPressed: () => Navigator.of(context).push(MessageListPage.buildRoute(
context: context, narrow: KeywordSearchNarrow(''))),
),
onPressed: () => Navigator.of(context).push(MessageListPage.buildRoute(
context: context, narrow: KeywordSearchNarrow('')))),

Comment on lines +94 to +96
Copy link
Member

Choose a reason for hiding this comment

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

nit: more generally, make this look as similar as possible to the other navigation callback found in this same widget. That way the only substantive difference between them — the difference in what narrow to go to — stands out.

Similarly, make this look identical to the navigation callback added in your other commit, which is intended to have an identical effect.

In particular that means use Navigator.push(context, …) rather than Navigator.of(context).push(…). (They mean the same thing, so avoid making it look like there's a difference.) Also make the formatting look the same.

];
case _HomePageTab.channels:
case _HomePageTab.directMessages:
return null;
}
}

@override
Widget build(BuildContext context) {
const pageBodies = [
Expand Down Expand Up @@ -120,7 +137,9 @@ class _HomePageState extends State<HomePage> {
final designVariables = DesignVariables.of(context);
return Scaffold(
appBar: ZulipAppBar(titleSpacing: 16,
title: Text(_currentTabTitle)),
title: Text(_currentTabTitle),
actions: _currentTabAppBarActions
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
actions: _currentTabAppBarActions
actions: _currentTabAppBarActions,

(or else put the closing ) on the same line)

),
body: Stack(
children: [
for (final (tab, body) in pageBodies)
Expand Down
6 changes: 6 additions & 0 deletions lib/widgets/message_list.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,12 @@ abstract class _MessageListAppBar {
List<Widget> actions = [];
switch (narrow) {
case CombinedFeedNarrow():
actions.add(IconButton(
Copy link
Member

Choose a reason for hiding this comment

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

nit in commit message:

message_list: Add search button to combined feed

In our commit-message summary lines, the message list is so frequent that we use an abbreviation for it: msglist:.

icon: const Icon(ZulipIcons.search),
tooltip: zulipLocalizations.searchMessagesPageTitle,
onPressed: () => Navigator.push(context,
MessageListPage.buildRoute(context: context,
narrow: KeywordSearchNarrow('')))));
case MentionsNarrow():
case StarredMessagesNarrow():
case KeywordSearchNarrow():
Expand Down
9 changes: 8 additions & 1 deletion test/widgets/home_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,24 +111,31 @@ void main () {
check(find.byIcon(ZulipIcons.arrow_right)).findsExactly(2);
});

testWidgets('update app bar title when switching between views', (tester) async {
testWidgets('update app bar title and actions when switching between views', (tester) async {
await prepare(tester);

final findSearchButton = find.descendant(
of: find.byType(ZulipAppBar),
matching: find.byIcon(ZulipIcons.search));

check(find.descendant(
of: find.byType(ZulipAppBar),
matching: find.text('Inbox'))).findsOne();
check(findSearchButton).findsOne();

await tester.tap(find.byIcon(ZulipIcons.hash_italic));
await tester.pump();
check(find.descendant(
of: find.byType(ZulipAppBar),
matching: find.text('Channels'))).findsOne();
check(findSearchButton).findsNothing();

await tester.tap(find.byIcon(ZulipIcons.two_person));
await tester.pump();
check(find.descendant(
of: find.byType(ZulipAppBar),
matching: find.text('Direct messages'))).findsOne();
check(findSearchButton).findsNothing();
});

testWidgets('combined feed', (tester) async {
Expand Down
28 changes: 28 additions & 0 deletions test/widgets/inbox_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,23 @@ import 'package:flutter_checks/flutter_checks.dart';
import 'package:flutter_test/flutter_test.dart';
import 'package:zulip/api/model/events.dart';
import 'package:zulip/api/model/model.dart';
import 'package:zulip/model/narrow.dart';
import 'package:zulip/model/store.dart';
import 'package:zulip/widgets/app_bar.dart';
import 'package:zulip/widgets/color.dart';
import 'package:zulip/widgets/home.dart';
import 'package:zulip/widgets/icons.dart';
import 'package:zulip/widgets/channel_colors.dart';
import 'package:zulip/widgets/message_list.dart';
import 'package:zulip/widgets/page.dart';
import 'package:zulip/widgets/unread_count_badge.dart';

import '../example_data.dart' as eg;
import '../flutter_checks.dart';
import '../model/binding.dart';
import '../model/test_store.dart';
import '../test_navigation.dart';
import 'checks.dart';
import 'test_app.dart';

/// Repeatedly drags `view` by `moveStep` until `finder` is invisible.
Expand Down Expand Up @@ -649,5 +655,27 @@ void main() {
// reappear because you unmuted a conversation.)
});
});

testWidgets('tapping search button navigates to search page', (tester) async {
final pushedRoutes = <Route<dynamic>>[];
final testNavObserver = TestNavigatorObserver()
..onPushed = (route, prevRoute) => pushedRoutes.add(route);

await setupPage(tester,
unreadMessages: [],
navigatorObserver: testNavObserver);

assert(pushedRoutes.length == 1);
pushedRoutes.clear();

await tester.tap(find.descendant(
of: find.byType(ZulipAppBar),
matching: find.byIcon(ZulipIcons.search)));
await tester.pump();

check(pushedRoutes).single.isA<WidgetRoute>().page
.isA<MessageListPage>()
.initNarrow.equals(KeywordSearchNarrow(''));
});
});
}
29 changes: 29 additions & 0 deletions test/widgets/message_list_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,35 @@ void main() {

check(find.text('DMs with Muted user, User 2, Muted user')).findsOne();
});

testWidgets('search button on combined feed navigates to search page', (tester) async {
Copy link
Member

Choose a reason for hiding this comment

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

As I requested above (#1864 (comment)), let's also have a test that looks at a message list on some other narrow and checks that there isn't a search button there.

(But, as discussed in that subthread, not the whole suite of all the other narrow types.)

final pushedRoutes = <Route<dynamic>>[];
final testNavObserver = TestNavigatorObserver()
..onPushed = (route, prevRoute) => pushedRoutes.add(route);

await setupMessageListPage(tester,
narrow: const CombinedFeedNarrow(),
messages: [],
navObservers: [testNavObserver]);

final searchButtonFinder = find.descendant(
of: find.byType(ZulipAppBar),
matching: find.byIcon(ZulipIcons.search));
check(searchButtonFinder).findsOne();

pushedRoutes.clear();
Copy link
Member

Choose a reason for hiding this comment

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

nit: move this to right after the step that pushed these routes (and then join it in the same stanza, with no separating blank line)


connection.prepare(json: eg.newestGetMessagesResult(
foundOldest: true, messages: []).toJson());

await tester.tap(searchButtonFinder);
Comment on lines +380 to +387
Copy link
Member

Choose a reason for hiding this comment

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

This can be simplified a bit: the check(searchButtonFinder).findsOne() is redundant with the fact that tester.tap on the same finder succeeds immediately after it.

await tester.pump();

check(pushedRoutes).single.isA<WidgetRoute>().page
.isA<MessageListPage>()
.initNarrow.equals(KeywordSearchNarrow(''));
await tester.pump(Duration.zero);
Copy link
Member

Choose a reason for hiding this comment

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

Interesting — what's this final pump about? What happens if it's removed?

});
});

group('no-messages placeholder', () {
Expand Down