action: Add dialog to markNarrowAsRead when outside of conversation view #2006

chungwwei · 2025-11-25T18:20:28Z

Still picture

narrow	tap when long press

Screencasts

combined feed narrow

combined_feed_sc.mp4

channel narrow

channel_sc.mp4

mentions narrow

mention_sc.mp4

chrisbobbe

Thanks! Comments below.

assets/l10n/app_en.arb

test/widgets/message_list_test.dart

test/widgets/actions_test.dart

lib/widgets/actions.dart

chungwwei · 2025-12-02T21:33:17Z

@chrisbobbe Thanks for reviewing!

Squash the 4 commits into one.

KeywordSearchNarrow was the other narrow. Was not looking at the code, but when i tested in the app, the Mark all messages as read button didn't appear in the search view when searched. Would you be okay with me filing a follow up issue for KeywordSearchNarrow?

chrisbobbe · 2025-12-02T22:45:22Z

KeywordSearchNarrow was the other narrow.

That's not accurate. We should show the dialog when the narrow is not a conversation narrow. A "conversation narrow" is a topic narrow or a DM narrow; see e.g. the top of the Zulip Help Center "Reading conversations" article. These details are provided for you in the issue description and discussions linked from there.

In the zulip-flutter codebase, there are multiple kinds of Narrow other than TopicNarrow and DmNarrow. CombinedFeedNarrow and KeywordSearchNarrow are just two of those.

chrisbobbe

Thanks! Comments below.

Since the issue is about the mark-as-read button at the bottom of the message list, I'd like to have some tests in the 'MarkAsReadWidget' group in test/widgets/message_list_test.dart that explicitly target the behavior. (I see you've already made some changes to existing tests there, basically as needed to make them pass.)

How about a new group('confirmation dialog', () {}); inside the 'MarkAsReadWidget' group, with tests like these:

test('CombinedFeedNarrow: show dialog', () {});
test('MentionsNarrow: show dialog', () {});
test('ChannelNarrow: show dialog', () {});
test('DmNarrow: do not show dialog', () {});
test('TopicNarrow: do not show dialog', () {});

For the 'do not show dialog' tests, you should be able to use the checkNoDialog helper in test/widgets/dialog_checks.dart.

assets/l10n/app_en.arb

lib/widgets/actions.dart

chrisbobbe · 2025-12-10T23:52:35Z

test/widgets/message_list_test.dart

+      return checkSuggestedActionDialog(tester,
+        expectedTitle: zulipLocalizations.markAllAsReadConfirmationDialogTitle,
+        expectedMessage: zulipLocalizations.markAllAsReadConfirmationDialogMessage(unreadCount),
+        expectDestructiveActionButton: true,


Why expectDestructiveActionButton: true?

Set to false. Read can be reverted.

test/widgets/actions_test.dart

lib/widgets/actions.dart

chrisbobbe · 2025-12-11T00:48:32Z

(I've also just filed #2030 and #2031 for future work, but those are out of scope for this PR; I don't expect them to require changes here. 🙂)

chungwwei · 2025-12-12T15:01:08Z

Thanks @chrisbobbe! it's ready for another review.

chrisbobbe

Thanks! Small comments below.

assets/l10n/app_en.arb

lib/widgets/actions.dart

chrisbobbe · 2025-12-15T21:56:46Z

test/widgets/actions_test.dart

+      Future<void> confirmToMarkNarrowAsRead({required WidgetTester tester,
+        required BuildContext context, required Narrow narrow, required int unreadCount,
+      }) async {
+        final future = ZulipAction.markNarrowAsRead(context, narrow);
+        await tester.pump(); // confirmation dialog appears
+        final (confirmButton, _) = checkConfirmDialog(tester, unreadCount);
+        await tester.tap(find.byWidget(confirmButton));
+        await tester.pump(Duration.zero); // wait through API request
+        await future;
+      }


Bump on #2006 (comment) .

…Ah, I think maybe my meaning wasn't clear: I want to keep the checkConfirmDialog helper function, but not this confirmToMarkNarrowAsRead helper. Can you remove it please?

test/widgets/actions_test.dart

test/widgets/message_list_test.dart

chungwwei · 2025-12-16T22:49:31Z

Thanks @chrisbobbe! It's ready for another look.

chrisbobbe · 2025-12-17T00:16:47Z

Thanks! LGTM; marking for Greg's review.

One small request that would make reviewing just a bit easier for us 🙂:

If it's not my first review, I'll start by looking at each comment from my previous review, to check that it's resolved.
If you've used GitHub's "resolve comment" feature on those comments, then I have to click "Show resolved" on each one, which makes this part take longer than it would otherwise:
On the other hand, it's helpful for you to have a way to track which comments you've responded to 🙂. The 👍 reaction works well for that.
So, please feel free to add the 👍 reaction to my review comments, to help you track your progress, but please don't use GitHub's "resolve comment" feature.

chrisbobbe · 2025-12-17T00:18:56Z

Ah and one comment I've been meaning to make, and almost forgot again: please squash this revision's three commits—

169ad4c actions: Add markNarrowAsRead dialog outside conversation views
07a56a7 msglist test: Verify whether the MarkNarrowAsRead dialog appears
49caa61 actions: Update dartdoc for markNarrowAsRead func

—into just one commit. The first commit is incomplete without the tests and dartdoc updates in the latter two commits.

A confirm ActionDialog is added to prevent accidentally marking many messages as read outside conversation views. Also, adjust existing MarkAsReadWidget tests and MarkChannelAsReadButton tests to account for the added confirmation dialog. Fixes: zulip#1858.

gnprice

Thanks @chungwwei, and thanks @chrisbobbe for the previous reviews!

Comments below. I haven't yet fully read the tests, but I've read the other changes.

One commit-message nit:

Fixes: #1858.

This should read "Fixes #1858.", no colon, at the beginning. (Also acceptable would be "Fixes: #1858", no period, at the end.) See examples: take a look at our Git history, following our guide.

gnprice · 2025-12-17T01:47:25Z

lib/widgets/actions.dart

+        actionButtonText: zulipLocalizations.markAllAsReadConfirmationDialogConfirmButton);
+
+      if (await didConfirm.result != true) return;


