From fee6de6e4df559700467d610ac25d18082c8a18d Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 16 Oct 2025 12:21:06 -0700 Subject: [PATCH 1/7] message [nfc]: Fix a reference in a comment Thanks to Greg for pointing this out: https://github.com/zulip/zulip-flutter/pull/1912#discussion_r2434283016 --- lib/model/message.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/model/message.dart b/lib/model/message.dart index 90a0c28840..6f82d6da6e 100644 --- a/lib/model/message.dart +++ b/lib/model/message.dart @@ -512,7 +512,7 @@ class MessageStoreImpl extends HasChannelStore with MessageStore, _OutboxMessage void handleMessageEvent(MessageEvent event) { // If the message is one we already know about (from a fetch), // clobber it with the one from the event system. - // See [fetchedMessages] for reasoning. + // See [reconcileMessages] for reasoning. messages[event.message.id] = event.message; _handleMessageEventOutbox(event); From d6f244177178598d4ac151fd50dc9b566a5ff54f Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 10 Oct 2025 14:23:34 -0700 Subject: [PATCH 2/7] compose_box test: Switch to using subscribed channels, where boring It's way more common to be using the app with subscribed channels than unsubscribed channels, so we might as well test with subscribed channels except where we specifically want to check behavior for unsubscribed channels. --- test/widgets/compose_box_test.dart | 73 ++++++++++++++++++------------ 1 file changed, 45 insertions(+), 28 deletions(-) diff --git a/test/widgets/compose_box_test.dart b/test/widgets/compose_box_test.dart index 15a0a7f81e..ef317277ae 100644 --- a/test/widgets/compose_box_test.dart +++ b/test/widgets/compose_box_test.dart @@ -160,7 +160,7 @@ void main() { final channel = eg.stream(); await prepareComposeBox(tester, narrow: ChannelNarrow(channel.streamId), - streams: [channel], + subscriptions: [eg.subscription(channel)], messages: [eg.streamMessage(stream: channel)]); check(controller).isA() ..topicFocusNode.hasFocus.isFalse() @@ -171,7 +171,7 @@ void main() { final channel = eg.stream(); await prepareComposeBox(tester, narrow: ChannelNarrow(channel.streamId), - streams: [channel], + subscriptions: [eg.subscription(channel)], messages: []); check(controller).isA() .topicFocusNode.hasFocus.isTrue(); @@ -181,7 +181,7 @@ void main() { final channel = eg.stream(); await prepareComposeBox(tester, narrow: TopicNarrow(channel.streamId, eg.t('topic')), - streams: [channel], + subscriptions: [eg.subscription(channel)], messages: [eg.streamMessage(stream: channel, topic: 'topic')]); check(controller).isNotNull().contentFocusNode.hasFocus.isFalse(); }); @@ -190,7 +190,7 @@ void main() { final channel = eg.stream(); await prepareComposeBox(tester, narrow: TopicNarrow(channel.streamId, eg.t('topic')), - streams: [channel], + subscriptions: [eg.subscription(channel)], messages: []); check(controller).isNotNull().contentFocusNode.hasFocus.isTrue(); }); @@ -379,7 +379,7 @@ void main() { addTearDown(MessageStoreImpl.debugReset); final narrow = ChannelNarrow(channel.streamId); - await prepareComposeBox(tester, narrow: narrow, streams: [channel]); + await prepareComposeBox(tester, narrow: narrow, subscriptions: [eg.subscription(channel)]); await enterTopic(tester, narrow: narrow, topic: 'some topic'); await enterContent(tester, content); } @@ -418,7 +418,7 @@ void main() { addTearDown(MessageStoreImpl.debugReset); final narrow = ChannelNarrow(channel.streamId); - await prepareComposeBox(tester, narrow: narrow, streams: [channel]); + await prepareComposeBox(tester, narrow: narrow, subscriptions: [eg.subscription(channel)]); await enterTopic(tester, narrow: narrow, topic: topic); await enterContent(tester, 'some content'); } @@ -462,7 +462,7 @@ void main() { await prepareComposeBox(tester, narrow: narrow, otherUsers: [eg.otherUser, eg.thirdUser], - streams: [channel], + subscriptions: [eg.subscription(channel)], mandatoryTopics: mandatoryTopics, zulipFeatureLevel: zulipFeatureLevel); } @@ -736,14 +736,14 @@ void main() { testWidgets('_StreamComposeBox', (tester) async { final channel = eg.stream(); await prepareComposeBox(tester, - narrow: ChannelNarrow(channel.streamId), streams: [channel]); + narrow: ChannelNarrow(channel.streamId), subscriptions: [eg.subscription(channel)]); checkComposeBoxTextFields(tester, expectTopicTextField: true); }); testWidgets('_FixedDestinationComposeBox', (tester) async { final channel = eg.stream(); await prepareComposeBox(tester, - narrow: eg.topicNarrow(channel.streamId, 'topic'), streams: [channel]); + narrow: eg.topicNarrow(channel.streamId, 'topic'), subscriptions: [eg.subscription(channel)]); checkComposeBoxTextFields(tester, expectTopicTextField: false); }); }); @@ -762,7 +762,8 @@ void main() { } testWidgets('smoke TopicNarrow', (tester) async { - await prepareComposeBox(tester, narrow: narrow, streams: [channel]); + await prepareComposeBox(tester, + narrow: narrow, subscriptions: [eg.subscription(channel)]); await checkStartTyping(tester, narrow); @@ -786,7 +787,8 @@ void main() { testWidgets('smoke ChannelNarrow', (tester) async { final narrow = ChannelNarrow(channel.streamId); final destinationNarrow = eg.topicNarrow(narrow.streamId, 'test topic'); - await prepareComposeBox(tester, narrow: narrow, streams: [channel]); + await prepareComposeBox(tester, + narrow: narrow, subscriptions: [eg.subscription(channel)]); await enterTopic(tester, narrow: narrow, topic: 'test topic'); await checkStartTyping(tester, destinationNarrow); @@ -797,7 +799,8 @@ void main() { }); testWidgets('clearing text sends a "typing stopped" notice', (tester) async { - await prepareComposeBox(tester, narrow: narrow, streams: [channel]); + await prepareComposeBox(tester, + narrow: narrow, subscriptions: [eg.subscription(channel)]); await checkStartTyping(tester, narrow); @@ -809,7 +812,8 @@ void main() { testWidgets('hitting send button sends a "typing stopped" notice', (tester) async { MessageStoreImpl.debugOutboxEnable = false; addTearDown(MessageStoreImpl.debugReset); - await prepareComposeBox(tester, narrow: narrow, streams: [channel]); + await prepareComposeBox(tester, + narrow: narrow, subscriptions: [eg.subscription(channel)]); await checkStartTyping(tester, narrow); @@ -855,7 +859,8 @@ void main() { testWidgets('for content input, unfocusing sends a "typing stopped" notice', (tester) async { final narrow = ChannelNarrow(channel.streamId); final destinationNarrow = eg.topicNarrow(narrow.streamId, 'test topic'); - await prepareComposeBox(tester, narrow: narrow, streams: [channel]); + await prepareComposeBox(tester, + narrow: narrow, subscriptions: [eg.subscription(channel)]); await enterTopic(tester, narrow: narrow, topic: 'test topic'); await checkStartTyping(tester, destinationNarrow); @@ -867,7 +872,8 @@ void main() { }); testWidgets('selection change sends a "typing started" notice', (tester) async { - await prepareComposeBox(tester, narrow: narrow, streams: [channel]); + await prepareComposeBox(tester, + narrow: narrow, subscriptions: [eg.subscription(channel)]); await checkStartTyping(tester, narrow); @@ -887,7 +893,8 @@ void main() { }); testWidgets('unfocusing app sends a "typing stopped" notice', (tester) async { - await prepareComposeBox(tester, narrow: narrow, streams: [channel]); + await prepareComposeBox(tester, + narrow: narrow, subscriptions: [eg.subscription(channel)]); await checkStartTyping(tester, narrow); @@ -919,8 +926,9 @@ void main() { addTearDown(MessageStoreImpl.debugReset); final zulipLocalizations = GlobalLocalizations.zulipLocalizations; - await prepareComposeBox(tester, narrow: eg.topicNarrow(123, 'some topic'), - streams: [eg.stream(streamId: 123)]); + await prepareComposeBox(tester, + narrow: eg.topicNarrow(123, 'some topic'), + subscriptions: [eg.subscription(eg.stream(streamId: 123))]); await enterContent(tester, 'hello world'); @@ -977,7 +985,7 @@ void main() { channel = eg.stream(); final narrow = ChannelNarrow(channel.streamId); await prepareComposeBox(tester, - narrow: narrow, streams: [channel], + narrow: narrow, subscriptions: [eg.subscription(channel)], mandatoryTopics: mandatoryTopics, zulipFeatureLevel: zulipFeatureLevel); @@ -1057,7 +1065,8 @@ void main() { final channel = eg.stream(); final narrow = ChannelNarrow(channel.streamId); - await prepareComposeBox(tester, narrow: narrow, streams: [channel]); + await prepareComposeBox(tester, + narrow: narrow, subscriptions: [eg.subscription(channel)]); // (When we check that the send button looks disabled, it should be because // the file is uploading, not a pre-existing reason.) @@ -1175,7 +1184,8 @@ void main() { final channel = eg.stream(); final narrow = eg.topicNarrow(channel.streamId, 'a topic'); - await prepareComposeBox(tester, narrow: narrow, streams: [channel]); + await prepareComposeBox(tester, + narrow: narrow, subscriptions: [eg.subscription(channel)]); testBinding.pickFilesResult = FilePickerResult([PlatformFile( readStream: Stream.fromIterable(['asdf'.codeUnits]), @@ -1631,7 +1641,8 @@ void main() { } testWidgets('normal text scale factor', (tester) async { - await prepareComposeBox(tester, narrow: narrow, streams: [stream]); + await prepareComposeBox(tester, + narrow: narrow, subscriptions: [eg.subscription(stream)]); await checkContentInputMaxHeight(tester, maxHeight: verticalPadding + 170, maxVisibleLines: 8); @@ -1640,7 +1651,8 @@ void main() { testWidgets('lower text scale factor', (tester) async { tester.platformDispatcher.textScaleFactorTestValue = 0.8; addTearDown(tester.platformDispatcher.clearTextScaleFactorTestValue); - await prepareComposeBox(tester, narrow: narrow, streams: [stream]); + await prepareComposeBox(tester, + narrow: narrow, subscriptions: [eg.subscription(stream)]); await checkContentInputMaxHeight(tester, maxHeight: verticalPadding + 170 * 0.8, maxVisibleLines: 8); }); @@ -1648,7 +1660,8 @@ void main() { testWidgets('higher text scale factor', (tester) async { tester.platformDispatcher.textScaleFactorTestValue = 1.5; addTearDown(tester.platformDispatcher.clearTextScaleFactorTestValue); - await prepareComposeBox(tester, narrow: narrow, streams: [stream]); + await prepareComposeBox(tester, + narrow: narrow, subscriptions: [eg.subscription(stream)]); await checkContentInputMaxHeight(tester, maxHeight: verticalPadding + 170 * 1.5, maxVisibleLines: 8); }); @@ -1656,7 +1669,8 @@ void main() { testWidgets('higher text scale factor exceeding threshold', (tester) async { tester.platformDispatcher.textScaleFactorTestValue = 2; addTearDown(tester.platformDispatcher.clearTextScaleFactorTestValue); - await prepareComposeBox(tester, narrow: narrow, streams: [stream]); + await prepareComposeBox(tester, + narrow: narrow, subscriptions: [eg.subscription(stream)]); await checkContentInputMaxHeight(tester, maxHeight: verticalPadding + 170 * 1.5, maxVisibleLines: 6); }); @@ -1671,7 +1685,8 @@ void main() { final channel = eg.stream(); await prepareComposeBox(tester, - narrow: eg.topicNarrow(channel.streamId, 'topic'), streams: [channel]); + narrow: eg.topicNarrow(channel.streamId, 'topic'), + subscriptions: [eg.subscription(channel)]); await enterContent(tester, 'some content'); checkContentInputValue(tester, 'some content'); @@ -1769,7 +1784,9 @@ void main() { TypingNotifier.debugEnable = false; addTearDown(TypingNotifier.debugReset); await prepareComposeBox(tester, - narrow: narrow, streams: [channel], otherUsers: otherUsers); + narrow: narrow, + subscriptions: [eg.subscription(channel)], + otherUsers: otherUsers); if (narrow is ChannelNarrow) { connection.prepare(json: GetStreamTopicsResult(topics: []).toJson()); @@ -1937,7 +1954,7 @@ void main() { addTearDown(MessageStoreImpl.debugReset); await prepareComposeBox(tester, narrow: narrow, - streams: [channel]); + subscriptions: [eg.subscription(channel)]); await store.addMessages([message, dmMessage]); await tester.pump(); // message list updates } From f89ce2e4e9b7d2403a883975fd1de75734be230e Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 9 Oct 2025 18:21:48 -0700 Subject: [PATCH 3/7] msglist test: Switch to using subscribed channels, where boring Like we did in the compose-box tests in the previous commit. --- test/widgets/message_list_test.dart | 49 +++++++++++++++++------------ 1 file changed, 29 insertions(+), 20 deletions(-) diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index acdde3fd2c..240285a222 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -154,7 +154,7 @@ void main() { testWidgets('MessageListPageState.narrow', (tester) async { final stream = eg.stream(); await setupMessageListPage(tester, narrow: ChannelNarrow(stream.streamId), - streams: [stream], + subscriptions: [eg.subscription(stream)], messages: [eg.streamMessage(stream: stream, content: "

