Keep track of the known topics in store #1951

sm-sayedi · 2025-10-28T21:24:22Z

#1508 but rebased on top of main with the review of @gnprice addressed.

(Thanks @PIG208 for all of your previous work in #1508)

Fixes: #1499

chrisbobbe

Thanks! Glad to be maintaining a data structure for this. 🙂

Comments below, from reading the implementation in the first commit (I haven't yet read the tests):

d6b2242 channel: Keep track of channel topics, and keep up-to-date with events

chrisbobbe · 2025-11-04T03:57:05Z

lib/model/channel.dart

 import 'store.dart';
 import 'user.dart';

+final _apiGetChannelTopics = getStreamTopics;  // similar to _apiSendMessage in lib/model/message.dart


nit: too-long line; put comment on line above

chrisbobbe · 2025-11-04T04:08:11Z

lib/model/channel.dart

+  /// can be retrieved with [getChannelTopics].
+  Future<void> fetchTopics(int channelId);
+
+  /// Pairs of the known topics and its latest message ID, in the given channel.


Suggested change

/// Pairs of the known topics and its latest message ID, in the given channel.

/// The topics in the given channel, along with their latest message ID.

chrisbobbe · 2025-11-04T04:09:01Z

lib/model/channel.dart

+
+  /// Pairs of the known topics and its latest message ID, in the given channel.
+  ///
+  /// Returns null if the data has never been fetched yet.


nit:

Suggested change

/// Returns null if the data has never been fetched yet.

/// Returns null if the data has not been fetched yet.

chrisbobbe · 2025-11-04T04:09:45Z

lib/model/channel.dart

+  /// The result is guaranteed to be sorted by [GetStreamTopicsEntry.maxId]
+  /// descending, and the topics are guaranteed to be distinct.


nit, omit needless words:

Suggested change

/// The result is guaranteed to be sorted by [GetStreamTopicsEntry.maxId]

/// descending, and the topics are guaranteed to be distinct.

/// The result is sorted by [GetStreamTopicsEntry.maxId] descending,

/// and the topics are distinct.

chrisbobbe · 2025-11-04T04:34:54Z

lib/model/channel.dart

+  /// In some cases, the same maxId affected by message moves can be present in
+  /// multiple [GetStreamTopicsEntry] entries. For this reason, the caller
+  /// should not rely on [getChannelTopics] to determine which topic the message
+  /// is in. Instead, refer to [PerAccountStore.messages].
+  /// See [handleUpdateMessageEvent] on how this could happen.


It seems worth highlighting in general that maxId might be…incorrect, basically. (Not just that two topics might have the same maxId.) Maybe something like:

(Also, isn't message deletion another reason maxId could be wrong?)

Suggested change

/// In some cases, the same maxId affected by message moves can be present in

/// multiple [GetStreamTopicsEntry] entries. For this reason, the caller

/// should not rely on [getChannelTopics] to determine which topic the message

/// is in. Instead, refer to [PerAccountStore.messages].

/// See [handleUpdateMessageEvent] on how this could happen.

/// Occasionally, [GetStreamTopicsEntry.maxId] will refer to a message

/// that doesn't exist or is no longer in the topic.

/// This happens when a topic's latest message is moved or deleted

/// and we don't have enough information

/// to replace [GetStreamTopicsEntry.maxId] accurately.

/// (We don't keep a snapshot of all messages.)

/// Use [PerAccountStore.messages] to check a message's topic accurately.

chrisbobbe · 2025-11-04T05:14:42Z

lib/model/channel.dart

+  /// Handle a [MessageEvent], returning whether listeners should be notified.
+  bool handleMessageEvent(MessageEvent event) {
+    if (event.message is! StreamMessage) return false;
+    final StreamMessage(:streamId, :topic) = event.message as StreamMessage;


nit:

Suggested change

final StreamMessage(:streamId, :topic) = event.message as StreamMessage;

final StreamMessage(streamId: channelId, :topic) = event.message as StreamMessage;

chrisbobbe · 2025-11-04T05:18:28Z

lib/model/channel.dart

+    // If this message is already the latest message in the topic because it was
+    // received through fetch in fetch/event race, or it is a message sent even
+    // before the latest message of the fetch, we don't do the update.


I'm getting confused trying to parse this sentence. Instead, how about, inside the if just below this, say something like:

// The event raced with a message fetch.

chrisbobbe · 2025-11-04T05:25:50Z

lib/model/channel.dart

+    // If we don't already know about the list of topics of the channel this
+    // message belongs to, we don't want to proceed and put one entry about the
+    // topic of this message, otherwise [fetchTopics] and the callers of
+    // [getChannelTopics] would assume that the channel only has this one topic
+    // and would never fetch the complete list of topics for that matter.
+    if (latestMessageIdsByTopic == null) return false;


How about:

Suggested change

// If we don't already know about the list of topics of the channel this

// message belongs to, we don't want to proceed and put one entry about the

// topic of this message, otherwise [fetchTopics] and the callers of

// [getChannelTopics] would assume that the channel only has this one topic

// and would never fetch the complete list of topics for that matter.

if (latestMessageIdsByTopic == null) return false;

if (latestMessageIdsByTopic == null) {

// We're not tracking this channel's topics yet.

// We start doing that when [fetchTopics] is called,

// and we fill in all the topics at that time.

return false;

}

chrisbobbe · 2025-11-04T05:39:53Z

lib/model/channel.dart

+    final origLatestMessageIdsByTopics = _latestMessageIdsByChannelTopic[origStreamId];
+    // We only handle the case where all the messages of [origTopic] are
+    // moved to [newTopic]; in that case we can remove [origTopic] safely.
+    // But if only one messsage is moved (`PropagateMode.changeOne`) or a few
+    // messages are moved (`PropagateMode.changeLater`), we cannot do anything
+    // about [origTopic] here as we cannot determine the new `maxId` for it.
+    // (This is the case where there could be multiple channel-topic keys with
+    //  the same `maxId`)
+    if (propagateMode == PropagateMode.changeAll
+        && origLatestMessageIdsByTopics != null) {
+      shouldNotify = origLatestMessageIdsByTopics.remove(origTopic) != null;
+    }


With a switch/case on propagateMode, I think we can be less verbose in the comment, e.g.:

Suggested change

final origLatestMessageIdsByTopics = _latestMessageIdsByChannelTopic[origStreamId];

// We only handle the case where all the messages of [origTopic] are

// moved to [newTopic]; in that case we can remove [origTopic] safely.

// But if only one messsage is moved (`PropagateMode.changeOne`) or a few

// messages are moved (`PropagateMode.changeLater`), we cannot do anything

// about [origTopic] here as we cannot determine the new `maxId` for it.

// (This is the case where there could be multiple channel-topic keys with

// the same `maxId`)

if (propagateMode == PropagateMode.changeAll

&& origLatestMessageIdsByTopics != null) {

shouldNotify = origLatestMessageIdsByTopics.remove(origTopic) != null;

}

final origLatestMessageIdsByTopics = _latestMessageIdsByChannelTopic[origStreamId];

switch (propagateMode) {

case PropagateMode.changeOne:

case PropagateMode.changeLater:

// We can't know the new `maxId` for the original topic.

// Shrug; leave it unchanged. (See dartdoc of [getChannelTopics],

// where we call out this possibility that `maxId` is incorrect.

break;

case PropagateMode.changeAll:

if (origLatestMessageIdsByTopics != null) {

origLatestMessageIdsByTopics.remove(origTopic);

shouldNotify = true;

}

}

chrisbobbe · 2025-11-04T05:54:30Z

lib/model/channel.dart

+    final newLatestMessageIdsByTopics = _latestMessageIdsByChannelTopic[newStreamId];
+    if (newLatestMessageIdsByTopics != null) {
+      final movedMaxId = event.messageIds.max;
+      if (!newLatestMessageIdsByTopics.containsKey(newTopic)
+          || newLatestMessageIdsByTopics[newTopic]! < movedMaxId) {
+        newLatestMessageIdsByTopics[newTopic] = movedMaxId;
+        shouldNotify = true;
+      }
+    }


A bit easier to read, I think:

Suggested change

final newLatestMessageIdsByTopics = _latestMessageIdsByChannelTopic[newStreamId];

if (newLatestMessageIdsByTopics != null) {

final movedMaxId = event.messageIds.max;

if (!newLatestMessageIdsByTopics.containsKey(newTopic)

|| newLatestMessageIdsByTopics[newTopic]! < movedMaxId) {

newLatestMessageIdsByTopics[newTopic] = movedMaxId;

shouldNotify = true;

}

}

final newLatestMessageIdsByTopics = _latestMessageIdsByChannelTopic[newStreamId];

if (newLatestMessageIdsByTopics != null) {

final movedMaxId = event.messageIds.max;

final currentMaxId = newLatestMessageIdsByTopics[newTopic];

if (currentMaxId == null || currentMaxId < movedMaxId) {

newLatestMessageIdsByTopics[newTopic] = movedMaxId;

shouldNotify = true;

}

}

sm-sayedi · 2025-11-04T19:22:05Z

Thanks @chrisbobbe for the review. Pushed new revision.

chrisbobbe

Thanks, and sorry for the delay! Comments below, this time from a full review.

chrisbobbe · 2025-11-08T01:57:46Z

test/widgets/topic_list_test.dart

  });

-  testWidgets('fetch again when navigating away and back', (tester) async {
+  testWidgets("dont't fetch again when navigating away and back", (tester) async {


nit: spelling of "don't"

chrisbobbe · 2025-11-19T00:37:29Z

test/api/route/route_checks.dart

  Subject<Uri> get realmUrl => has((e) => e.realmUrl, 'realmUrl');
 }

+extension GetStreamTopicEntryChecks on Subject<GetStreamTopicsEntry> {


"GetStreamTopicsEntryChecks"

chrisbobbe · 2025-11-19T00:44:45Z

test/model/store_checks.dart

  Subject<Map<int, ZulipStream>> get streams => has((x) => x.streams, 'streams');
  Subject<Map<String, ZulipStream>> get streamsByName => has((x) => x.streamsByName, 'streamsByName');
  Subject<Map<int, Subscription>> get subscriptions => has((x) => x.subscriptions, 'subscriptions');
+  Subject<List<GetStreamTopicsEntry>?> getStreamTopics(int streamId) => has((x) => x.getChannelTopics(streamId), 'getStreamTopics');


getChannelTopics and 'getChannelTopics', to match PerAccountStore.getChannelTopics

chrisbobbe · 2025-11-19T00:50:01Z

lib/model/channel.dart

 import 'user.dart';

+// similar to _apiSendMessage in lib/model/message.dart
+final _apiGetChannelTopics = getStreamTopics;


Could you add a prep commit that renames the existing binding getStreamTopics to getChannelTopics, then consistently use "channel topics" naming instead of "stream topics" naming?

chrisbobbe · 2025-11-19T00:50:55Z

lib/model/channel.dart

+  /// can be retrieved with [getChannelTopics].
+  Future<void> fetchTopics(int channelId);
+
+  /// The topics in the given channel, along with their latest message ID.


Suggested change

/// The topics in the given channel, along with their latest message ID.

/// The topics the user can access, along with their latest message ID,

/// reflecting updates from events that arrived since the data was fetched.

chrisbobbe · 2025-11-19T04:10:18Z

test/widgets/topic_list_test.dart

    connection.prepare(json: GetStreamTopicsResult(
      topics: [eg.getStreamTopicsEntry(name: 'topic B')]).toJson());


If we don't expect to fetch again, we shouldn't prepare a response, right?

chrisbobbe · 2025-11-19T04:13:44Z

test/widgets/topic_list_test.dart

-    ]);
+  testWidgets('show topic action sheet before and after moves', (tester) async {
+    final channel = eg.stream();
+    final message = eg.streamMessage(id: 123, stream: channel, topic: 'foo');


Is 123 chosen because it equals eg.defaultStreamMessageStreamId? We should use eg.defaultStreamMessageStreamId instead, if so, or else pass 123 as streamId to the eg.stream() call.

In this case 123 is just an arbitrary message ID, compared to eg.defaultStreamMessageStreamId which is stream/channel ID. Anyways, changed it to 100 to differentiate from the stream ID.

chrisbobbe · 2025-11-19T04:14:25Z

test/widgets/topic_list_test.dart

+    // After the move, the message with maxId moved away from "foo". The topic
+    // actions that require `someMessageIdInTopic` is no longer available.


nit: "are no longer available"

chrisbobbe · 2025-11-19T04:14:59Z

test/widgets/topic_list_test.dart

+    await tester.tap(find.text('Cancel'));
+    await tester.pump();
+
+    // Topic actions that require `someMessageIdInTopic` is available


nit: "are available"

chrisbobbe · 2025-11-19T04:16:53Z

test/widgets/topic_list_test.dart

+    check(find.descendant(of: topicItemFinder,
+      matching: find.text('foo'))).findsOne();


How about:

check(findInTopicItemAt(0, find.text('foo'))).findsOne();

?

sm-sayedi · 2025-11-25T02:34:57Z

Thanks @chrisbobbe for the review. New changes pushed, PTAL.

gnprice

Thanks to you both!

I see this is still in maintainer review, and want that to continue until it's done. Since I'll be on vacation next week, though (and with the Thanksgiving holiday here starting Thursday), here's some high-level comments from an initial skim.

gnprice · 2025-11-25T23:52:39Z

lib/model/narrow.dart

  bool containsMessage(MessageBase message) {
    final conversation = message.conversation;
    return conversation is StreamConversation
-      && conversation.streamId == streamId && conversation.topic == topic;
+      && conversation.streamId == streamId && conversation.topic.isSameAs(topic);


Hmm, good catch.

narrow: Use TopicName.isSameAs in TopicNarrow.containsMessage TopicNarrow.containsMessage used to use raw equality operator (==) for comparing topics, which was not ideal for how we treat two topics the same.

Did this have a user-visible symptom? What were the symptoms?

Yeah, there is one: when looking at a topic narrow for a topic named "t", and then there's a new message event in the topic named "T", it will not be shown in the current view.
Will mention this in the commit message.

Cool, thanks. Would you file a quick issue describing that bug? Then this can point to that, which will help us track which release we fix the bug in.

Filed #2060.

gnprice · 2025-11-26T00:02:12Z

lib/model/channel.dart

+  Future<void> fetchChannelTopics(int channelId) async {
+    if (_latestMessageIdsByChannelTopic[channelId] != null) return;
+
+    final result = await _apiGetChannelTopics(connection, channelId: channelId,
+      allowEmptyTopicName: true);
+    (_latestMessageIdsByChannelTopic[channelId] = makeTopicKeyedMap())
+      .addEntries(result.topics.map((entry) => MapEntry(entry.name, entry.maxId)));


If this method gets called while another call is already in the middle of a request, it'll make a fresh request. Then whichever request finishes later will clobber the results of the one that finished first.

Is that a situation that's likely to happen in practice? What would the consequences be?

If we want to handle that situation smoothly, see the logic in GlobalStore.perAccount around _perAccountStoresLoading.

Yeah, that can happen in cases like when a topic-list page is opened, but then before the request is finished, the page is closed and then opened again, resulting in a fresh request. It may result in stale data when the data of the latest request is replaced by an earlier request that took longer.

Added the logic you mentioned to handle this situation.

gnprice · 2025-11-26T00:20:16Z

lib/model/store.dart

        unreads.handleMessageEvent(event);
        recentDmConversationsView.handleMessageEvent(event);
        recentSenders.handleMessage(event.message); // TODO(#824)
+        if (_channels.handleMessageEvent(event)) notifyListeners();


Hmmm — this means that once the user has been either looking at a topic list, or choosing a topic in a channel-narrow compose box, any new-message events in that channel will cause the overall PerAccountStore to notify listeners.

There are a small number of event types that happen frequently: new message, mark as read, typing status, and presence. Because those happen much more often than other events, those are the ones where we've spent effort on having separate ChangeNotifiers so that the bulk of the app's UI doesn't need to rebuild when those events happen.

It'd therefore be good to maintain that property here. Probably cleanest is to have this data structure go on a new substore, maybe TopicListStore. The whole substore could be the ChangeNotifier (like Unreads and friends), or there could be one ChangeNotifier per channel; whichever seems easier to implement is fine.

gnprice · 2025-11-26T00:24:27Z

lib/widgets/topic_list.dart

+    // `maxId` might be incorrect (see [ChannelStore.getChannelTopics]).
+    // Check if it refers to a message that's currently in the topic;
+    // if not, we just won't have `someMessageIdInTopic` for the action sheet.


Hmm interesting. This was already something that could happen, right? So it seems like this change could just as well happen in a separate earlier commit, and that'd be clearer.

What was the consequence of using maxId here without this check?

What was the consequence of using maxId here without this check?

Quoting from #1508 (comment):

If the message ID is not in the topic anymore, then resolve/unresolve will not be successful and will display a dialog with the following message: You only have permission to move the 1/7 most recent messages in this topic.
(the topic where the ID now actually belongs to has 7 messages in total)

That's the case when the message is moved.

And when the message is deleted, it will show the dialog with this message: Invalid message(s).

sm-sayedi · 2025-12-04T16:07:59Z

Thanks @gnprice for the review. New changes pushed.

@chrisbobbe This is now ready for a new round of maintainer review.

chrisbobbe

Thanks! Comments below, all small.

chrisbobbe · 2025-12-10T22:17:00Z

test/model/message_list_test.dart

      check(model).messages.length.equals(30);
    });

+    test('in topic narrow with case-sensitively different topic', () async {


How about 'in topic narrow: topics compared case-insensitively'

("Different topic" is potentially confusing wording, because in Zulip we consider "T" and "t" to be the same topic.)

chrisbobbe · 2025-12-10T22:23:04Z

lib/model/store.dart

      presence: Presence(realm: realm,
        initial: initialSnapshot.presences),
      channels: channels,
+      topics: .new(core: core),


Interesting! This must be the new Dart shorthand feature that's come up a few times in our weekly calls.

Here, I find myself wondering…"new what?" I guess the answer is "new Topics". I may feel differently when we've used this feature in more places in the code, but for now my preference is to stick with the old style: topics: Topics(core: core),.

I think we've been mostly excited for this feature in the context of enums.

chrisbobbe · 2025-12-10T22:28:10Z

lib/model/topics.dart

+      (_latestMessageIdsByChannelTopic[channelId] = makeTopicKeyedMap())
+        .addEntries(result.topics.map((entry) => .new(entry.name, entry.maxId)));


Let's add an assert(_latestMessageIdsByChannelTopic[channelId] == null) just above this.

Also same comment about the .new shorthand: I may feel differently later, but for now I don't love it; it feels like I'm losing an opportunity to see really quickly what kind of new thing is being created.

chrisbobbe · 2025-12-10T22:34:50Z

lib/model/topics.dart

+      switch (propagateMode) {
+        case .changeOne:
+        case .changeLater:


On the other hand, the shorthand as it's used here looks great to me!

chrisbobbe · 2025-12-10T22:40:18Z

test/model/topics_test.dart

+    connection.prepare(json: GetChannelTopicsResult(topics: [
+      eg.getChannelTopicsEntry(name: 'foo', maxId: 100),
+    ]).toJson(), delay: Duration(seconds: 3));


The delay is important for the story of the test, right, but I feel like it's getting a little lost when it shares a line with the characters ]).toJson(), . How about putting it on its own line, maybe something like this:

connection.prepare( json: GetChannelTopicsResult( topics: [eg.getChannelTopicsEntry(name: 'foo', maxId: 100)]).toJson(), delay: Duration(seconds: 3));

I've often liked putting the delay parameter first, before the content of the response.

After all, the delay happens chronologically prior to the response arriving. The delay is also in the nature of metadata, while the response (json:) is the content or payload — similar to a child or children parameter on a widget constructor, which we always put last.

chrisbobbe · 2025-12-10T22:53:19Z

test/model/topics_test.dart

+        checkNotifiedOnce();
+      });
+
+      test('topic moved to known topic in the same channel', () async {


(same comment about "existing" vs. "known")

chrisbobbe · 2025-12-10T22:57:39Z

test/model/autocomplete_test.dart

+
+    connection.prepare(json: GetChannelTopicsResult(topics: [topic1, topic2]).toJson());
+    final view1 = TopicAutocompleteView.init(store: store, streamId: 1000,
+      query: .new(''));


(same comment about .new, here and below)

chrisbobbe · 2025-12-10T23:07:13Z

lib/widgets/topic_list.dart

+    unreadsModel = newStore.unreads..addListener(_modelChanged);
+    topicsModel?.removeListener(_modelChanged);
+    topicsModel = newStore.topics..addListener(_modelChanged);
+    _fetchTopics(topicsModel!);


I think we don't have to pass topicsModel through like this; the _fetchTopics implementation can just look it up using the getter, with !:

diff --git lib/widgets/topic_list.dart lib/widgets/topic_list.dart index 1e29c5516..73673f38a 100644 --- lib/widgets/topic_list.dart +++ lib/widgets/topic_list.dart @@ -135,7 +135,7 @@ class _TopicListState extends State<_TopicList> with PerAccountStoreAwareStateMi unreadsModel = newStore.unreads..addListener(_modelChanged); topicsModel?.removeListener(_modelChanged); topicsModel = newStore.topics..addListener(_modelChanged); - _fetchTopics(topicsModel!); + _fetchTopics(); } @override @@ -150,11 +150,11 @@ class _TopicListState extends State<_TopicList> with PerAccountStoreAwareStateMi }); } - void _fetchTopics(Topics topicsModel) async { + void _fetchTopics() async { // Do nothing when the fetch fails; the topic-list will stay on // the loading screen, until the user navigates away and back. // TODO(design) show a nice error message on screen when this fails - await topicsModel.fetchChannelTopics(widget.streamId); + await topicsModel!.fetchChannelTopics(widget.streamId); if (!mounted) return; setState(() { // The actual state lives in the `topicsModel`.

chrisbobbe · 2025-12-10T23:07:48Z

lib/widgets/topic_list.dart

-      // The actual state lives in `unreadsModel`.
+      // The actual states lives in `unreadsModel` and `topicsModel`.


nit: "live"

chrisbobbe · 2025-12-10T23:08:28Z

lib/widgets/topic_list.dart

-    _fetchTopics();
+    unreadsModel = newStore.unreads..addListener(_modelChanged);
+    topicsModel?.removeListener(_modelChanged);
+    topicsModel = newStore.topics..addListener(_modelChanged);


We should remove the listener in dispose.

sm-sayedi · 2025-12-11T03:30:38Z

Thanks for the review. Pushed the new revision.

chrisbobbe · 2025-12-23T21:50:40Z

Thanks! LGTM; marking for Greg's review.

gnprice

Thanks @sm-sayedi for picking this up, and @chrisbobbe for the previous reviews! Comments below.

I've read the first 8 commits (including the main model commit):
200f744 narrow: Use TopicName.isSameAs in TopicNarrow.containsMessage
6ad5a40 api [nfc]: Rename GetStreamTopicsEntry to GetChannelTopicsEntry
6b1fdcd example data [nfc]: Rename getStreamTopicsEntry to getChannelTopicsEntry
2497a31 api [nfc]: Rename GetStreamTopicsResult to GetChannelTopicsResult
8f80e29 api [nfc]: Rename getStreamTopics to getChannelTopics
6857a4b topics: Introduce Topics model for tracking known channel topics
2d0df3c autocomplete test [nfc]: s/streams/topics in a topic test name
33e848f autocomplete test [nfc]: Add and use isTopic condition

I'll leave the remaining 3 for a future round:
ec9c495 autocomplete: Use topic data from Topics model in topic autocomplete
ecc407c topics: Pass maxId to topic sheet if the related message is still in topic
0235d91 topics: Keep topic-list page updated, by using data from Topics model

gnprice · 2025-12-27T05:36:02Z

lib/model/narrow.dart

  bool containsMessage(MessageBase message) {
    final conversation = message.conversation;
    return conversation is StreamConversation
-      && conversation.streamId == streamId && conversation.topic == topic;
+      && conversation.streamId == streamId && conversation.topic.isSameAs(topic);


Cool, thanks. Would you file a quick issue describing that bug? Then this can point to that, which will help us track which release we fix the bug in.

gnprice · 2025-12-27T05:43:00Z

lib/widgets/topic_list.dart

    setState(() {
-      lastFetchedTopics = result.topics;
+      // The actual state lives in the `topicsModel`.
    });


Is this still needed? Seems like the listener on Topics should cover it.

gnprice · 2025-12-27T05:44:13Z

lib/model/autocomplete.dart

+    await store.topics.fetchChannelTopics(streamId); // TODO: handle fetch failure
+    _topics = store.topics.getChannelTopics(streamId)!.map((e) => e.name);
    return _startSearch();


Hmm. If a fetch is already underway, then this fetchChannelTopics will complete immediately, right? Then we'll attempt to start the search but we have no data to search through.

gnprice · 2025-12-27T05:47:14Z

lib/model/topics.dart

+  ///
+  /// Once fetched, the data will be updated by events;
+  /// use [getChannelTopics] to consume the data.
+  Future<void> fetchChannelTopics(int channelId) async {


Do the callers of this method want to await it?

If they do, then they probably want to await the fetch being finished. As is, the future returned by this method can have two quite different meanings:

Sometimes it means the result of the actual fetch.

Other times it just means that a fetch was ongoing when the method was called, at which the method immediately returned.

The caller can't tell which meaning the future has. It's unlikely that something with those two different meanings is possible to use in a way that has an effect but doesn't have a bug in one of those cases.

If callers don't want to await the result, then the method can return void rather than Future<void>, to indicate that.

In some cases the callers need to wait for it, for example in topic autocomplete. Changed it so that it returns the ongoing future instead, so it has one meaning.

gnprice · 2025-12-27T06:02:01Z

test/model/topics_test.dart

+    await model.fetchChannelTopics(channel.streamId);
+    check(connection.takeRequests()).isEmpty();
+    check(model).getChannelTopics(channel.streamId).isNotNull()


These test cases should be checking whether listeners are notified or not (just like many of the later test cases do).

In general, all the test cases that are about making changes to the data — therefore something that potentially might cause listeners to be notified — should be checking that.

gnprice · 2025-12-27T06:07:50Z

test/model/topics_test.dart

+      assert(messages.isNotEmpty);
+      assert(messages.every((m) => m.streamId == channel.streamId));
+      await store.addMessages(messages);
+      check(model).getChannelTopics(channel.streamId).isNotNull().deepEquals(expectedTopics);


I think this step would be clearest at the respective call sites. It'd make them only one line longer; and the benefit is that it makes this check a visible point of comparison for the later checks in each test.

The point of this check is basically to be a baseline for those later checks, so that's a significant benefit in ease of reading.

gnprice · 2025-12-27T06:16:24Z

test/model/topics_test.dart

+      });
+    });
+
+    group('PropagateMode.changeLater', () {


This behaves exactly the same as .changeOne, right?

I don't really want to read 🙂 additional tests that say almost the same thing as the tests above. Let's pick just one of these .changeLater tests, whichever seems most emblematic of the .changeLater behavior

gnprice · 2025-12-27T06:17:17Z

test/model/autocomplete_test.dart

-  test('TopicAutocompleteView updates results when streams are loaded', () async {
+  test('TopicAutocompleteView updates results when topics are loaded', () async {


nit:

autocomplete test [nfc]: s/streams/topics in a topic test name

Include a closing slash:

autocomplete test [nfc]: s/streams/topics/ in a topic test name

Otherwise it looks like the replacement text extends through the rest of the line: "topics in a topic test name".

TopicNarrow.containsMessage used to use raw equality operator (==) for comparing topics, which was not ideal for how we treat two topics the same. This would result in not displaying a new message of topic "T" when looking at a topic narrow of topic "t" (treated message topics case-sensitively in topic narrow). Fixes: zulip#2060

To make things consistent for renaming the binding getStreamTopics to getChannelTopics in the following commits.

sm-sayedi · 2025-12-30T16:46:38Z

Thanks Greg for the review. Pushed new changes, PTAL.

gnprice

Thanks for the revision! Generally this looks good. I've now read the whole thing; comments below.

gnprice · 2025-12-30T23:39:16Z

lib/model/topics.dart

+  }
+
+
+  void handleMessageEvent(MessageEvent event) {


nit: double blank line

gnprice · 2025-12-30T23:49:31Z

lib/model/topics.dart

+      topics = await future;
+      notifyListeners();


nit: I think it's a bit clearer for the reader to verify the logic here if this notifyListeners happens next to the mutations that make it necessary, i.e. in _getChannelTopics where we update _latestMessageIdsByChannelTopic.

gnprice · 2025-12-31T00:01:03Z

lib/model/autocomplete.dart


-  /// Fetches topics of the current stream narrow, expected to fetch
-  /// only once per lifecycle.
+  /// Fetches topics of the current stream narrow.


Suggested change

/// Fetches topics of the current stream narrow.

/// Fetches topics of the current stream narrow, if needed.

Since this no longer necessarily causes a fetch.

gnprice · 2025-12-31T00:13:11Z

lib/widgets/topic_list.dart

    final designVariables = DesignVariables.of(context);
+    // `maxId` might be incorrect (see [ChannelStore.getChannelTopics]).


nit: separate stanza, because this multi-line calculation has a different character from the two quick boring lookups preceding it

Suggested change

final designVariables = DesignVariables.of(context);

// `maxId` might be incorrect (see [ChannelStore.getChannelTopics]).

final designVariables = DesignVariables.of(context);

// `maxId` might be incorrect (see [ChannelStore.getChannelTopics]).

gnprice · 2025-12-31T00:13:48Z

lib/widgets/topic_list.dart

+    final someMessageIdInTopic =
+      (maxIdMessage != null && TopicNarrow(streamId, topic).containsMessage(maxIdMessage))
+        ? maxIdMessage.id
+        : null;


topics: Pass `maxId` to topic sheet if the related message is still in topic

This should say "only if", not plain "if". As is, it sounds like the change is that we now start passing maxId when we didn't before.

gnprice · 2025-12-31T00:15:29Z

lib/widgets/topic_list.dart

+    unreadsModel = newStore.unreads..addListener(_modelChanged);
+    topicsModel?.removeListener(_modelChanged);


nit: put topicsModel before unreadsModel (here and in declarations); seems like it's more fundamental to what this topic-list widget is doing

gnprice · 2025-12-31T00:17:11Z

lib/widgets/topic_list.dart

-      // The actual state lives in `unreadsModel`.
+      // The actual states live in `unreadsModel` and `topicsModel`.


nit: I'd still say "state lives", singular — "the state" is a bit abstract but describes the whole ensemble of data that drives this widget

gnprice · 2025-12-31T00:22:49Z

test/widgets/topic_list_test.dart

+    check(connection.takeRequests())
+    ..length.equals(2) // one for the messages request, another for the topics


This can be made a bit cleaner by turning this comment into self-checking code: put another connection.takeRequests call sooner, after the messages request, and just check there that there's 1 request. (Or not even check that, since the point of this test is about the requests on the topic-list page.)

That way this one covers only the request(s) made at this step of navigating to the topic-list page, and it can make a stronger check by saying there's just this one request.

gnprice · 2025-12-31T00:30:56Z

test/widgets/topic_list_test.dart

+    // … then back to the topic-list page, expecting not to fetch again but
+    // use existing data which is kept up-to-date anyway.


This comment reads a little oddly. I think it also doesn't entirely make sense when reading the new version of the code as it is — it's really more part of a conversation about the change from the old version (before this PR) to the new version.

Instead, that conversation should be kept to commit messages (and PR descriptions), and the comments in the code should be about the current version of the code:
https://github.com/zulip/zulip-mobile/blob/main/docs/style.md#commit-messages-vs-comments

So e.g.:

// … then back to the topic-list page. No new fetch request occurs.

sm-sayedi · 2025-12-31T03:46:49Z

Thanks for the review. Pushed new revision.

Co-authored-by: Zixuan James Li <zixuan@zulip.com>

This will be cleaner when we add more tests in future commits.

…ill in topic Co-authored-by: Zixuan James Li <zixuan@zulip.com>

Fixes: zulip#1499 Co-authored-by: Zixuan James Li <zixuan@zulip.com>

gnprice · 2026-01-02T19:55:18Z

Thanks! Looks good; merging.

sm-sayedi added the maintainer review PR ready for review by Zulip maintainers label Oct 28, 2025

sm-sayedi requested a review from chrisbobbe October 28, 2025 21:25

sm-sayedi assigned chrisbobbe Oct 28, 2025

sm-sayedi force-pushed the 1499-track-topics branch from 1019746 to 2e0fcfd Compare November 3, 2025 17:06

chrisbobbe reviewed Nov 4, 2025

View reviewed changes

sm-sayedi force-pushed the 1499-track-topics branch from 2e0fcfd to c37de07 Compare November 4, 2025 19:18

sm-sayedi requested a review from chrisbobbe November 4, 2025 19:22

sm-sayedi force-pushed the 1499-track-topics branch 2 times, most recently from 02ed253 to 7fca9bb Compare November 14, 2025 13:54

chrisbobbe reviewed Nov 19, 2025

View reviewed changes

sm-sayedi force-pushed the 1499-track-topics branch from 7fca9bb to 62ed7ea Compare November 24, 2025 06:28

sm-sayedi mentioned this pull request Nov 25, 2025

Handle more cases/events where topic data in store needs changing #2004

Open

sm-sayedi force-pushed the 1499-track-topics branch from 62ed7ea to b008d16 Compare November 25, 2025 02:32

sm-sayedi requested a review from chrisbobbe November 25, 2025 02:35

sm-sayedi force-pushed the 1499-track-topics branch from b008d16 to ec8f84d Compare November 25, 2025 15:01

gnprice reviewed Nov 26, 2025

View reviewed changes

gnprice mentioned this pull request Nov 26, 2025

msglist: Fetch newer messages despite previous muted batch #1989

Open

sm-sayedi force-pushed the 1499-track-topics branch 6 times, most recently from e6866a2 to 7f1e560 Compare December 4, 2025 15:58

chrisbobbe reviewed Dec 10, 2025

View reviewed changes

sm-sayedi force-pushed the 1499-track-topics branch from 7f1e560 to 1c569dc Compare December 11, 2025 03:17

sm-sayedi requested a review from chrisbobbe December 11, 2025 03:30

chrisbobbe requested a review from gnprice December 23, 2025 21:50

chrisbobbe assigned gnprice and unassigned chrisbobbe Dec 23, 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 23, 2025

sm-sayedi force-pushed the 1499-track-topics branch from 72b24e2 to 0235d91 Compare December 24, 2025 02:59

gnprice reviewed Dec 27, 2025

View reviewed changes

sm-sayedi added 5 commits December 30, 2025 21:09

api [nfc]: Rename GetStreamTopicsEntry to GetChannelTopicsEntry

b0e7c1b

To make things consistent for renaming the binding getStreamTopics to getChannelTopics in the following commits.

example data [nfc]: Rename getStreamTopicsEntry to getChannelTopicsEntry

1d3fead

To make things consistent for renaming the binding getStreamTopics to getChannelTopics in the following commits.

api [nfc]: Rename GetStreamTopicsResult to GetChannelTopicsResult

74af33d

To make things consistent for renaming the binding getStreamTopics to getChannelTopics in the following commits.

api [nfc]: Rename getStreamTopics to getChannelTopics

d30b0bf

sm-sayedi force-pushed the 1499-track-topics branch from 0235d91 to f87c571 Compare December 30, 2025 16:40

sm-sayedi requested a review from gnprice December 30, 2025 16:46

gnprice reviewed Dec 31, 2025

View reviewed changes

sm-sayedi force-pushed the 1499-track-topics branch from f87c571 to 77cf465 Compare December 31, 2025 03:46

sm-sayedi requested a review from gnprice December 31, 2025 03:47

sm-sayedi and others added 6 commits December 31, 2025 08:28

topics: Introduce Topics model for tracking known channel topics

299ef48

Co-authored-by: Zixuan James Li <zixuan@zulip.com>

autocomplete test [nfc]: s/streams/topics/ in a topic test name

5c79a45

autocomplete test [nfc]: Add and use isTopic condition

9bb8bd4

This will be cleaner when we add more tests in future commits.

autocomplete: Use topic data from Topics model in topic autocomplete

dc0d95d

topics: Pass maxId to topic sheet only if the related message is st…

e04bf1a

…ill in topic Co-authored-by: Zixuan James Li <zixuan@zulip.com>

topics: Keep topic-list page updated, by using data from Topics model

5d27008

Fixes: zulip#1499 Co-authored-by: Zixuan James Li <zixuan@zulip.com>

sm-sayedi force-pushed the 1499-track-topics branch from 77cf465 to 5d27008 Compare December 31, 2025 03:58

gnprice merged commit 5d27008 into zulip:main Jan 2, 2026
1 check passed

gnprice mentioned this pull request Jan 2, 2026

msglist: Add in-conversation-narrow asserts in _messageVisible #2051

Merged

	/// Pairs of the known topics and its latest message ID, in the given channel.
	/// The topics in the given channel, along with their latest message ID.

	/// Returns null if the data has never been fetched yet.
	/// Returns null if the data has not been fetched yet.

		/// The result is guaranteed to be sorted by [GetStreamTopicsEntry.maxId]
		/// descending, and the topics are guaranteed to be distinct.

-  /// In some cases, the same maxId affected by message moves can be present in
-  /// multiple [GetStreamTopicsEntry] entries. For this reason, the caller
-  /// should not rely on [getChannelTopics] to determine which topic the message
-  /// is in. Instead, refer to [PerAccountStore.messages].
-  /// See [handleUpdateMessageEvent] on how this could happen.
+  /// Occasionally, [GetStreamTopicsEntry.maxId] will refer to a message
+  /// that doesn't exist or is no longer in the topic.
+  /// This happens when a topic's latest message is moved or deleted
+  /// and we don't have enough information
+  /// to replace [GetStreamTopicsEntry.maxId] accurately.
+  /// (We don't keep a snapshot of all messages.)
+  /// Use [PerAccountStore.messages] to check a message's topic accurately.

	final StreamMessage(:streamId, :topic) = event.message as StreamMessage;
	final StreamMessage(streamId: channelId, :topic) = event.message as StreamMessage;

	/// The topics in the given channel, along with their latest message ID.
	/// The topics the user can access, along with their latest message ID,
	/// reflecting updates from events that arrived since the data was fetched.

		connection.prepare(json: GetStreamTopicsResult(
		topics: [eg.getStreamTopicsEntry(name: 'topic B')]).toJson());

		// After the move, the message with maxId moved away from "foo". The topic
		// actions that require `someMessageIdInTopic` is no longer available.

		check(find.descendant(of: topicItemFinder,
		matching: find.text('foo'))).findsOne();

		(_latestMessageIdsByChannelTopic[channelId] = makeTopicKeyedMap())
		.addEntries(result.topics.map((entry) => .new(entry.name, entry.maxId)));

		// The actual state lives in `unreadsModel`.
		// The actual states lives in `unreadsModel` and `topicsModel`.

		test('TopicAutocompleteView updates results when streams are loaded', () async {
		test('TopicAutocompleteView updates results when topics are loaded', () async {

	/// Fetches topics of the current stream narrow.
	/// Fetches topics of the current stream narrow, if needed.

		final designVariables = DesignVariables.of(context);
		// `maxId` might be incorrect (see [ChannelStore.getChannelTopics]).

		unreadsModel = newStore.unreads..addListener(_modelChanged);
		topicsModel?.removeListener(_modelChanged);

		// The actual state lives in `unreadsModel`.
		// The actual states live in `unreadsModel` and `topicsModel`.

		check(connection.takeRequests())
		..length.equals(2) // one for the messages request, another for the topics

		// … then back to the topic-list page, expecting not to fetch again but
		// use existing data which is kept up-to-date anyway.

Keep track of the known topics in store #1951

Keep track of the known topics in store #1951

Uh oh!

Conversation

sm-sayedi commented Oct 28, 2025

Uh oh!

chrisbobbe 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!

Choose a reason for hiding this comment

Uh oh!

sm-sayedi commented Nov 4, 2025

Uh oh!

chrisbobbe 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sm-sayedi commented Nov 25, 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!

sm-sayedi Dec 2, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sm-sayedi Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

sm-sayedi Dec 2, 2025 •

edited

Loading

sm-sayedi Nov 26, 2025 •

edited

Loading