From 4163ebe12b8c72b6dec80983aa58fed5adf9cff1 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Wed, 29 Oct 2025 01:00:05 +0430 Subject: [PATCH 1/5] channel: Keep track of channel topics, and keep up-to-date with events Co-authored-by: Sayed Mahmood Sayedi --- lib/model/channel.dart | 128 +++++++++++ lib/model/store.dart | 2 + test/api/route/route_checks.dart | 7 + test/model/channel_test.dart | 362 +++++++++++++++++++++++++++++++ test/model/store_checks.dart | 2 + 5 files changed, 501 insertions(+) diff --git a/lib/model/channel.dart b/lib/model/channel.dart index 10e9596eed..31f2088ba9 100644 --- a/lib/model/channel.dart +++ b/lib/model/channel.dart @@ -1,14 +1,20 @@ +import 'dart:async'; import 'dart:collection'; +import 'package:collection/collection.dart'; import 'package:flutter/foundation.dart'; import '../api/model/events.dart'; import '../api/model/initial_snapshot.dart'; import '../api/model/model.dart'; +import '../api/route/channels.dart'; import 'realm.dart'; import 'store.dart'; import 'user.dart'; +// similar to _apiSendMessage in lib/model/message.dart +final _apiGetChannelTopics = getStreamTopics; + /// The portion of [PerAccountStore] for channels, topics, and stuff about them. /// /// This type is useful for expressing the needs of other parts of the @@ -78,6 +84,29 @@ mixin ChannelStore on UserStore { }; } + /// Fetch topics in a channel from the server, only if they're not fetched yet. + /// + /// The results from the last successful fetch + /// can be retrieved with [getChannelTopics]. + Future fetchTopics(int channelId); + + /// The topics in the given channel, along with their latest message ID. + /// + /// Returns null if the data has not been fetched yet. + /// To fetch it from the server, use [fetchTopics]. + /// + /// The result is sorted by [GetStreamTopicsEntry.maxId] descending, + /// and the topics are distinct. + /// + /// 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 deleted or moved + /// 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. + List? getChannelTopics(int channelId); + /// The visibility policy that the self-user has for the given topic. /// /// This does not incorporate the user's channel-level policy, @@ -288,6 +317,13 @@ mixin ProxyChannelStore on ChannelStore { @override Map get channelFolders => channelStore.channelFolders; + @override + Future fetchTopics(int channelId) => channelStore.fetchTopics(channelId); + + @override + List? getChannelTopics(int channelId) => + channelStore.getChannelTopics(channelId); + @override UserTopicVisibilityPolicy topicVisibilityPolicy(int streamId, TopicName topic) => channelStore.topicVisibilityPolicy(streamId, topic); @@ -368,6 +404,39 @@ class ChannelStoreImpl extends HasUserStore with ChannelStore { @override final Map channelFolders; + /// Maps indexed by channel IDs, of the known latest message IDs in each topic. + /// + /// For example: `_latestMessageIdsByChannelTopic[channel.streamId][topic] = maxId` + /// + /// Occasionally, the latest message ID of a topic will refer to a message + /// that doesn't exist or is no longer in the topic. + /// This happens when the topic's latest message is deleted or moved + /// and we don't have enough information to replace it accurately. + /// (We don't keep a snapshot of all messages.) + final Map> _latestMessageIdsByChannelTopic = {}; + + @override + Future fetchTopics(int channelId) async { + if (_latestMessageIdsByChannelTopic[channelId] != null) return; + + final result = await _apiGetChannelTopics(connection, streamId: channelId, + allowEmptyTopicName: true); + (_latestMessageIdsByChannelTopic[channelId] = makeTopicKeyedMap()).addAll({ + for (final GetStreamTopicsEntry(:name, :maxId) in result.topics) + name: maxId, + }); + } + + @override + List? getChannelTopics(int channelId) { + final latestMessageIdsByTopic = _latestMessageIdsByChannelTopic[channelId]; + if (latestMessageIdsByTopic == null) return null; + return [ + for (final MapEntry(:key, :value) in latestMessageIdsByTopic.entries) + GetStreamTopicsEntry(maxId: value, name: key), + ].sortedBy((value) => -value.maxId); + } + @override Map> get debugTopicVisibility => topicVisibility; @@ -572,6 +641,65 @@ class ChannelStoreImpl extends HasUserStore with ChannelStore { forStream[event.topicName] = visibilityPolicy; } } + + /// Handle a [MessageEvent], returning whether listeners should be notified. + bool handleMessageEvent(MessageEvent event) { + if (event.message is! StreamMessage) return false; + final StreamMessage(streamId: channelId, :topic) = event.message as StreamMessage; + + final latestMessageIdsByTopic = _latestMessageIdsByChannelTopic[channelId]; + 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; + } + + final currentLatestMessageId = latestMessageIdsByTopic[topic]; + if (currentLatestMessageId != null && currentLatestMessageId >= event.message.id) { + // The event raced with a message fetch. + return false; + } + latestMessageIdsByTopic[topic] = event.message.id; + return true; + } + + /// Handle an [UpdateMessageEvent], returning whether listeners should be + /// notified. + bool handleUpdateMessageEvent(UpdateMessageEvent event) { + if (event.moveData == null) return false; + final UpdateMessageMoveData( + :origStreamId, :origTopic, :newStreamId, :newTopic, :propagateMode, + ) = event.moveData!; + bool shouldNotify = false; + + 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; + } + } + + 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; + } + } + + return shouldNotify; + } } /// A [Map] with [TopicName] keys and [V] values. diff --git a/lib/model/store.dart b/lib/model/store.dart index fda6e5b695..bd1cf45c5a 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -862,6 +862,7 @@ class PerAccountStore extends PerAccountStoreBase with unreads.handleMessageEvent(event); recentDmConversationsView.handleMessageEvent(event); recentSenders.handleMessage(event.message); // TODO(#824) + if (_channels.handleMessageEvent(event)) notifyListeners(); // When adding anything here (to handle [MessageEvent]), // it probably belongs in [reconcileMessages] too. @@ -869,6 +870,7 @@ class PerAccountStore extends PerAccountStoreBase with assert(debugLog("server event: update_message ${event.messageId}")); _messages.handleUpdateMessageEvent(event); unreads.handleUpdateMessageEvent(event); + if (_channels.handleUpdateMessageEvent(event)) notifyListeners(); case DeleteMessageEvent(): assert(debugLog("server event: delete_message ${event.messageIds}")); diff --git a/test/api/route/route_checks.dart b/test/api/route/route_checks.dart index daa4628efd..2e9825a790 100644 --- a/test/api/route/route_checks.dart +++ b/test/api/route/route_checks.dart @@ -1,4 +1,6 @@ import 'package:checks/checks.dart'; +import 'package:zulip/api/model/model.dart'; +import 'package:zulip/api/route/channels.dart'; import 'package:zulip/api/route/messages.dart'; import 'package:zulip/api/route/realm.dart'; import 'package:zulip/api/route/saved_snippets.dart'; @@ -15,4 +17,9 @@ extension GetServerSettingsResultChecks on Subject { Subject get realmUrl => has((e) => e.realmUrl, 'realmUrl'); } +extension GetStreamTopicEntryChecks on Subject { + Subject get maxId => has((e) => e.maxId, 'maxId'); + Subject get name => has((e) => e.name, 'name'); +} + // TODO add similar extensions for other classes in api/route/*.dart diff --git a/test/model/channel_test.dart b/test/model/channel_test.dart index 6c2de3e83c..21c1e195d2 100644 --- a/test/model/channel_test.dart +++ b/test/model/channel_test.dart @@ -1,16 +1,21 @@ import 'package:checks/checks.dart'; import 'package:collection/collection.dart'; +import 'package:http/http.dart' as http; import 'package:test/scaffolding.dart'; import 'package:zulip/api/model/events.dart'; import 'package:zulip/api/model/initial_snapshot.dart'; import 'package:zulip/api/model/model.dart'; +import 'package:zulip/api/route/channels.dart'; import 'package:zulip/model/store.dart'; import 'package:zulip/model/channel.dart'; +import '../api/fake_api.dart'; import '../api/model/model_checks.dart'; +import '../api/route/route_checks.dart'; import '../example_data.dart' as eg; import '../stdlib_checks.dart'; import 'binding.dart'; +import 'store_checks.dart'; import 'test_store.dart'; void main() { @@ -221,6 +226,363 @@ void main() { }); }); + group('topics data', () { + final channel = eg.stream(); + final otherChannel = eg.stream(); + late PerAccountStore store; + late FakeApiConnection connection; + late int notifiedCount; + + void checkNotified({required int count}) { + check(notifiedCount).equals(count); + notifiedCount = 0; + } + void checkNotNotified() => checkNotified(count: 0); + void checkNotifiedOnce() => checkNotified(count: 1); + + Condition isStreamTopicsEntry(String topic, int maxId) { + return (it) => it.isA() + ..maxId.equals(maxId) + ..name.equals(eg.t(topic)); + } + + Future prepare({ + List? topics, + List? messages, + }) async { + notifiedCount = 0; + store = eg.store(); + await store.addStreams([channel, otherChannel]); + await store.addSubscriptions([ + eg.subscription(channel), + eg.subscription(otherChannel), + ]); + await store.addMessages(messages ?? []); + store.addListener(() { + notifiedCount++; + }); + + connection = store.connection as FakeApiConnection; + connection.prepare(json: GetStreamTopicsResult(topics: topics ?? []).toJson()); + await store.fetchTopics(channel.streamId); + check(connection.takeRequests()).single.isA() + ..method.equals('GET') + ..url.path.equals('/api/v1/users/me/${channel.streamId}/topics'); + } + + test('fetch topics -> update data, try fetching again -> no request sent', () async { + await prepare(topics: [ + eg.getStreamTopicsEntry(name: 'foo', maxId: 100), + eg.getStreamTopicsEntry(name: 'bar', maxId: 200), + eg.getStreamTopicsEntry(name: 'baz', maxId: 300), + ]); + check(store).getStreamTopics(channel.streamId).isNotNull().deepEquals([ + isStreamTopicsEntry('baz', 300), + isStreamTopicsEntry('bar', 200), + isStreamTopicsEntry('foo', 100), + ]); + + connection.prepare(json: GetStreamTopicsResult(topics: [ + eg.getStreamTopicsEntry(name: 'foo', maxId: 100), + eg.getStreamTopicsEntry(name: 'bar', maxId: 200), + eg.getStreamTopicsEntry(name: 'baz', maxId: 300), + ]).toJson()); + await store.fetchTopics(channel.streamId); + check(connection.takeRequests()).isEmpty(); + }); + + test('getChannelTopics sorts descending by maxId', () async { + await prepare(topics: [ + eg.getStreamTopicsEntry(name: 'bar', maxId: 200), + eg.getStreamTopicsEntry(name: 'foo', maxId: 100), + eg.getStreamTopicsEntry(name: 'baz', maxId: 300), + ]); + check(store).getStreamTopics(channel.streamId).isNotNull().deepEquals([ + isStreamTopicsEntry('baz', 300), + isStreamTopicsEntry('bar', 200), + isStreamTopicsEntry('foo', 100), + ]); + + await store.addMessage(eg.streamMessage( + id: 301, stream: channel, topic: 'foo')); + check(store).getStreamTopics(channel.streamId).isNotNull().deepEquals([ + isStreamTopicsEntry('foo', 301), + isStreamTopicsEntry('baz', 300), + isStreamTopicsEntry('bar', 200), + ]); + }); + + group('handleMessageEvent', () { + test('new message in fetched channel', () async { + await prepare(topics: [ + eg.getStreamTopicsEntry(name: 'old topic', maxId: 1), + ]); + + await store.addMessage( + eg.streamMessage(id: 2, stream: channel, topic: 'new topic')); + check(store).getStreamTopics(channel.streamId).isNotNull().deepEquals([ + isStreamTopicsEntry('new topic', 2), + isStreamTopicsEntry('old topic', 1), + ]); + checkNotifiedOnce(); + + await store.addMessage( + eg.streamMessage(id: 3, stream: channel, topic: 'old topic')); + check(store).getStreamTopics(channel.streamId).isNotNull().deepEquals([ + isStreamTopicsEntry('old topic', 3), + isStreamTopicsEntry('new topic', 2), + ]); + checkNotifiedOnce(); + }); + + test('new message in channel not fetched before', () async { + await prepare(); + check(store).getStreamTopics(otherChannel.streamId).isNull(); + + await store.addMessage( + eg.streamMessage(id: 2, stream: otherChannel, topic: 'new topic')); + check(store).getStreamTopics(otherChannel.streamId).isNull(); + checkNotNotified(); + }); + + test('new message with equal or lower message ID', () async { + await prepare(topics: [ + eg.getStreamTopicsEntry(name: 'topic', maxId: 2), + ]); + + await store.addMessage( + eg.streamMessage(id: 2, stream: channel, topic: 'topic')); + check(store).getStreamTopics(channel.streamId).isNotNull().single + ..name.equals(eg.t('topic')) + ..maxId.equals(2); + checkNotNotified(); + + await store.addMessage( + eg.streamMessage(id: 1, stream: channel, topic: 'topic')); + check(store).getStreamTopics(channel.streamId).isNotNull().single + ..name.equals(eg.t('topic')) + ..maxId.equals(2); + checkNotNotified(); + }); + + test('ignore DM messages', () async { + await prepare(); + await store.addUsers([eg.selfUser, eg.otherUser]); + checkNotified(count: 2); + + await store.addMessage(eg.dmMessage(from: eg.selfUser, to: [eg.otherUser])); + checkNotNotified(); + }); + }); + + group('handleUpdateMessageEvent', () { + Future prepareWithMessages(List messages, { + required List> expectedTopics, + }) async { + await prepare(); + assert(messages.isNotEmpty); + assert(messages.every((m) => m.streamId == channel.streamId)); + await store.addMessages(messages); + check(store).getStreamTopics(channel.streamId).isNotNull().deepEquals(expectedTopics); + checkNotified(count: messages.length); + } + + test('ignore content only update', () async { + final message = eg.streamMessage(id: 123, stream: channel, topic: 'foo'); + await prepareWithMessages([message], expectedTopics: [ + isStreamTopicsEntry('foo', 123), + ]); + + await store.handleEvent(eg.updateMessageEditEvent(message)); + checkNotNotified(); + }); + + group('PropagateMode.changedAll', () { + test('topic moved to another stream with no previously fetched topics', () async { + final messagesToMove = List.generate(10, (i) => + eg.streamMessage(id: 100 + i, stream: channel, topic: 'foo')); + await prepareWithMessages(messagesToMove, expectedTopics: [ + isStreamTopicsEntry('foo', 109), + ]); + + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: messagesToMove, + newStreamId: otherChannel.streamId, + propagateMode: PropagateMode.changeAll)); + check(store).getStreamTopics(channel.streamId).isNotNull().isEmpty(); + check(store).getStreamTopics(otherChannel.streamId).isNull(); + checkNotifiedOnce(); + }); + + test('topic moved to new topic in another stream', () async { + final messagesToMove = List.generate(10, (i) => + eg.streamMessage(id: 100 + i, stream: channel, topic: 'foo')); + await prepareWithMessages(messagesToMove, expectedTopics: [ + isStreamTopicsEntry('foo', 109), + ]); + + // Make sure that topics in otherChannel has been fetched. + connection.prepare(json: GetStreamTopicsResult(topics: [ + eg.getStreamTopicsEntry(name: 'foo', maxId: 1), + ]).toJson()); + await store.fetchTopics(otherChannel.streamId); + check(store).getStreamTopics(otherChannel.streamId).isNotNull().single + ..name.equals(eg.t('foo')) + ..maxId.equals(1); + checkNotNotified(); + + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: messagesToMove, + newStreamId: otherChannel.streamId, + newTopicStr: 'bar', + propagateMode: PropagateMode.changeAll)); + check(store).getStreamTopics(channel.streamId).isNotNull().isEmpty(); + check(store).getStreamTopics(otherChannel.streamId).isNotNull().deepEquals([ + isStreamTopicsEntry('bar', 109), + isStreamTopicsEntry('foo', 1), + ]); + checkNotifiedOnce(); + }); + + test('topic moved to known topic in another stream', () async { + final messagesToMove = List.generate(10, (i) => + eg.streamMessage(id: 100 + i, stream: channel, topic: 'foo')); + await prepareWithMessages(messagesToMove, expectedTopics: [ + isStreamTopicsEntry('foo', 109), + ]); + + // Make sure that topics in otherChannel has been fetched. + connection.prepare(json: GetStreamTopicsResult(topics: [ + eg.getStreamTopicsEntry(name: 'foo', maxId: 1), + ]).toJson()); + await store.fetchTopics(otherChannel.streamId); + check(store).getStreamTopics(otherChannel.streamId).isNotNull().single + ..name.equals(eg.t('foo')) + ..maxId.equals(1); + checkNotNotified(); + + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: messagesToMove, + newStreamId: otherChannel.streamId, + propagateMode: PropagateMode.changeAll)); + check(store).getStreamTopics(channel.streamId).isNotNull().isEmpty(); + check(store).getStreamTopics(otherChannel.streamId).isNotNull().single + ..name.equals(eg.t('foo')) + ..maxId.equals(109); + checkNotifiedOnce(); + }); + + test('topic moved to new topic in the same stream', () async { + final messagesToMove = List.generate(10, (i) => + eg.streamMessage(id: 100 + i, stream: channel, topic: 'foo')); + await prepareWithMessages(messagesToMove, expectedTopics: [ + isStreamTopicsEntry('foo', 109), + ]); + + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: messagesToMove, + newTopicStr: 'bar', + propagateMode: PropagateMode.changeAll)); + check(store).getStreamTopics(channel.streamId).isNotNull().single + ..name.equals(eg.t('bar')) + ..maxId.equals(109); + checkNotifiedOnce(); + }); + + test('topic moved to known topic in the same stream', () async { + final messagesToMove = List.generate(10, (i) => + eg.streamMessage(id: 100 + i, stream: channel, topic: 'foo')); + await prepareWithMessages([ + ...messagesToMove, + eg.streamMessage(id: 1, stream: channel, topic: 'bar'), + ], expectedTopics: [ + isStreamTopicsEntry('foo', 109), + isStreamTopicsEntry('bar', 1), + ]); + + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: messagesToMove, + newTopicStr: 'bar', + propagateMode: PropagateMode.changeAll)); + check(store).getStreamTopics(channel.streamId).isNotNull().single + ..name.equals(eg.t('bar')) + ..maxId.equals(109); + checkNotifiedOnce(); + }); + }); + + test('message moved to new topic', () async { + final messageToMove = + eg.streamMessage(id: 101, stream: channel, topic: 'foo'); + await prepareWithMessages([ + messageToMove, + ], expectedTopics: [ + isStreamTopicsEntry('foo', 101), + ]); + + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: [messageToMove], + newTopicStr: 'bar', + propagateMode: PropagateMode.changeOne)); + check(store).getStreamTopics(channel.streamId).isNotNull().deepEquals([ + isStreamTopicsEntry('foo', 101), + isStreamTopicsEntry('bar', 101), + ]); + checkNotifiedOnce(); + }); + + test('message moved to known topic; moved message ID < maxId', () async { + final messageToMove = + eg.streamMessage(id: 100, stream: channel, topic: 'foo'); + await prepareWithMessages([ + messageToMove, + eg.streamMessage(id: 999, stream: channel, topic: 'bar'), + ], expectedTopics: [ + isStreamTopicsEntry('bar', 999), + isStreamTopicsEntry('foo', 100), + ]); + + // Message with ID 100 moved from "foo" to "bar", whose maxId was 999. + // We expect no updates to "bar"'s maxId, since the moved message + // has a lower ID. + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: [messageToMove], + newTopicStr: 'bar', + propagateMode: PropagateMode.changeOne)); + check(store).getStreamTopics(channel.streamId).isNotNull().deepEquals([ + isStreamTopicsEntry('bar', 999), + isStreamTopicsEntry('foo', 100), + ]); + checkNotNotified(); + }); + + test('message moved to known topic; moved message ID > maxId', () async { + final messageToMove = + eg.streamMessage(id: 999, stream: channel, topic: 'foo'); + await prepareWithMessages([ + messageToMove, + eg.streamMessage(id: 100, stream: channel, topic: 'bar'), + ], expectedTopics: [ + isStreamTopicsEntry('foo', 999), + isStreamTopicsEntry('bar', 100), + ]); + + // Message with ID 999 moved from "foo" to "bar", whose maxId was 100. + // We expect an update to "bar"'s maxId, since the moved message has + // a higher ID. + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: [messageToMove], + newTopicStr: 'bar', + propagateMode: PropagateMode.changeOne)); + check(store).getStreamTopics(channel.streamId).isNotNull().deepEquals([ + isStreamTopicsEntry('foo', 999), + isStreamTopicsEntry('bar', 999), + ]); + checkNotifiedOnce(); + }); + }); + }); + group('topic visibility', () { final stream1 = eg.stream(); final stream2 = eg.stream(); diff --git a/test/model/store_checks.dart b/test/model/store_checks.dart index dac078a434..579246b89e 100644 --- a/test/model/store_checks.dart +++ b/test/model/store_checks.dart @@ -2,6 +2,7 @@ import 'package:checks/checks.dart'; import 'package:zulip/api/core.dart'; import 'package:zulip/api/model/initial_snapshot.dart'; import 'package:zulip/api/model/model.dart'; +import 'package:zulip/api/route/channels.dart'; import 'package:zulip/model/autocomplete.dart'; import 'package:zulip/model/binding.dart'; import 'package:zulip/model/database.dart'; @@ -67,6 +68,7 @@ extension PerAccountStoreChecks on Subject { Subject> get streams => has((x) => x.streams, 'streams'); Subject> get streamsByName => has((x) => x.streamsByName, 'streamsByName'); Subject> get subscriptions => has((x) => x.subscriptions, 'subscriptions'); + Subject?> getStreamTopics(int streamId) => has((x) => x.getChannelTopics(streamId), 'getStreamTopics'); Subject> get messages => has((x) => x.messages, 'messages'); Subject get unreads => has((x) => x.unreads, 'unreads'); Subject get recentDmConversationsView => has((x) => x.recentDmConversationsView, 'recentDmConversationsView'); From 8bd4f4bd917ab9196b90f357c204f7ac18a6e976 Mon Sep 17 00:00:00 2001 From: Sayed Mahmood Sayedi Date: Mon, 3 Nov 2025 21:10:09 +0430 Subject: [PATCH 2/5] autocomplete test [nfc]: s/streams/topics in a topic test name --- test/model/autocomplete_test.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/model/autocomplete_test.dart b/test/model/autocomplete_test.dart index a560512553..6d825c49ff 100644 --- a/test/model/autocomplete_test.dart +++ b/test/model/autocomplete_test.dart @@ -1258,7 +1258,7 @@ void main() { .topic.equals(third.name); }); - test('TopicAutocompleteView updates results when streams are loaded', () async { + test('TopicAutocompleteView updates results when topics are loaded', () async { final store = eg.store(); final connection = store.connection as FakeApiConnection; connection.prepare(json: GetStreamTopicsResult( From 35589f1b1efdd790fed1b3856101cf66333f10c7 Mon Sep 17 00:00:00 2001 From: Sayed Mahmood Sayedi Date: Tue, 4 Nov 2025 23:42:29 +0430 Subject: [PATCH 3/5] autocomplete test [nfc]: Add and use isTopic condition --- test/model/autocomplete_test.dart | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/test/model/autocomplete_test.dart b/test/model/autocomplete_test.dart index 6d825c49ff..30190ecb63 100644 --- a/test/model/autocomplete_test.dart +++ b/test/model/autocomplete_test.dart @@ -1233,6 +1233,10 @@ void main() { doTest('a^bc^', TopicAutocompleteQuery('abc')); }); + Condition isTopic(TopicName topic) { + return (it) => it.isA().topic.equals(topic); + } + test('TopicAutocompleteView misc', () async { final store = eg.store(); final connection = store.connection as FakeApiConnection; @@ -1253,9 +1257,7 @@ void main() { await Future(() {}); await Future(() {}); check(done).isTrue(); - check(view.results).single - .isA() - .topic.equals(third.name); + check(view.results).single.which(isTopic(third.name)); }); test('TopicAutocompleteView updates results when topics are loaded', () async { From f7deae43c326eea69f26176803864ab2a81599b2 Mon Sep 17 00:00:00 2001 From: Sayed Mahmood Sayedi Date: Wed, 29 Oct 2025 01:18:53 +0430 Subject: [PATCH 4/5] autocomplete: Use topic data from store in topic autocomplete --- lib/model/autocomplete.dart | 7 ++---- test/model/autocomplete_test.dart | 42 +++++++++++++++++++++++++------ 2 files changed, 37 insertions(+), 12 deletions(-) diff --git a/lib/model/autocomplete.dart b/lib/model/autocomplete.dart index cf5c6b86e0..37d5db44db 100644 --- a/lib/model/autocomplete.dart +++ b/lib/model/autocomplete.dart @@ -6,7 +6,6 @@ import 'package:unorm_dart/unorm_dart.dart' as unorm; import '../api/model/events.dart'; import '../api/model/model.dart'; -import '../api/route/channels.dart'; import '../generated/l10n/zulip_localizations.dart'; import '../widgets/compose_box.dart'; import 'algorithms.dart'; @@ -1175,10 +1174,8 @@ class TopicAutocompleteView extends AutocompleteView _fetch() async { assert(!_isFetching); _isFetching = true; - final result = await getStreamTopics(store.connection, streamId: streamId, - allowEmptyTopicName: true, - ); - _topics = result.topics.map((e) => e.name); + await store.fetchTopics(streamId); + _topics = store.getChannelTopics(streamId)!.map((e) => e.name); _isFetching = false; return _startSearch(); } diff --git a/test/model/autocomplete_test.dart b/test/model/autocomplete_test.dart index 30190ecb63..7b3b39d143 100644 --- a/test/model/autocomplete_test.dart +++ b/test/model/autocomplete_test.dart @@ -1279,19 +1279,47 @@ void main() { check(done).isTrue(); }); - test('TopicAutocompleteView getStreamTopics request', () async { + test('TopicAutocompleteView fetch topics once for a channel', () async { final store = eg.store(); final connection = store.connection as FakeApiConnection; - connection.prepare(json: GetStreamTopicsResult( - topics: [eg.getStreamTopicsEntry(name: '')], - ).toJson()); - TopicAutocompleteView.init(store: store, streamId: 1000, - query: TopicAutocompleteQuery('foo')); - check(connection.lastRequest).isA() + final topic1 = eg.getStreamTopicsEntry(maxId: 20, name: 'server releases'); + final topic2 = eg.getStreamTopicsEntry(maxId: 10, name: 'mobile releases'); + + connection.prepare(json: GetStreamTopicsResult(topics: [topic1, topic2]).toJson()); + final view1 = TopicAutocompleteView.init(store: store, streamId: 1000, + query: TopicAutocompleteQuery('')); + bool done = false; + view1.addListener(() { done = true; }); + + check(connection.takeRequests()).last.isA() ..method.equals('GET') ..url.path.equals('/api/v1/users/me/1000/topics') ..url.queryParameters['allow_empty_topic_name'].equals('true'); + + await Future(() {}); + await Future(() {}); + check(done).isTrue(); + check(view1.results).deepEquals([isTopic(topic1.name), isTopic(topic2.name)]); + + view1.query = TopicAutocompleteQuery('server'); + await Future(() {}); + check(connection.takeRequests()).isEmpty(); + check(view1.results).single.which(isTopic(topic1.name)); + view1.dispose(); + + connection.prepare(json: GetStreamTopicsResult(topics: [topic1]).toJson()); + final view2 = TopicAutocompleteView.init(store: store, streamId: 1000, + query: TopicAutocompleteQuery('mobile')); + done = false; + view2.addListener(() { done = true; }); + + check(connection.takeRequests()).isEmpty(); + + await Future(() {}); + await Future(() {}); + check(done).isTrue(); + check(view2.results).single.which(isTopic(topic2.name)); }); group('TopicAutocompleteQuery.testTopic', () { From 02ed253554dde53bee80062761014a98253953ad Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Wed, 29 Oct 2025 01:05:46 +0430 Subject: [PATCH 5/5] topics: Keep topic-list page updated, by using topic data from store Fixes: #1499 Co-authored-by: Sayed Mahmood Sayedi --- lib/widgets/topic_list.dart | 39 +++++------ test/widgets/action_sheet_test.dart | 7 +- test/widgets/topic_list_test.dart | 104 ++++++++++++++++++++++++---- 3 files changed, 110 insertions(+), 40 deletions(-) diff --git a/lib/widgets/topic_list.dart b/lib/widgets/topic_list.dart index 846f2202f7..29e12c1cff 100644 --- a/lib/widgets/topic_list.dart +++ b/lib/widgets/topic_list.dart @@ -4,6 +4,7 @@ import '../api/model/model.dart'; import '../api/route/channels.dart'; import '../generated/l10n/zulip_localizations.dart'; import '../model/narrow.dart'; +import '../model/store.dart'; import '../model/unreads.dart'; import 'action_sheet.dart'; import 'app_bar.dart'; @@ -125,16 +126,13 @@ class _TopicList extends StatefulWidget { class _TopicListState extends State<_TopicList> with PerAccountStoreAwareStateMixin { Unreads? unreadsModel; - // TODO(#1499): store the results on [ChannelStore], and keep them - // up-to-date by handling events - List? lastFetchedTopics; @override void onNewStore() { unreadsModel?.removeListener(_modelChanged); final store = PerAccountStoreWidget.of(context); unreadsModel = store.unreads..addListener(_modelChanged); - _fetchTopics(); + _fetchTopics(store); } @override @@ -149,23 +147,22 @@ class _TopicListState extends State<_TopicList> with PerAccountStoreAwareStateMi }); } - void _fetchTopics() async { + void _fetchTopics(PerAccountStore store) 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 - final store = PerAccountStoreWidget.of(context); - final result = await getStreamTopics(store.connection, - streamId: widget.streamId, - allowEmptyTopicName: true); + await store.fetchTopics(widget.streamId); if (!mounted) return; setState(() { - lastFetchedTopics = result.topics; + // The actual state lives in the PerAccountStore. }); } @override Widget build(BuildContext context) { - if (lastFetchedTopics == null) { + final store = PerAccountStoreWidget.of(context); + final channelTopics = store.getChannelTopics(widget.streamId); + if (channelTopics == null) { return const Center(child: CircularProgressIndicator()); } @@ -173,7 +170,7 @@ class _TopicListState extends State<_TopicList> with PerAccountStoreAwareStateMi // This is adapted from parts of the build method on [_InboxPageState]. final topicItems = <_TopicItemData>[]; - for (final GetStreamTopicsEntry(:maxId, name: topic) in lastFetchedTopics!) { + for (final GetStreamTopicsEntry(:maxId, name: topic) in channelTopics) { final unreadMessageIds = unreadsModel!.streams[widget.streamId]?[topic] ?? []; final countInTopic = unreadMessageIds.length; @@ -183,17 +180,9 @@ class _TopicListState extends State<_TopicList> with PerAccountStoreAwareStateMi topic: topic, unreadCount: countInTopic, hasMention: hasMention, - // `lastFetchedTopics.maxId` can become outdated when a new message - // arrives or when there are message moves, until we re-fetch. - // TODO(#1499): track changes to this maxId: maxId, )); } - topicItems.sort((a, b) { - final aMaxId = a.maxId; - final bMaxId = b.maxId; - return bMaxId.compareTo(aMaxId); - }); return SafeArea( // Don't pad the bottom here; we want the list content to do that. @@ -235,6 +224,14 @@ class _TopicItem extends StatelessWidget { final store = PerAccountStoreWidget.of(context); final designVariables = DesignVariables.of(context); + // The message with `maxId` might not remain in `topic` since we last fetch + // the list of topics. Make sure we check for that before passing `maxId` + // to the topic action sheet. + // See also: [ChannelStore.getChannelTopics] + final message = store.messages[maxId]; + final isMaxIdInTopic = message is StreamMessage + && message.streamId == streamId + && message.topic.isSameAs(topic); final visibilityPolicy = store.topicVisibilityPolicy(streamId, topic); final double opacity; @@ -263,7 +260,7 @@ class _TopicItem extends StatelessWidget { onLongPress: () => showTopicActionSheet(context, channelId: streamId, topic: topic, - someMessageIdInTopic: maxId), + someMessageIdInTopic: isMaxIdInTopic ? maxId : null), splashFactory: NoSplash.splashFactory, child: ConstrainedBox( constraints: BoxConstraints(minHeight: 40), diff --git a/test/widgets/action_sheet_test.dart b/test/widgets/action_sheet_test.dart index c43c6daab7..781ed3afd4 100644 --- a/test/widgets/action_sheet_test.dart +++ b/test/widgets/action_sheet_test.dart @@ -222,12 +222,7 @@ void main() { channel ??= someChannel; connection.prepare(json: eg.newestGetMessagesResult( - foundOldest: true, messages: []).toJson()); - if (narrow case ChannelNarrow()) { - // We auto-focus the topic input when there are no messages; - // this is for topic autocomplete. - connection.prepare(json: GetStreamTopicsResult(topics: []).toJson()); - } + foundOldest: true, messages: [eg.streamMessage(stream: channel)]).toJson()); await tester.pumpWidget(TestZulipApp( accountId: eg.selfAccount.id, child: MessageListPage( diff --git a/test/widgets/topic_list_test.dart b/test/widgets/topic_list_test.dart index 1cd9c4bb26..f0be2f671f 100644 --- a/test/widgets/topic_list_test.dart +++ b/test/widgets/topic_list_test.dart @@ -14,8 +14,10 @@ import 'package:zulip/widgets/message_list.dart'; import 'package:zulip/widgets/topic_list.dart'; import '../api/fake_api.dart'; +import '../api/route/route_checks.dart'; import '../example_data.dart' as eg; import '../model/binding.dart'; +import '../model/store_checks.dart'; import '../model/test_store.dart'; import '../stdlib_checks.dart'; import 'test_app.dart'; @@ -124,7 +126,7 @@ void main() { check(find.byType(CircularProgressIndicator)).findsNothing(); }); - testWidgets('fetch again when navigating away and back', (tester) async { + testWidgets("dont't fetch again when navigating away and back", (tester) async { addTearDown(testBinding.reset); await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); final store = await testBinding.globalStore.perAccount(eg.selfAccount.id); @@ -145,20 +147,28 @@ void main() { await tester.tap(find.byIcon(ZulipIcons.topics)); await tester.pump(); await tester.pump(Duration.zero); + check(connection.takeRequests()) + ..length.equals(2) // one for the messages request, another for the topics + ..last.which((last) => last + .isA() + ..method.equals('GET') + ..url.path.equals('/api/v1/users/me/${channel.streamId}/topics')); check(find.text('topic A')).findsOne(); // … go back to the message list page… await tester.pageBack(); await tester.pump(); - // … then back to the topic-list page, expecting to fetch again. + // … then back to the topic-list page, expecting not to fetch again but + // use existing data which is kept up-to-date anyways. connection.prepare(json: GetStreamTopicsResult( topics: [eg.getStreamTopicsEntry(name: 'topic B')]).toJson()); await tester.tap(find.byIcon(ZulipIcons.topics)); await tester.pump(); await tester.pump(Duration.zero); - check(find.text('topic A')).findsNothing(); - check(find.text('topic B')).findsOne(); + check(connection.takeRequests()).isEmpty(); + check(find.text('topic B')).findsNothing(); + check(find.text('topic A')).findsOne(); }); Finder topicItemFinder = find.descendant( @@ -169,6 +179,18 @@ void main() { of: topicItemFinder.at(index), matching: finder); + testWidgets('sort topics by maxId', (tester) async { + await prepare(tester, topics: [ + eg.getStreamTopicsEntry(name: 'A', maxId: 3), + eg.getStreamTopicsEntry(name: 'B', maxId: 2), + eg.getStreamTopicsEntry(name: 'C', maxId: 4), + ]); + + check(findInTopicItemAt(0, find.text('C'))).findsOne(); + check(findInTopicItemAt(1, find.text('A'))).findsOne(); + check(findInTopicItemAt(2, find.text('B'))).findsOne(); + }); + testWidgets('show topic action sheet', (tester) async { final channel = eg.stream(); await prepare(tester, channel: channel, @@ -190,16 +212,72 @@ void main() { }); }); - testWidgets('sort topics by maxId', (tester) async { - await prepare(tester, topics: [ - eg.getStreamTopicsEntry(name: 'A', maxId: 3), - eg.getStreamTopicsEntry(name: 'B', maxId: 2), - eg.getStreamTopicsEntry(name: 'C', maxId: 4), - ]); + 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'); + await prepare(tester, channel: channel, + topics: [eg.getStreamTopicsEntry(name: 'foo', maxId: 123)], + messages: [message]); + check(store).getStreamTopics(channel.streamId).isNotNull().single + .maxId.equals(123); + check(topicItemFinder).findsOne(); + + // Before the move, "foo"'s maxId is known to be accurate. This makes + // topic actions that require `someMessageIdInTopic` available. + await tester.longPress(find.text('foo')); + await tester.pump(Duration(milliseconds: 150)); // bottom-sheet animation + check(find.text('Mark as resolved')).findsOne(); + await tester.tap(find.text('Cancel')); - check(findInTopicItemAt(0, find.text('C'))).findsOne(); - check(findInTopicItemAt(1, find.text('A'))).findsOne(); - check(findInTopicItemAt(2, find.text('B'))).findsOne(); + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: [message], + newTopicStr: 'bar')); + await tester.pump(); + check(topicItemFinder).findsExactly(2); + + // After the move, the message with maxId moved away from "foo". The topic + // actions that require `someMessageIdInTopic` is no longer available. + await tester.longPress(find.text('foo')); + await tester.pump(Duration(milliseconds: 150)); // bottom-sheet animation + check(find.text('Mark as resolved')).findsNothing(); + await tester.tap(find.text('Cancel')); + await tester.pump(); + + // Topic actions that require `someMessageIdInTopic` is available + // for "bar", the new topic that the message moved to. + await tester.longPress(find.text('bar')); + await tester.pump(Duration(milliseconds: 150)); // bottom-sheet animation + check(find.text('Mark as resolved')).findsOne(); + }); + + // event handling is more thoroughly tested in test/model/channel_test.dart + testWidgets('smoke resolve topic from topic action sheet', (tester) async { + final channel = eg.stream(); + final messages = List.generate(10, (i) => + eg.streamMessage(id: 100+i, stream: channel, topic: 'foo')); + + await prepare(tester, channel: channel, + topics: [eg.getStreamTopicsEntry(maxId: messages.last.id, name: 'foo')], + messages: messages); + await tester.longPress(topicItemFinder); + await tester.pump(Duration(milliseconds: 150)); // bottom-sheet animation + check(findInTopicItemAt(0, find.byIcon(ZulipIcons.check))).findsNothing(); + check(find.descendant(of: topicItemFinder, + matching: find.text('foo'))).findsOne(); + + connection.prepare(json: {}); + await tester.tap(find.text('Mark as resolved')); + await tester.pump(); + await tester.pump(Duration.zero); + + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: messages, + newTopic: eg.t('foo').resolve(), + propagateMode: PropagateMode.changeAll)); + await tester.pump(); + check(findInTopicItemAt(0, find.byIcon(ZulipIcons.check))).findsOne(); + check(find.descendant(of: topicItemFinder, + matching: find.text('foo'))).findsOne(); }); testWidgets('resolved and unresolved topics', (tester) async {