a message

")]); final state = MessageListPage.ancestorOf(tester.element(find.text("a message"))); check(state.narrow).equals(ChannelNarrow(stream.streamId)); @@ -167,7 +167,7 @@ void main() { final topic = eg.defaultRealmEmptyTopicDisplayName; final topicNarrow = eg.topicNarrow(stream.streamId, topic); await setupMessageListPage(tester, narrow: topicNarrow, - streams: [stream], + subscriptions: [eg.subscription(stream)], messages: [eg.streamMessage(stream: stream, topic: topic, content: "

a message

")]); final state = MessageListPage.ancestorOf(tester.element(find.text("a message"))); // The page's narrow has been updated; the topic is "", not "general chat". @@ -177,7 +177,7 @@ void main() { testWidgets('composeBoxState finds compose box', (tester) async { final stream = eg.stream(); await setupMessageListPage(tester, narrow: ChannelNarrow(stream.streamId), - streams: [stream], + subscriptions: [eg.subscription(stream)], messages: [eg.streamMessage(stream: stream, content: "

a message

")]); final state = MessageListPage.ancestorOf(tester.element(find.text("a message"))); check(state.composeBoxState).isNotNull(); @@ -238,7 +238,7 @@ void main() { final channel = eg.stream(); await setupMessageListPage(tester, narrow: eg.topicNarrow(channel.streamId, ''), - streams: [channel], + subscriptions: [eg.subscription(channel)], messageCount: 1); checkAppBarChannelTopic( channel.name, eg.defaultRealmEmptyTopicDisplayName); @@ -281,7 +281,7 @@ void main() { final channel = eg.stream(); await setupMessageListPage(tester, narrow: eg.topicNarrow(channel.streamId, 'hi'), navObservers: [navObserver], - streams: [channel], messageCount: 1); + subscriptions: [eg.subscription(channel)], messageCount: 1); // Clear out initial route. assert(pushedRoutes.length == 1); @@ -298,7 +298,7 @@ void main() { final channel = eg.stream(name: 'channel foo'); await setupMessageListPage(tester, narrow: eg.topicNarrow(channel.streamId, 'topic foo'), - streams: [channel], + subscriptions: [eg.subscription(channel)], messages: [eg.streamMessage(stream: channel, topic: 'topic foo')]); connection.prepare(json: GetStreamTopicsResult(topics: [ @@ -333,7 +333,7 @@ void main() { final channel = eg.stream(name: 'channel foo'); await setupMessageListPage(tester, narrow: ChannelNarrow(channel.streamId), - streams: [channel], + subscriptions: [eg.subscription(channel)], messages: [eg.streamMessage(stream: channel, topic: 'topic foo')]); connection.prepare(json: GetStreamTopicsResult(topics: [ @@ -390,7 +390,7 @@ void main() { final channel = eg.stream(); await setupMessageListPage(tester, narrow: TopicNarrow(channel.streamId, eg.t('topic')), - streams: [channel], + subscriptions: [eg.subscription(channel)], messages: []); check(findPlaceholder).findsOne(); @@ -488,9 +488,11 @@ void main() { group('topic permalink', () { final someStream = eg.stream(); + final someSubscription = eg.subscription(someStream); const someTopic = 'some topic'; final otherStream = eg.stream(); + final otherSubscription = eg.subscription(otherStream); const otherTopic = 'other topic'; testWidgets('with message move', (tester) async { @@ -499,7 +501,7 @@ void main() { narrow: narrow, // server sends the /with/ message in its current, different location messages: [eg.streamMessage(id: 1, stream: otherStream, topic: otherTopic)], - streams: [someStream, otherStream], + subscriptions: [someSubscription, otherSubscription], skipPumpAndSettle: true); await tester.pump(); // global store loaded await tester.pump(); // per-account store loaded @@ -533,7 +535,7 @@ void main() { narrow: narrow, // server sends the /with/ message in its current, different location messages: [eg.streamMessage(id: 1, stream: someStream, topic: someTopic)], - streams: [someStream], + subscriptions: [someSubscription], skipPumpAndSettle: true); await tester.pump(); // global store loaded await tester.pump(); // per-account store loaded @@ -1182,7 +1184,9 @@ void main() { group('Update Narrow on message move', () { const topic = 'foo'; final channel = eg.stream(); + final subscription = eg.subscription(channel); final otherChannel = eg.stream(); + final otherSubscription = eg.subscription(otherChannel); final narrow = eg.topicNarrow(channel.streamId, topic); void prepareGetMessageResponse(List messages) { @@ -1200,7 +1204,10 @@ void main() { testWidgets('compose box send message after move', (tester) async { final message = eg.streamMessage(stream: channel, topic: topic, content: 'Message to move'); - await setupMessageListPage(tester, narrow: narrow, messages: [message], streams: [channel, otherChannel]); + await setupMessageListPage(tester, + narrow: narrow, + messages: [message], + subscriptions: [subscription, otherSubscription]); final channelContentInputFinder = find.descendant( of: find.byType(ComposeAutocomplete), @@ -1240,7 +1247,8 @@ void main() { testWidgets('Move to narrow with existing messages', (tester) async { final message = eg.streamMessage(stream: channel, topic: topic, content: 'Message to move'); - await setupMessageListPage(tester, narrow: narrow, messages: [message], streams: [channel]); + await setupMessageListPage(tester, + narrow: narrow, messages: [message], subscriptions: [subscription]); check(find.textContaining('Existing message').evaluate()).length.equals(0); check(find.textContaining('Message to move').evaluate()).length.equals(1); @@ -1256,7 +1264,8 @@ void main() { testWidgets('show new topic in TopicNarrow after move', (tester) async { final message = eg.streamMessage(stream: channel, topic: topic, content: 'Message to move'); - await setupMessageListPage(tester, narrow: narrow, messages: [message], streams: [channel]); + await setupMessageListPage(tester, + narrow: narrow, messages: [message], subscriptions: [subscription]); prepareGetMessageResponse([message]); await handleMessageMoveEvent([message], 'new topic'); @@ -1315,7 +1324,7 @@ void main() { testWidgets('do not show channel name in ChannelNarrow', (tester) async { await setupMessageListPage(tester, narrow: ChannelNarrow(stream.streamId), - messages: [message], streams: [stream]); + messages: [message], subscriptions: [eg.subscription(stream)]); await tester.pump(); check(findInMessageList('stream name')).length.equals(0); check(findInMessageList('topic name')).length.equals(1); @@ -1324,7 +1333,7 @@ void main() { testWidgets('do not show stream name in TopicNarrow', (tester) async { await setupMessageListPage(tester, narrow: TopicNarrow.ofMessage(message), - messages: [message], streams: [stream]); + messages: [message], subscriptions: [eg.subscription(stream)]); await tester.pump(); check(findInMessageList('stream name')).length.equals(0); check(findInMessageList('topic name')).length.equals(1); @@ -1505,7 +1514,7 @@ void main() { final message = eg.streamMessage(stream: channel, topic: 'topic name'); await setupMessageListPage(tester, narrow: ChannelNarrow(channel.streamId), - streams: [channel], + subscriptions: [eg.subscription(channel)], messages: [message], navObservers: [navObserver]); @@ -1531,7 +1540,7 @@ void main() { final message = eg.streamMessage(stream: channel, topic: 'topic name'); await setupMessageListPage(tester, narrow: TopicNarrow.ofMessage(message), - streams: [channel], + subscriptions: [eg.subscription(channel)], messages: [message], navObservers: [navObserver]); @@ -2136,7 +2145,7 @@ void main() { testWidgets('hidden -> waiting', (tester) async { await setupMessageListPage(tester, - narrow: topicNarrow, streams: [stream], + narrow: topicNarrow, subscriptions: [eg.subscription(stream)], messages: []); await sendMessageAndSucceed(tester); @@ -2152,7 +2161,7 @@ void main() { testWidgets('hidden -> failed, tap to restore message', (tester) async { await setupMessageListPage(tester, - narrow: topicNarrow, streams: [stream], + narrow: topicNarrow, subscriptions: [eg.subscription(stream)], messages: []); // Send a message and fail. Dismiss the error dialog as it pops up. await sendMessageAndFail(tester); @@ -2200,7 +2209,7 @@ void main() { testWidgets('waiting -> waitPeriodExpired, tap to restore message', (tester) async { await setupMessageListPage(tester, - narrow: topicNarrow, streams: [stream], + narrow: topicNarrow, subscriptions: [eg.subscription(stream)], messages: []); await sendMessageAndFail(tester, delay: kSendMessageOfferRestoreWaitPeriod + const Duration(seconds: 1)); From 0bacdbceb8e0d2fb54162cd9731839c7d558aa35 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 10 Oct 2025 15:33:51 -0700 Subject: [PATCH 4/7] message test [nfc]: Add conditionIdentical helper, for reconcileMessages --- test/model/message_test.dart | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/test/model/message_test.dart b/test/model/message_test.dart index 4eccf75112..5bc251d41c 100644 --- a/test/model/message_test.dart +++ b/test/model/message_test.dart @@ -498,6 +498,9 @@ void main() { }); group('reconcileMessages', () { + Condition conditionIdentical(T element) => + (it) => it.identicalTo(element); + test('from empty', () async { await prepare(); check(store.messages).isEmpty(); @@ -507,8 +510,7 @@ void main() { final messages = [message1, message2, message3]; store.reconcileMessages(messages); check(messages).deepEquals( - [message1, message2, message3] - .map((m) => (Subject it) => it.identicalTo(m))); + [message1, message2, message3].map(conditionIdentical)); check(store.messages).deepEquals({ for (final m in messages) m.id: m, }); @@ -524,8 +526,7 @@ void main() { final newMessage = eg.streamMessage(); store.reconcileMessages([newMessage]); check(messages).deepEquals( - [message1, message2, message3] - .map((m) => (Subject it) => it.identicalTo(m))); + [message1, message2, message3].map(conditionIdentical)); check(store.messages).deepEquals({ for (final m in messages) m.id: m, newMessage.id: newMessage, @@ -540,7 +541,9 @@ void main() { final newMessage = eg.streamMessage(id: 1, content: '

