home: Show starred-message count in main menu, subject to user setting #1999

chrisbobbe · 2025-11-20T00:58:01Z

Stacked atop #1998.

"Subject to user setting" refers to the "toggle starred messages counter" setting: https://zulip.com/help/star-a-message#toggle-starred-messages-counter

Fixes-partly: #1088

Before this (but after #1998)	After

chrisbobbe · 2025-11-20T01:00:06Z

cc @alya

alya · 2025-11-20T04:57:41Z

The screenshots look good to me! We should remember to note that the setting affects the mobile app in https://zulip.com/help/star-a-message#toggle-starred-messages-counter.

rajveermalviya

Thanks @chrisbobbe! LGTM and test great, moving over to Greg's review.

gnprice

Thanks! Here's comments on the first 7/8 commits, including the new data structure:
a3d7fd8 unreads [nfc]: Factor out countInAllDms, for public use and as a helper
45bfcb0 unreads: Exclude muted DM conversations in combined-feed and all-dm counts
248cab2 home: Show unread counts in main menu
e8f51b2 home: Tweak main-menu buttons to follow Figma
247a3d4 api: Add UserSettings.starredMessageCounts
a22b49f api: Add starredMessages to initial snapshot
34f9a0b message: Add MessageStore.starredMessages

and a brief look at the last commit with the UI changes:
1e74454 home: Show starred-message count in main menu, subject to user setting

gnprice · 2025-11-25T01:16:41Z

lib/model/message.dart

    };