nit: these are tightly related; join visually as one stanza:

Suggested change

actionButtonText: zulipLocalizations.markAllAsReadConfirmationDialogConfirmButton);

if (await didConfirm.result != true) return;

actionButtonText: zulipLocalizations.markAllAsReadConfirmationDialogConfirmButton);

if (await didConfirm.result != true) return;

gnprice · 2025-12-17T01:47:36Z

lib/widgets/actions.dart

+    };
+
+    if (shouldShowConfirmationDialog) {


nit: similarly:

Suggested change

};

if (shouldShowConfirmationDialog) {

};

if (shouldShowConfirmationDialog) {

gnprice · 2025-12-17T01:50:08Z

lib/widgets/actions.dart

+    final shouldShowConfirmationDialog = switch (narrow) {
+      CombinedFeedNarrow()


Let's include a comment linking to our thinking for what sort of narrows should get this behavior:
https://chat.zulip.org/#narrow/channel/48-mobile/topic/mark.20all.20messages.20as.20read/near/2261768

That way, when we add more types of narrows in the future, it will help us make the decision of whether the new narrows should get this behavior or not.

gnprice · 2025-12-17T01:56:23Z

lib/widgets/actions.dart

+      final unreadCount = store.unreads.countInNarrow(narrow);
+      final didConfirm = showSuggestedActionDialog(context: context,
+        title: zulipLocalizations.markAllAsReadConfirmationDialogTitle,
+        message: zulipLocalizations.markAllAsReadConfirmationDialogMessage(unreadCount),


The issue calls for some nontrivial nuances in how this text interacts with the number of known unread messages. Would you try implementing those nuances?

If part of the spec isn't included in what the PR implements, that always needs to be highlighted in the PR. Then we can decide whether to leave it for later, and can make sure to track the follow-up task.

gnprice added the maintainer review PR ready for review by Zulip maintainers label Nov 25, 2025

chrisbobbe reviewed Dec 2, 2025

View reviewed changes

assets/l10n/app_en.arb Outdated Show resolved Hide resolved

test/widgets/message_list_test.dart Outdated Show resolved Hide resolved

test/widgets/actions_test.dart Outdated Show resolved Hide resolved

lib/widgets/actions.dart Outdated Show resolved Hide resolved

chungwwei force-pushed the read-confirm-dialog branch from 1de2a51 to 970aee6 Compare December 2, 2025 21:19

chungwwei requested a review from chrisbobbe December 2, 2025 21:33

chungwwei force-pushed the read-confirm-dialog branch 7 times, most recently from 1613f50 to f0bf130 Compare December 3, 2025 22:28

chrisbobbe mentioned this pull request Dec 10, 2025

msglist: Make mark-as-read button listen to the Unreads model #2020

Open

chrisbobbe requested changes Dec 11, 2025

View reviewed changes

chungwwei force-pushed the read-confirm-dialog branch 8 times, most recently from ef8f94f to a2042c6 Compare December 12, 2025 14:34

chungwwei requested a review from chrisbobbe December 12, 2025 15:01

chrisbobbe reviewed Dec 16, 2025

View reviewed changes

chungwwei force-pushed the read-confirm-dialog branch 3 times, most recently from fb8131c to 9bdce8a Compare December 16, 2025 21:55

chungwwei force-pushed the read-confirm-dialog branch 2 times, most recently from f3a1475 to 49caa61 Compare December 16, 2025 22:27

chungwwei requested a review from chrisbobbe December 16, 2025 22:37

chungwwei changed the title ~~action: Add dialog to markNarrowAsRead for CombinedFeedNarrow view~~ action: Add dialog to markNarrowAsRead when outside of conversation view Dec 16, 2025

chrisbobbe requested a review from gnprice December 17, 2025 00:16

chrisbobbe assigned gnprice Dec 17, 2025

chrisbobbe added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Dec 17, 2025

chungwwei force-pushed the read-confirm-dialog branch from 49caa61 to d643776 Compare December 17, 2025 00:52

gnprice reviewed Dec 17, 2025

View reviewed changes

		actionButtonText: zulipLocalizations.markAllAsReadConfirmationDialogConfirmButton);

		if (await didConfirm.result != true) return;

		final shouldShowConfirmationDialog = switch (narrow) {
		CombinedFeedNarrow()

action: Add dialog to markNarrowAsRead when outside of conversation view #2006

Are you sure you want to change the base?

action: Add dialog to markNarrowAsRead when outside of conversation view #2006

Uh oh!

Conversation

chungwwei commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

combined feed narrow

channel narrow

mentions narrow

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chungwwei commented Dec 2, 2025

Uh oh!

chrisbobbe commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chrisbobbe Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

chungwwei Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chrisbobbe commented Dec 11, 2025

Uh oh!

chungwwei commented Dec 12, 2025

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

chrisbobbe Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chungwwei commented Dec 16, 2025

Uh oh!

chrisbobbe commented Dec 17, 2025

Uh oh!

chrisbobbe commented Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

gnprice Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

gnprice Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

gnprice Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

gnprice Dec 17, 2025

chungwwei commented Nov 25, 2025 •

edited

Loading

chrisbobbe commented Dec 2, 2025 •

edited

Loading

chrisbobbe commented Dec 17, 2025 •

edited

Loading