bar

'); final messages = [newMessage]; store.reconcileMessages(messages); - check(messages).single.identicalTo(message); + check(messages).deepEquals( + // (We'll check more messages in an upcoming commit.) + [message].map(conditionIdentical)); check(store.messages).deepEquals({1: message}); }); From ab5458a00692668869f9f4f564ed43aec532c1af Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 17 Oct 2025 13:28:04 -0700 Subject: [PATCH 5/7] message [nfc]: Pull out _stripMatchFields helper for reconcileMessages --- lib/model/message.dart | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/model/message.dart b/lib/model/message.dart index 6f82d6da6e..7c204d1cce 100644 --- a/lib/model/message.dart +++ b/lib/model/message.dart @@ -426,13 +426,17 @@ class MessageStoreImpl extends HasChannelStore with MessageStore, _OutboxMessage for (int i = 0; i < messages.length; i++) { final message = messages[i]; messages[i] = this.messages.putIfAbsent(message.id, () { - message.matchContent = null; - message.matchTopic = null; - return message; + return _stripMatchFields(message); }); } } + Message _stripMatchFields(Message message) { + message.matchContent = null; + message.matchTopic = null; + return message; + } + @override bool? getEditMessageErrorStatus(int messageId) { assert(!_disposed); From 5469cbe2b50b3681dd7b095d70169540c8269fbe Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 17 Oct 2025 13:34:57 -0700 Subject: [PATCH 6/7] message [nfc]: Pull out some helpers for reconcileMessages; add TODO(#1798) --- lib/model/message.dart | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/lib/model/message.dart b/lib/model/message.dart index 7c204d1cce..9d4558b408 100644 --- a/lib/model/message.dart +++ b/lib/model/message.dart @@ -408,7 +408,20 @@ class MessageStoreImpl extends HasChannelStore with MessageStore, _OutboxMessage void reconcileMessages(List messages) { assert(!_disposed); - // What to do when some of the just-fetched messages are already known? + for (int i = 0; i < messages.length; i++) { + final message = messages[i]; + messages[i] = this.messages.update(message.id, + ifAbsent: () => _reconcileUnrecognizedMessage(message), + (current) => _reconcileRecognizedMessage(current, message)); + } + } + + Message _reconcileUnrecognizedMessage(Message incoming) { + return _stripMatchFields(incoming); + } + + Message _reconcileRecognizedMessage(Message current, Message incoming) { + // This just-fetched message is one we already know about. // This is common and normal: in particular it happens when one message list // overlaps another, e.g. a stream and a topic within it. // @@ -423,12 +436,8 @@ class MessageStoreImpl extends HasChannelStore with MessageStore, _OutboxMessage // we won't hear those events again; the only way to wind up with an // updated message is to use the version we have, that already reflects // those events' changes. So we always stick with the version we have. - for (int i = 0; i < messages.length; i++) { - final message = messages[i]; - messages[i] = this.messages.putIfAbsent(message.id, () { - return _stripMatchFields(message); - }); - } + // TODO(#1798) consider unsubscribed channels + return current; } Message _stripMatchFields(Message message) { From 2ab661c8b1a0f529cdbbbea94142c77ff08549ef Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 9 Oct 2025 18:05:03 -0700 Subject: [PATCH 7/7] message: Consider unsubscribed/unknown channels in reconcileMessages This fixes the "fourth buggy behavior" in #1798: https://github.com/zulip/zulip-flutter/issues/1798#issuecomment-3387895414 Fixes-partly: #1798 --- lib/model/message.dart | 110 ++++++++++++++++++++++--- lib/model/store.dart | 6 ++ test/model/message_test.dart | 154 +++++++++++++++++++++++++++++++---- 3 files changed, 242 insertions(+), 28 deletions(-) diff --git a/lib/model/message.dart b/lib/model/message.dart index 9d4558b408..936bc3bb1f 100644 --- a/lib/model/message.dart +++ b/lib/model/message.dart @@ -417,6 +417,14 @@ class MessageStoreImpl extends HasChannelStore with MessageStore, _OutboxMessage } Message _reconcileUnrecognizedMessage(Message incoming) { + if ( + incoming is StreamMessage + && subscriptions[incoming.streamId] == null + ) { + // The message is in an unsubscribed channel. It might grow stale; + // add it to _maybeStaleChannelMessages. + _maybeStaleChannelMessages.add(incoming.id); + } return _stripMatchFields(incoming); } @@ -426,20 +434,57 @@ class MessageStoreImpl extends HasChannelStore with MessageStore, _OutboxMessage // overlaps another, e.g. a stream and a topic within it. // // Most often, the just-fetched message will look just like the one we - // already have. But they can differ: message fetching happens out of band - // from the event queue, so there's inherently a race. - // - // If the fetched message reflects changes we haven't yet heard from the - // event queue, then it doesn't much matter which version we use: we'll - // soon get the corresponding events and apply the changes anyway. - // But if it lacks changes we've already heard from the event queue, then - // we won't hear those events again; the only way to wind up with an - // updated message is to use the version we have, that already reflects - // those events' changes. So we always stick with the version we have. - // TODO(#1798) consider unsubscribed channels - return current; + // already have. But not always, and we can choose intelligently whether + // to keep the stored version or clobber it with the incoming one. + + bool currentIsMaybeStale = false; + if (incoming is StreamMessage) { + if (subscriptions[incoming.streamId] != null) { + // The incoming version won't grow stale; it's in a subscribed channel. + // Remove it from _maybeStaleChannelMessages if it was there. + currentIsMaybeStale = _maybeStaleChannelMessages.remove(incoming.id); + } else { + assert(_maybeStaleChannelMessages.contains(incoming.id)); + currentIsMaybeStale = true; + } + } + + if (currentIsMaybeStale) { + // The event queue is unreliable for this message; the message was in an + // unsubscribed channel when we stored it or sometime since, so the stored + // version might be stale. Refresh it with the fetched version. + return _stripMatchFields(incoming); + } else { + // Message fetching happens out of band from the event queue, so there's + // inherently a race. + // + // If the fetched message reflects changes we haven't yet heard from the + // event queue, then it doesn't much matter which version we use: we'll + // soon get the corresponding events and apply the changes anyway. + // But if it lacks changes we've already heard from the event queue, then + // we won't hear those events again; the only way to wind up with an + // updated message is to use the version we have, that already reflects + // those events' changes. So, stick with the version we have. + return current; + } } + /// Messages in [messages] whose data stream is or was presumably broken + /// by the message being in an unsubscribed channel. + /// + /// This is the subset of [messages] where the message was + /// in an unsubscribed channel when we added it or sometime since. + /// + /// We don't expect update events for messages in unsubscribed channels, + /// so if some of these maybe-stale messages appear in a fetch, + /// we'll always clobber our stored version with the fetched version. + /// See [reconcileMessages]. + /// + /// (We have seen a few such events, actually -- + /// maybe because the channel only recently became unsubscribed? -- + /// but not consistently, and we're not supposed to rely on them.) + final Set _maybeStaleChannelMessages = {}; + Message _stripMatchFields(Message message) { message.matchContent = null; message.matchTopic = null; @@ -510,6 +555,30 @@ class MessageStoreImpl extends HasChannelStore with MessageStore, _OutboxMessage ); } + void handleChannelDeleteEvent(ChannelDeleteEvent event) { + final channelIds = event.streams.map((channel) => channel.streamId); + _handleSubscriptionsRemoved(channelIds); + } + + void handleSubscriptionRemoveEvent(SubscriptionRemoveEvent event) { + _handleSubscriptionsRemoved(event.streamIds); + } + + void _handleSubscriptionsRemoved(Iterable channelIds) { + if (channelIds.length > 8) { + assert(channelIds is! Set); + // optimization: https://github.com/zulip/zulip-flutter/pull/1912#discussion_r2479350329 + channelIds = Set.from(channelIds); + } + + // Linear in [messages]. + final affectedKnownMessageIds = messages.values + .where((message) => message is StreamMessage && channelIds.contains(message.streamId)) + .map((message) => message.id); + + _maybeStaleChannelMessages.addAll(affectedKnownMessageIds); + } + void handleUserTopicEvent(UserTopicEvent event) { for (final view in _messageListViews) { view.handleUserTopicEvent(event); @@ -523,10 +592,18 @@ class MessageStoreImpl extends HasChannelStore with MessageStore, _OutboxMessage } void handleMessageEvent(MessageEvent event) { + final message = event.message; + // If the message is one we already know about (from a fetch), // clobber it with the one from the event system. // See [reconcileMessages] for reasoning. - messages[event.message.id] = event.message; + messages[message.id] = message; + + if (message is StreamMessage && subscriptions[message.streamId] == null) { + // We didn't expect this event, because the channel is unsubscribed. But + // that doesn't mean we should expect future events about this message. + _maybeStaleChannelMessages.add(message.id); + } _handleMessageEventOutbox(event); @@ -615,6 +692,12 @@ class MessageStoreImpl extends HasChannelStore with MessageStore, _OutboxMessage // See [StreamConversation.displayRecipient] on why the invalidation is // needed. message.conversation.displayRecipient = null; + + if (subscriptions[newStreamId] == null) { + // The message was moved into an unsubscribed channel, which means + // we expect our data on it to get stale. + _maybeStaleChannelMessages.add(messageId); + } } if (newTopic != origTopic) { @@ -637,6 +720,7 @@ class MessageStoreImpl extends HasChannelStore with MessageStore, _OutboxMessage void handleDeleteMessageEvent(DeleteMessageEvent event) { for (final messageId in event.messageIds) { messages.remove(messageId); + _maybeStaleChannelMessages.remove(messageId); _editMessageRequests.remove(messageId); } for (final view in _messageListViews) { diff --git a/lib/model/store.dart b/lib/model/store.dart index e9974894c6..fda6e5b695 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -821,11 +821,17 @@ class PerAccountStore extends PerAccountStoreBase with case ChannelEvent(): assert(debugLog("server event: stream/${event.op}")); + if (event is ChannelDeleteEvent) { + _messages.handleChannelDeleteEvent(event); + } _channels.handleChannelEvent(event); notifyListeners(); case SubscriptionEvent(): assert(debugLog("server event: subscription/${event.op}")); + if (event is SubscriptionRemoveEvent) { + _messages.handleSubscriptionRemoveEvent(event); + } _channels.handleSubscriptionEvent(event); notifyListeners(); diff --git a/test/model/message_test.dart b/test/model/message_test.dart index 5bc251d41c..96d95c0c04 100644 --- a/test/model/message_test.dart +++ b/test/model/message_test.dart @@ -36,7 +36,7 @@ void main() { // These "late" variables are the common state operated on by each test. // Each test case calls [prepare] to initialize them. - late Subscription subscription; + late Subscription? subscription; late PerAccountStore store; late FakeApiConnection connection; // [messageList] is here only for the sake of checking when it notifies. @@ -54,15 +54,20 @@ void main() { /// Initialize [store] and the rest of the test state. Future prepare({ ZulipStream? stream, + bool isChannelSubscribed = true, int? zulipFeatureLevel, }) async { stream ??= eg.stream(streamId: eg.defaultStreamMessageStreamId); - subscription = eg.subscription(stream); final selfAccount = eg.selfAccount.copyWith(zulipFeatureLevel: zulipFeatureLevel); store = eg.store(account: selfAccount, initialSnapshot: eg.initialSnapshot(zulipFeatureLevel: zulipFeatureLevel)); await store.addStream(stream); - await store.addSubscription(subscription); + if (isChannelSubscribed) { + subscription = eg.subscription(stream); + await store.addSubscription(subscription!); + } else { + subscription = null; + } connection = store.connection as FakeApiConnection; notifiedCount = 0; messageList = MessageListView.init(store: store, @@ -533,18 +538,137 @@ void main() { }); }); - test('on ID collision, new message does not clobber old in store.messages', () async { - await prepare(); - final message = eg.streamMessage(id: 1, content: '

foo

'); - await addMessages([message]); - check(store.messages).deepEquals({1: message}); - final newMessage = eg.streamMessage(id: 1, content: '

bar

'); - final messages = [newMessage]; - store.reconcileMessages(messages); - check(messages).deepEquals( - // (We'll check more messages in an upcoming commit.) - [message].map(conditionIdentical)); - check(store.messages).deepEquals({1: message}); + group('fetched message with ID already in store.messages', () { + /// Makes a copy of the single message in [MessageStore.messages] + /// by round-tripping through [Message.fromJson] and [Message.toJson]. + /// + /// If that message's [StreamMessage.conversation.displayRecipient] + /// is null, callers must provide a non-null [displayRecipient] + /// to allow [StreamConversation.fromJson] to complete without throwing. + Message copyStoredMessage({String? displayRecipient}) { + final message = store.messages.values.single; + + final json = message.toJson(); + if ( + message is StreamMessage + && message.conversation.displayRecipient == null + ) { + if (displayRecipient == null) throw ArgumentError(); + json['display_recipient'] = displayRecipient; + } + + return Message.fromJson(json); + } + + /// Checks if the single message in [MessageStore.messages] + /// is identical to [message]. + void checkStoredMessageIdenticalTo(Message message) { + check(store.messages) + .deepEquals({message.id: conditionIdentical(message)}); + } + + void checkClobber({Message? withMessageCopy}) { + final messageCopy = withMessageCopy ?? copyStoredMessage(); + store.reconcileMessages([messageCopy]); + checkStoredMessageIdenticalTo(messageCopy); + } + + void checkNoClobber() { + final messageBefore = store.messages.values.single; + store.reconcileMessages([copyStoredMessage()]); + checkStoredMessageIdenticalTo(messageBefore); + } + + test('DM', () async { + await prepare(); + final message = eg.dmMessage(id: 1, from: eg.otherUser, to: [eg.selfUser]); + + store.reconcileMessages([message]); + + // Not clobbering, because the first call didn't mark stale. + checkNoClobber(); + }); + + group('channel message; chooses correctly whether to clobber the stored version', () { + // Exercise the ways we move the message in and out of the "maybe stale" + // state. These include reconcileMessage itself, so sometimes we test + // repeated calls to that with nothing else happening in between. + + test('various conditions', () async { + final channel = eg.stream(); + await prepare(stream: channel, isChannelSubscribed: true); + final message = eg.streamMessage(stream: channel); + + store.reconcileMessages([message]); + + // Not clobbering, because the first call didn't mark stale, + // because the message was in a subscribed channel. + checkNoClobber(); + + await store.removeSubscription(channel.streamId); + // Clobbering because the unsubscribe event marked the message stale. + checkClobber(); + // (Check that reconcileMessage itself didn't unmark as stale.) + checkClobber(); + + await store.addSubscription(eg.subscription(channel)); + // The channel became subscribed, + // but the message's data hasn't been refreshed, so clobber… + checkClobber(); + + // …Now it's been refreshed, by reconcileMessages, so don't clobber. + checkNoClobber(); + + final otherChannel = eg.stream(); + await store.addStream(otherChannel); + check(store.subscriptions[otherChannel.streamId]).isNull(); + await store.handleEvent( + eg.updateMessageEventMoveFrom(origMessages: [message], + newStreamId: otherChannel.streamId)); + // Message was moved to an unsubscribed channel, so clobber. + checkClobber( + withMessageCopy: copyStoredMessage(displayRecipient: otherChannel.name)); + // (Check that reconcileMessage itself didn't unmark as stale.) + checkClobber(); + + // Subscribe, to mark message as not-stale, setting up another check… + await store.addSubscription(eg.subscription(otherChannel)); + + await store.handleEvent(ChannelDeleteEvent(id: 1, streams: [otherChannel])); + // Message was in a channel that became unknown, so clobber. + checkClobber(); + }); + + test('in unsubscribed channel on first call', () async { + await prepare(isChannelSubscribed: false); + final message = eg.streamMessage(); + + store.reconcileMessages([message]); + + checkClobber(); + checkClobber(); + }); + + test('new-message event when in unsubscribed channel', () async { + await prepare(isChannelSubscribed: false); + final message = eg.streamMessage(); + + await store.handleEvent(eg.messageEvent(message)); + + checkClobber(); + checkClobber(); + }); + + test('new-message event when in a subscribed channel', () async { + await prepare(isChannelSubscribed: true); + final message = eg.streamMessage(); + + await store.handleEvent(eg.messageEvent(message)); + + checkNoClobber(); + checkNoClobber(); + }); + }); }); test('matchContent and matchTopic are removed', () async {