-
    if (isAdd && (event as UpdateMessageFlagsAddEvent).all) {


nit: these stanzas don't become any more closely related with this change; in fact they become more separate, because this adds another stanza below which consumes isAdd separately

gnprice · 2025-11-25T01:22:59Z

lib/model/message.dart

+  /// All starred messages, as message IDs.
+  Set<int> get starredMessages;


Hmm, I see.

I'm trying to think if there are other cases where this should get updated. I guess maybe not:

We can learn of messages through handleMessageEvent. But maybe those can't ever be starred.

We can learn of messages through reconcileMessages. But maybe if those are starred then they should already be in this set.

I think it'd be good to make that reasoning explicit, though, in those methods. Probably including asserts to verify those expectations.

Yes, I think both of those "maybe"s are true, modulo the fetch/event race for reconcileMessages. I'll add some comments, and a TODO(log) for the handleMessageEvent case. (Not for reconcileMessages, though, because of the fetch/event race.)

I think asserts wouldn't be right since it's about our expectations of the server rather than our own code; does that sound right?

Hmm yeah — agreed re these being not actually right for asserts.

gnprice · 2025-11-25T01:25:32Z

lib/model/store.dart

+        if (_messages.handleDeleteMessageEvent(event)) {
+          notifyListeners();
+        }
        unreads.handleDeleteMessageEvent(event);


It makes me a bit nervous to be calling notifyListeners at a point where the event has been only partially applied — the store is in a somewhat inconsistent state.

Instead let's delay the notifyListeners call to the end.

gnprice · 2025-11-25T01:34:01Z

test/model/message_test.dart

+      await prepareMessages([message]);
+      check(store).starredMessages.single.equals(message.id);
+      await store.handleEvent(eg.deleteMessageEvent([message]));
+      checkNotifiedOnce();


This line is a bit confusing — it checks that the list view-model notified its listeners, but not whether the store did.

I think the key case here would actually be with prepareMessages not knowing about the message. That exercises the situation where the app hasn't gone and fetched a message list that had the starred message, and only knows about it through the list in the initial snapshot.

Aha could this have been what you were thinking of with #2020 (comment) ?

I think that was actually this, about maintaining topic lists per channel: #1951 (comment)

(Happened to run across that for unrelated reasons just this afternoon, and made that connection.)

gnprice · 2025-11-25T01:36:51Z

test/model/message_test.dart

+        void checkPerAccountStoreNotified({required int count}) {
+          check(perAccountStoreNotifiedCount).equals(count);
+          notifiedCount = 0;


Suggested change

void checkPerAccountStoreNotified({required int count}) {

check(perAccountStoreNotifiedCount).equals(count);

notifiedCount = 0;

void checkPerAccountStoreNotified({required int count}) {

check(perAccountStoreNotifiedCount).equals(count);

perAccountStoreNotifiedCount = 0;

right?

gnprice · 2025-11-25T01:38:51Z

test/model/message_test.dart

+        await prepareMessages([message1, message2]);
+        check(store).starredMessages.isEmpty();
+        await store.handleEvent(
+          mkAddEvent(MessageFlag.starred, [message1.id, message2.id]));


Suggested change

await prepareMessages([message1, message2]);

check(store).starredMessages.isEmpty();

await store.handleEvent(

mkAddEvent(MessageFlag.starred, [message1.id, message2.id]));

await prepareMessages([message2]);

check(store).starredMessages.isEmpty();

await store.handleEvent(

mkAddEvent(MessageFlag.starred, [message1.id, message2.id]));

That way we check that it works both with messages that the message store knows about, and those that it doesn't.

(Similarly for removing the flag, below.)

gnprice · 2025-11-25T01:40:54Z

lib/widgets/unread_count_badge.dart

-class UnreadCountBadge extends StatelessWidget {
-  const UnreadCountBadge({
+/// See [CounterStyle] and [CounterKind] for the possible variants.
+class Counter extends StatelessWidget {


Maybe CounterBadge?

The name "counter" feels awfully generic — it doesn't sound like necessarily even a widget, more like some kind of data structure that keeps count of things.

chrisbobbe · 2025-12-22T22:26:51Z

Thanks for the review! Revision pushed.

See in particular this new commit:

9380ac7 msglist: In "Starred messages", show DMs even if all recipients muted

which removes the message filtering in the starred-messages view (i.e. filtering out DMs in conversations where everyone is muted). The starred-messages data structure (initialized from InitialSnapshot.starredMessages) is just a list of message IDs, and we don't know which of those are in muted DM conversations. By removing this filtering step, which relies on fetched message data, it becomes possible to show a starred-message count that's always correct (i.e. matches what you'll see in the starred-messages message list). Also, it seems acceptable to remove this filtering step because it's not possible for other users to spam your starred-messages view; starring is something only the self-user does for themself.

I think in my ideal version of this code, I would only have to change the "is-message-visible" logic; I wouldn't have to also notice that some MutedUsersEvent code had to change too. I started exploring some refactors, but it kind of turned into a rabbit hole—Greg, we could talk about that on a call if you'd like?—but I sent #2051 which I noticed along the way.

gnprice

Thanks for the revision! Comments below.

As before, I've read all but the last commit, and part of that commit:
8af5930 home: Show starred-message count in main menu, subject to user setting

gnprice · 2025-12-27T01:53:35Z

test/widgets/message_list_test.dart

+            starredMessages: messages
+              .where((message) => message.flags.contains(MessageFlag.starred))
+              .map((message) => message.id).toList(),


Annoying to have to do these. Ah well.

gnprice · 2025-12-27T01:55:59Z

test/widgets/checks.dart

 }

-extension UnreadCountBadgeChecks on Subject<UnreadCountBadge> {
+extension UnreadCountBadgeChecks on Subject<CounterBadge> {


nit: update name

gnprice · 2025-12-27T01:57:58Z

lib/model/message_list.dart

+  ///   - Message-list UI that mitigates spam/harrassment
+  ///     by obscuring messages from muted senders, with a "reveal" button


I don't understand this as a "see also" — what would I go see?

(separately nit: s/rr/r/)

Ah—I meant "message-list UI code", but stopped short of naming that code with identifiers or filenames, to maintain the separation of concerns between lib/model and lib/widgets. Maybe that was overzealous? I'll plan to name the code explicitly in my next revision. 🙂

Ah, I see. Yeah, we keep the dependency arrows pointing one way from widgets to model (with a couple of exceptions which we'd ideally go refactor away); but a comment like this is basically by nature pointing backwards along those same arrows, explaining the context and motivation of this code by reference to the code that depends on it. So it's fine for such a comment in e.g. model code to refer to widgets code.

I'm still not sure a "see also" is the clearest format to express this point. Maybe a short paragraph of plain prose instead? (Which might or might not point to a specific widget by name.) But I'll see how it looks in the next revision.

gnprice · 2025-12-27T02:17:02Z

lib/model/message_list.dart

      case StarredMessagesNarrow():
+        return MutedUsersVisibilityEffect.none;


I think in my ideal version of this code, I would only have to change the "is-message-visible" logic; I wouldn't have to also notice that some MutedUsersEvent code had to change too. I started exploring some refactors, but it kind of turned into a rabbit hole—Greg, we could talk about that on a call if you'd like?

Hmm, I see.

Happy to talk about this on a call when we're next both online. My initial thought is that I think of this method _mutedUsersEventCanAffectVisibility as part of the "is-message-visible" logic, along with the three neighboring methods _messageVisible, _allMessagesVisible, _canAffectVisibility. Together these four methods amount to _messageVisible itself and then three different summaries of it.

The fact that this belongs logically with the other "is-message-visible" logic is also the reason this method is located here next to those other three, rather than by its one caller handleMutedUsersEvent.

There are doubtless things we can do to help make it clearer that updates to _messageVisible need to be paralleled in all three of these. Some comments for sure. And perhaps it'd also be helpful to group them together more explicitly in the code organization? They could go become a mixin of their own; I think the only info they take from the MessageListView is the store and the narrow, so the mixin could just define virtual getters for those.

Makes sense, yes.

There was another factor in my rabbit-hole feeling that probably wasn't obvious, and I could've helpfully mentioned 🙂: I wanted to move toward having one piece of code that controls both this (message-list message visibility) and whether messages are included in the unread counts provided by Unreads, because I think the two should always agree for a given unread message. That seems feasible in principle: in both places, the filtering is completely determined by the UI context (as a Narrow object), a relevant conversation, and whether that conversation is muted. But MessageListView._messageVisible acts on MessageBase objects, and Unreads doesn't, and I figured the refactor would need more attention to performance than we can spare right now.

gnprice · 2025-12-27T02:21:10Z

lib/model/message_list.dart

      case StarredMessagesNarrow():
+        // Include messages even if muted in some way.
+        // Other users can't spam the starred-messages view by starring;
+        // the starred state is read/write by the self-user only.


Reasonable. What does the web app do? If it's hiding (some of) these messages, then it'd be good to raise a thread for changing that, both for the reasons here and for the sake of keeping web and mobile aligned.

Sure; yeah, I noticed web is filtering out some messages and started https://chat.zulip.org/#narrow/channel/9-issues/topic/Show.20all.20starred.20messages.20in.20the.20.22Starred.20messages.22.20view/with/2342109 about it.

gnprice · 2025-12-27T02:23:33Z

test/model/message_test.dart

+        check(store).starredMessages.deepEquals([message1.id, message2.id]);
+        await store.handleEvent(
+          mkRemoveEvent(MessageFlag.starred, [message1, message2]));
+        checkPerAccountStoreNotifiedOnce();


These checks aren't totally satisfying because they don't verify whether the MessageListView notified its listeners or not.

I'll push an added commit on top which reorganizes these checks a bit and does that:
a8c245c message test: Strengthen checks on when notifyListeners called

gnprice · 2025-12-27T02:25:46Z

lib/widgets/home.dart

-    return UnreadCountBadge(
-      style: UnreadCountBadgeStyle.mainMenu,
+    return CounterBadge(
+      kind: CounterBadgeKind.unread,
+      style: CounterBadgeStyle.mainMenu,


Would you split this last commit:
8af5930 home: Show starred-message count in main menu, subject to user setting

into an NFC prep commit that renames the widget and adds the required kind parameter, and then a commit that makes the substantive change? The refactor changes a lot of lines (like here), and I think the substantive change would become easier to read if separated from that.

chrisbobbe · 2026-01-03T03:47:33Z

Thanks!! Revision pushed.

gnprice

Thanks! All looks good now outside that main pair of commits; comment below.

gnprice · 2026-01-05T20:34:28Z

lib/widgets/counter_badge.dart

-            color: textColor,
-          ).merge(weightVariableTextStyle(context, wght: wght)),
-          count.toString())));
+    Widget result = Padding(


Ah, the split I was really hoping for in #1999 (comment) was to keep the commit with all the boring changes really purely boring, but I guess my mention of the kind parameter was confusing. What I had in mind was:

First commit renames and moves this widget. Also adds the required kind parameter, because there are a lot of call sites that need to start passing it, but the parameter doesn't do anything yet — the enum has just one value.

I guess this could also be two boring commits instead of one: the rename/move, and the new required parameter.

Second (or third) commit adds the .quantity value and its implementation, as well as its one initial call site.

I think that would help by isolating the substantive changes separate from the many boring ones. As is:
75041c3 counter_badge [nfc]: Support "quantity" kind; rename from UnreadCountBadge
ece6e29 home: Show starred-message count in main menu, subject to user setting

the latter commit is just adding the call site, but most of the interesting changes are still in the earlier commit which also has all the boring mechanical changes.

Makes sense; thanks for explaining!

chrisbobbe · 2026-01-05T23:17:42Z

Thanks! Revision pushed; PTAL.

gnprice

Thanks, much clearer! Generally this looks good; one real comment below.

gnprice · 2026-01-06T00:59:17Z

lib/widgets/counter_badge.dart

+    required this.channelIdForBackground,
+  }) : assert(!(kind == CounterBadgeKind.quantity && channelIdForBackground != null));


Hmm, seems awkward for a caller wanting a "quantity" badge to be required to pass a channel ID when it's always irrelevant and required to be null.

OTOH I think I see why you didn't just make it optional — it's required because we don't want an "unread" badge to accidentally forget to pass it. The reason it's nullable is because a given caller might have null-vs-not depend on the data.

I think this situation is one that's solved with separate constructors. We can have CounterBadge.unread with the parameter required, and CounterBadge.quantity with no such parameter. Then kind isn't a parameter of either, and instead each constructor sets it to a different constant. (All this is a common pattern in Flutter upstream.)

I'm hoping the channelIdForBackground param will disappear very soon. 🙂 The current Figma doesn't actually have any channel-colorized unread-count badges, and I'll be matching that on the path to showing channel folders.

Ah OK, not worth refactoring the API to support that, then. Just leave a quick comment saying this API is awkward but we're planning to remove it soon — can link to your comment here.

gnprice · 2026-01-06T01:01:13Z

lib/widgets/counter_badge.dart

+    final double wght = switch ((style, kind)) {
+      (CounterBadgeStyle.mainMenu, CounterBadgeKind.unread  ) => 600,
+      (CounterBadgeStyle.mainMenu, CounterBadgeKind.quantity) => 500,
+      (CounterBadgeStyle.other,    CounterBadgeKind.unread  ) => 500,
+      (CounterBadgeStyle.other,    CounterBadgeKind.quantity) => 500,


Does this last case have examples in Figma? If not, we could assert against it instead. (Or with separate constructors, not even take style as a parameter in the "quantity" constructor.)

Ah OK, I guess the TODO(design) comments on the inbox answer that question 🙂

In this test, it's not important that specifically the starred flag is used; the hasAlertWord flag works just as well. It would be a bit annoying to refactor this around the starred flag, when we add a data structure for starred messages, coming up.

UpdateMessageEvent has a `flags` field, but the API doc is pretty clear that it's just meant to communicate changes in the mentioned and alert-words flags: https://zulip.com/api/get-events#update_message The separate event, UpdateMessageFlagsEvent, is what updates the starred flag.

See reasoning in added dartdoc and implementation comments in _messageVisible.

As the dartdoc says, this widget currently supports the Figma's "kind=unread" variant but not the "kind=quantity" variant. To prepare to support the latter, drop "unread" from the widget's name.

Fixes-partly: zulip#1088

Generally we've taken care to ensure that the highest-frequency events, including several events handled by MessageStore, don't cause the overall PerAccountStore to notify its listeners. The main effect of this test change is to verify that. We already have helpers checkNotifiedOnce and checkNotNotified, which numerous tests in this file use to confirm that MessageListView either did or didn't notify listeners in a given situation. With this change, those also verify that the PerAccountStore didn't notify listeners. A secondary effect is that in the handful of spots (recently added) where we do now expect PerAccountStore to notify listeners, we also check whether MessageListView did so, just like we do in other tests.

chrisbobbe · 2026-01-06T01:36:38Z

Thanks! Revision pushed.

gnprice · 2026-01-06T01:56:35Z

Thanks! Looks good; merging.

chrisbobbe requested a review from rajveermalviya November 20, 2025 00:58

chrisbobbe assigned rajveermalviya Nov 20, 2025

chrisbobbe added the maintainer review PR ready for review by Zulip maintainers label Nov 20, 2025

chrisbobbe added the product review Added by maintainers when a PR needs product review label Nov 20, 2025

chrisbobbe force-pushed the pr-starred-message-count-main-menu branch from 5d32321 to 1e74454 Compare November 20, 2025 22:17

rajveermalviya approved these changes Nov 21, 2025

View reviewed changes

rajveermalviya 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 Nov 21, 2025

rajveermalviya assigned gnprice and unassigned rajveermalviya Nov 21, 2025

rajveermalviya requested a review from gnprice November 21, 2025 15:57

gnprice reviewed Nov 25, 2025

View reviewed changes

chrisbobbe force-pushed the pr-starred-message-count-main-menu branch 2 times, most recently from 3b7df61 to 8af5930 Compare December 22, 2025 22:15

gnprice reviewed Dec 27, 2025

View reviewed changes

gnprice force-pushed the pr-starred-message-count-main-menu branch from 8af5930 to a8c245c Compare December 27, 2025 02:29

chrisbobbe mentioned this pull request Dec 30, 2025

Show archive icon for archived channels and add test (fixes #1992) #2037

Closed

chrisbobbe force-pushed the pr-starred-message-count-main-menu branch from a8c245c to f1c78db Compare January 3, 2026 03:45

gnprice reviewed Jan 5, 2026

View reviewed changes

chrisbobbe force-pushed the pr-starred-message-count-main-menu branch from f1c78db to 696e804 Compare January 5, 2026 23:17

gnprice reviewed Jan 6, 2026

View reviewed changes

chrisbobbe added 4 commits January 5, 2026 17:25

api: Add UserSettings.starredMessageCounts

758f93a

api: Add starredMessages to initial snapshot

904ed26

chrisbobbe and others added 7 commits January 5, 2026 17:25

message: Add MessageStore.starredMessages

3af14f2

msglist [nfc]: Expand/clarify _messageVisible dartdoc and add a comment

7b47714

msglist: In "Starred messages", show DMs even if all recipients muted

3556d6b

See reasoning in added dartdoc and implementation comments in _messageVisible.

counter_badge [nfc]: Rename UnreadCountBadge to CounterBadge

5361c1e

As the dartdoc says, this widget currently supports the Figma's "kind=unread" variant but not the "kind=quantity" variant. To prepare to support the latter, drop "unread" from the widget's name.

counter_badge [nfc]: Add kind param, with just one value for now

0537e32

home: Show starred-message count in main menu, subject to user setting

34d43be

Fixes-partly: zulip#1088

chrisbobbe force-pushed the pr-starred-message-count-main-menu branch from 696e804 to 55090e0 Compare January 6, 2026 01:36

gnprice merged commit 55090e0 into zulip:main Jan 6, 2026
1 check passed

		/// All starred messages, as message IDs.
		Set<int> get starredMessages;

		/// - Message-list UI that mitigates spam/harrassment
		/// by obscuring messages from muted senders, with a "reveal" button

		case StarredMessagesNarrow():
		return MutedUsersVisibilityEffect.none;

		required this.channelIdForBackground,
		}) : assert(!(kind == CounterBadgeKind.quantity && channelIdForBackground != null));

home: Show starred-message count in main menu, subject to user setting #1999

home: Show starred-message count in main menu, subject to user setting #1999

Uh oh!

Conversation

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

Uh oh!

Uh oh!

chrisbobbe commented Nov 20, 2025

Uh oh!

alya commented Nov 20, 2025

Uh oh!

rajveermalviya left a comment

Choose a reason for hiding this comment

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Dec 22, 2025

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Jan 3, 2026

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

chrisbobbe commented Nov 20, 2025 •

edited

Loading

gnprice Dec 17, 2025 •

edited

Loading

gnprice Dec 27, 2025 •

edited

Loading

gnprice Dec 27, 2025 •

edited

Loading