From b806f9d8065d613cdb788ad9baf79c4a8452507a Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Mon, 19 May 2025 15:01:20 -0400 Subject: [PATCH 1/3] message [nfc]: Add _disposed flag; check it This change should have no user-facing effect. The one spot where we have an `if (_disposed)` check in editMessage prevents a state update and a rebuild from happening. This only applies if the store is disposed before the edit request fails, but the MessageListView with the edited message should get rebuilt anyway (through onNewStore) when that happens. --- lib/model/message.dart | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/lib/model/message.dart b/lib/model/message.dart index 2573cfadc6..6266e886b8 100644 --- a/lib/model/message.dart +++ b/lib/model/message.dart @@ -94,12 +94,16 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore { @override void registerMessageList(MessageListView view) { + assert(!_disposed); final added = _messageListViews.add(view); assert(added); } @override void unregisterMessageList(MessageListView view) { + // TODO: Add `assert(!_disposed);` here once we ensure [PerAccountStore] is + // only disposed after [MessageListView]s with references to it are + // disposed. See [dispose] for details. final removed = _messageListViews.remove(view); assert(removed); } @@ -122,6 +126,8 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore { } } + bool _disposed = false; + void dispose() { // Not disposing the [MessageListView]s here, because they are owned by // (i.e., they get [dispose]d by) the [_MessageListState], including in the @@ -137,10 +143,14 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore { // [InheritedNotifier] to rebuild in the next frame) before the owner's // `dispose` or `onNewStore` is called. Discussion: // https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/MessageListView.20lifecycle/near/2086893 + + assert(!_disposed); + _disposed = true; } @override Future sendMessage({required MessageDestination destination, required String content}) { + assert(!_disposed); // TODO implement outbox; see design at // https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M3881.20Sending.20outbox.20messages.20is.20fraught.20with.20issues/near/1405739 return _apiSendMessage(connection, @@ -152,6 +162,7 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore { @override void reconcileMessages(List messages) { + assert(!_disposed); // What to do when some of the just-fetched messages are already known? // This is common and normal: in particular it happens when one message list // overlaps another, e.g. a stream and a topic within it. @@ -185,6 +196,7 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore { required String originalRawContent, required String newContent, }) async { + assert(!_disposed); if (_editMessageRequests.containsKey(messageId)) { throw StateError('an edit request is already in progress'); } @@ -202,6 +214,8 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore { } catch (e) { // TODO(log) if e is something unexpected + if (_disposed) return; + final status = _editMessageRequests[messageId]; if (status == null) { // The event actually arrived before this request failed @@ -216,6 +230,7 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore { @override ({String originalRawContent, String newContent}) takeFailedMessageEdit(int messageId) { + assert(!_disposed); final status = _editMessageRequests.remove(messageId); _notifyMessageListViewsForOneMessage(messageId); if (status == null) { From 17a1a4ce5a3ca11fe403470fad88f700c6aa2488 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 25 Mar 2025 16:32:30 -0400 Subject: [PATCH 2/3] message: Create an outbox message on send; manage its states While we do create outbox messages, there are in no way user-visible changes since the outbox messages don't end up in message list views. We create skeletons for helpers needed from message list view, but don't implement them yet, to make the diff smaller. For testing, similar to TypingNotifier.debugEnable, we add MessageStoreImpl.debugOutboxEnable for tests that do not intend to cover outbox messages. --- lib/model/message.dart | 460 +++++++++++++++++++++++++++- lib/model/message_list.dart | 20 ++ lib/model/store.dart | 8 +- test/api/model/model_checks.dart | 1 + test/example_data.dart | 4 +- test/fake_async_checks.dart | 6 + test/model/message_checks.dart | 9 + test/model/message_test.dart | 332 +++++++++++++++++++- test/model/narrow_test.dart | 81 +++-- test/model/store_test.dart | 5 +- test/widgets/compose_box_test.dart | 13 + test/widgets/message_list_test.dart | 11 +- 12 files changed, 887 insertions(+), 63 deletions(-) create mode 100644 test/fake_async_checks.dart create mode 100644 test/model/message_checks.dart diff --git a/lib/model/message.dart b/lib/model/message.dart index 6266e886b8..3fffdfc4a4 100644 --- a/lib/model/message.dart +++ b/lib/model/message.dart @@ -1,11 +1,15 @@ +import 'dart:async'; +import 'dart:collection'; import 'dart:convert'; import 'package:crypto/crypto.dart'; +import 'package:flutter/foundation.dart'; import '../api/model/events.dart'; import '../api/model/model.dart'; import '../api/route/messages.dart'; import '../log.dart'; +import 'binding.dart'; import 'message_list.dart'; import 'store.dart'; @@ -16,6 +20,9 @@ mixin MessageStore { /// All known messages, indexed by [Message.id]. Map get messages; + /// [OutboxMessage]s sent by the user, indexed by [OutboxMessage.localMessageId]. + Map get outboxMessages; + Set get debugMessageListViews; void registerMessageList(MessageListView view); @@ -26,6 +33,15 @@ mixin MessageStore { required String content, }); + /// Remove from [outboxMessages] given the [localMessageId], and return + /// the removed [OutboxMessage]. + /// + /// The outbox message to be taken must exist. + /// + /// The state of the outbox message must be either [OutboxMessageState.failed] + /// or [OutboxMessageState.waitPeriodExpired]. + OutboxMessage takeOutboxMessage(int localMessageId); + /// Reconcile a batch of just-fetched messages with the store, /// mutating the list. /// @@ -78,15 +94,29 @@ class _EditMessageRequestStatus { final String newContent; } -class MessageStoreImpl extends PerAccountStoreBase with MessageStore { - MessageStoreImpl({required super.core}) - // There are no messages in InitialSnapshot, so we don't have - // a use case for initializing MessageStore with nonempty [messages]. - : messages = {}; +class MessageStoreImpl extends PerAccountStoreBase with MessageStore, _OutboxMessageStore { + MessageStoreImpl({required super.core, required String? realmEmptyTopicDisplayName}) + : _realmEmptyTopicDisplayName = realmEmptyTopicDisplayName, + // There are no messages in InitialSnapshot, so we don't have + // a use case for initializing MessageStore with nonempty [messages]. + messages = {}; + + /// The display name to use for empty topics. + /// + /// This should only be accessed when FL >= 334, since topics cannot + /// be empty otherwise. + // TODO(server-10) simplify this + String get realmEmptyTopicDisplayName { + assert(zulipFeatureLevel >= 334); + assert(_realmEmptyTopicDisplayName != null); // TODO(log) + return _realmEmptyTopicDisplayName ?? 'general chat'; + } + final String? _realmEmptyTopicDisplayName; // TODO(#668): update this realm setting @override final Map messages; + @override final Set _messageListViews = {}; @override @@ -126,6 +156,7 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore { } } + @override bool _disposed = false; void dispose() { @@ -145,19 +176,24 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore { // https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/MessageListView.20lifecycle/near/2086893 assert(!_disposed); + _disposeOutboxMessages(); _disposed = true; } @override Future sendMessage({required MessageDestination destination, required String content}) { assert(!_disposed); - // TODO implement outbox; see design at - // https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M3881.20Sending.20outbox.20messages.20is.20fraught.20with.20issues/near/1405739 - return _apiSendMessage(connection, - destination: destination, - content: content, - readBySender: true, - ); + if (!debugOutboxEnable) { + return _apiSendMessage(connection, + destination: destination, + content: content, + readBySender: true); + } + return _outboxSendMessage( + destination: destination, content: content, + // TODO move [TopicName.processLikeServer] to a substore, eliminating this + // see https://github.com/zulip/zulip-flutter/pull/1472#discussion_r2099069276 + realmEmptyTopicDisplayName: _realmEmptyTopicDisplayName); } @override @@ -257,6 +293,8 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore { // See [fetchedMessages] for reasoning. messages[event.message.id] = event.message; + _handleMessageEventOutbox(event); + for (final view in _messageListViews) { view.handleMessageEvent(event); } @@ -450,4 +488,402 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore { // [Poll] is responsible for notifying the affected listeners. poll.handleSubmessageEvent(event); } + + /// In debug mode, controls whether outbox messages should be created when + /// [sendMessage] is called. + /// + /// Outside of debug mode, this is always true and the setter has no effect. + static bool get debugOutboxEnable { + bool result = true; + assert(() { + result = _debugOutboxEnable; + return true; + }()); + return result; + } + static bool _debugOutboxEnable = true; + static set debugOutboxEnable(bool value) { + assert(() { + _debugOutboxEnable = value; + return true; + }()); + } + + @visibleForTesting + static void debugReset() { + _debugOutboxEnable = true; + } +} + +/// The duration an outbox message stays hidden to the user. +/// +/// See [OutboxMessageState.waiting]. +const kLocalEchoDebounceDuration = Duration(milliseconds: 500); // TODO(#1441) find the right value for this + +/// The duration before an outbox message can be restored for resending, since +/// its creation. +/// +/// See [OutboxMessageState.waitPeriodExpired]. +const kSendMessageOfferRestoreWaitPeriod = Duration(seconds: 10); // TODO(#1441) find the right value for this + +/// States of an [OutboxMessage] since its creation from a +/// [MessageStore.sendMessage] call and before its eventual deletion. +/// +/// ``` +/// Got an [ApiRequestException]. +/// ┌──────┬──────────┬─────────────► failed +/// (create) │ │ │ │ +/// └► hidden waiting waitPeriodExpired ──┴──────────────► (delete) +/// │ ▲ │ ▲ User restores +/// └──────┘ └──────┘ the draft. +/// Debounce [sendMessage] request +/// timed out. not finished when +/// wait period timed out. +/// +/// Event received. +/// (any state) ─────────────────► (delete) +/// ``` +/// +/// During its lifecycle, it is guaranteed that the outbox message is deleted +/// as soon a message event with a matching [MessageEvent.localMessageId] +/// arrives. +enum OutboxMessageState { + /// The [sendMessage] HTTP request has started but the resulting + /// [MessageEvent] hasn't arrived, and nor has the request failed. In this + /// state, the outbox message is hidden to the user. + /// + /// This is the initial state when an [OutboxMessage] is created. + hidden, + + /// The [sendMessage] HTTP request has started but hasn't finished, and the + /// outbox message is shown to the user. + /// + /// This state can be reached after staying in [hidden] for + /// [kLocalEchoDebounceDuration]. + waiting, + + /// The [sendMessage] HTTP request did not finish in time and the user is + /// invited to retry it. + /// + /// This state can be reached when the request has not finished + /// [kSendMessageOfferRestoreWaitPeriod] since the outbox message's creation. + waitPeriodExpired, + + /// The message could not be delivered, and the user is invited to retry it. + /// + /// This state can be reached when we got an [ApiRequestException] from the + /// [sendMessage] HTTP request. + failed, +} + +/// An outstanding request to send a message, aka an outbox-message. +/// +/// This will be shown in the UI in the message list, as a placeholder +/// for the actual [Message] the request is anticipated to produce. +/// +/// A request remains "outstanding" even after the [sendMessage] HTTP request +/// completes, whether with success or failure. +/// The outbox-message persists until either the corresponding [MessageEvent] +/// arrives to replace it, or the user discards it (perhaps to try again). +/// For details, see the state diagram at [OutboxMessageState], +/// and [MessageStore.takeOutboxMessage]. +sealed class OutboxMessage extends MessageBase { + OutboxMessage({ + required this.localMessageId, + required int selfUserId, + required super.timestamp, + required this.contentMarkdown, + }) : _state = OutboxMessageState.hidden, + super(senderId: selfUserId); + + // TODO(dart): This has to be a plain static method, because factories/constructors + // do not support type parameters: https://github.com/dart-lang/language/issues/647 + static OutboxMessage fromConversation(Conversation conversation, { + required int localMessageId, + required int selfUserId, + required int timestamp, + required String contentMarkdown, + }) { + return switch (conversation) { + StreamConversation() => StreamOutboxMessage._( + localMessageId: localMessageId, + selfUserId: selfUserId, + timestamp: timestamp, + conversation: conversation, + contentMarkdown: contentMarkdown), + DmConversation() => DmOutboxMessage._( + localMessageId: localMessageId, + selfUserId: selfUserId, + timestamp: timestamp, + conversation: conversation, + contentMarkdown: contentMarkdown), + }; + } + + /// As in [MessageEvent.localMessageId]. + /// + /// This uniquely identifies this outbox message's corresponding message object + /// in events from the same event queue. + /// + /// See also: + /// * [MessageStoreImpl.sendMessage], where this ID is assigned. + final int localMessageId; + + @override + int? get id => null; + + final String contentMarkdown; + + OutboxMessageState get state => _state; + OutboxMessageState _state; + + /// Whether the [OutboxMessage] is hidden to [MessageListView] or not. + bool get hidden => state == OutboxMessageState.hidden; +} + +class StreamOutboxMessage extends OutboxMessage { + StreamOutboxMessage._({ + required super.localMessageId, + required super.selfUserId, + required super.timestamp, + required this.conversation, + required super.contentMarkdown, + }); + + @override + final StreamConversation conversation; +} + +class DmOutboxMessage extends OutboxMessage { + DmOutboxMessage._({ + required super.localMessageId, + required super.selfUserId, + required super.timestamp, + required this.conversation, + required super.contentMarkdown, + }) : assert(conversation.allRecipientIds.contains(selfUserId)); + + @override + final DmConversation conversation; +} + +/// Manages the outbox messages portion of [MessageStore]. +mixin _OutboxMessageStore on PerAccountStoreBase { + late final UnmodifiableMapView outboxMessages = + UnmodifiableMapView(_outboxMessages); + final Map _outboxMessages = {}; + + /// A map of timers to show outbox messages after a delay, + /// indexed by [OutboxMessage.localMessageId]. + /// + /// If the send message request fails within the time limit, + /// the outbox message's timer gets removed and cancelled. + final Map _outboxMessageDebounceTimers = {}; + + /// A map of timers to update outbox messages state to + /// [OutboxMessageState.waitPeriodExpired] if the [sendMessage] + /// request did not complete in time, + /// indexed by [OutboxMessage.localMessageId]. + /// + /// If the send message request completes within the time limit, + /// the outbox message's timer gets removed and cancelled. + final Map _outboxMessageWaitPeriodTimers = {}; + + /// A fresh ID to use for [OutboxMessage.localMessageId], + /// unique within this instance. + int _nextLocalMessageId = 1; + + /// As in [MessageStoreImpl._messageListViews]. + Set get _messageListViews; + + /// As in [MessageStoreImpl._disposed]. + bool get _disposed; + + /// Update the state of the [OutboxMessage] with the given [localMessageId], + /// and notify listeners if necessary. + /// + /// The outbox message with [localMessageId] must exist. + void _updateOutboxMessage(int localMessageId, { + required OutboxMessageState newState, + }) { + assert(!_disposed); + final outboxMessage = outboxMessages[localMessageId]; + if (outboxMessage == null) { + throw StateError( + 'Removing unknown outbox message with localMessageId: $localMessageId'); + } + final oldState = outboxMessage.state; + // See [OutboxMessageState] for valid state transitions. + final isStateTransitionValid = switch (newState) { + OutboxMessageState.hidden => false, + OutboxMessageState.waiting => + oldState == OutboxMessageState.hidden, + OutboxMessageState.waitPeriodExpired => + oldState == OutboxMessageState.waiting, + OutboxMessageState.failed => + oldState == OutboxMessageState.hidden + || oldState == OutboxMessageState.waiting + || oldState == OutboxMessageState.waitPeriodExpired, + }; + if (!isStateTransitionValid) { + throw StateError('Unexpected state transition: $oldState -> $newState'); + } + + outboxMessage._state = newState; + for (final view in _messageListViews) { + if (oldState == OutboxMessageState.hidden) { + view.addOutboxMessage(outboxMessage); + } else { + view.notifyListenersIfOutboxMessagePresent(localMessageId); + } + } + } + + /// Send a message and create an entry of [OutboxMessage]. + Future _outboxSendMessage({ + required MessageDestination destination, + required String content, + required String? realmEmptyTopicDisplayName, + }) async { + assert(!_disposed); + final localMessageId = _nextLocalMessageId++; + assert(!outboxMessages.containsKey(localMessageId)); + + final conversation = switch (destination) { + StreamDestination(:final streamId, :final topic) => + StreamConversation( + streamId, + _processTopicLikeServer( + topic, realmEmptyTopicDisplayName: realmEmptyTopicDisplayName), + displayRecipient: null), + DmDestination(:final userIds) => DmConversation(allRecipientIds: userIds), + }; + + _outboxMessages[localMessageId] = OutboxMessage.fromConversation( + conversation, + localMessageId: localMessageId, + selfUserId: selfUserId, + timestamp: ZulipBinding.instance.utcNow().millisecondsSinceEpoch ~/ 1000, + contentMarkdown: content); + + _outboxMessageDebounceTimers[localMessageId] = Timer( + kLocalEchoDebounceDuration, + () => _handleOutboxDebounce(localMessageId)); + + _outboxMessageWaitPeriodTimers[localMessageId] = Timer( + kSendMessageOfferRestoreWaitPeriod, + () => _handleOutboxWaitPeriodExpired(localMessageId)); + + try { + await _apiSendMessage(connection, + destination: destination, + content: content, + readBySender: true, + queueId: queueId, + localId: localMessageId.toString()); + } catch (e) { + if (_disposed) return; + if (!_outboxMessages.containsKey(localMessageId)) { + // The message event already arrived; the failure is probably due to + // networking issues. Don't rethrow; the send succeeded + // (we got the event) so we don't want to show an error dialog. + return; + } + _outboxMessageDebounceTimers.remove(localMessageId)?.cancel(); + _outboxMessageWaitPeriodTimers.remove(localMessageId)?.cancel(); + _updateOutboxMessage(localMessageId, newState: OutboxMessageState.failed); + rethrow; + } + if (_disposed) return; + if (!_outboxMessages.containsKey(localMessageId)) { + // The message event already arrived; nothing to do. + return; + } + // The send request succeeded, so the message was definitely sent. + // Cancel the timer that would have had us start presuming that the + // send might have failed. + _outboxMessageWaitPeriodTimers.remove(localMessageId)?.cancel(); + } + + TopicName _processTopicLikeServer(TopicName topic, { + required String? realmEmptyTopicDisplayName, + }) { + return topic.processLikeServer( + // Processing this just once on creating the outbox message + // allows an uncommon bug, because either of these values can change. + // During the outbox message's life, a topic processed from + // "(no topic)" could become stale/wrong when zulipFeatureLevel + // changes; a topic processed from "general chat" could become + // stale/wrong when realmEmptyTopicDisplayName changes. + // + // Shrug. The same effect is caused by an unavoidable race: + // an admin could change the name of "general chat" + // (i.e. the value of realmEmptyTopicDisplayName) + // concurrently with the user making the send request, + // so that the setting in effect by the time the request arrives + // is different from the setting the client last heard about. + zulipFeatureLevel: zulipFeatureLevel, + realmEmptyTopicDisplayName: realmEmptyTopicDisplayName); + } + + void _handleOutboxDebounce(int localMessageId) { + assert(!_disposed); + assert(outboxMessages.containsKey(localMessageId), + 'The timer should have been canceled when the outbox message was removed.'); + _outboxMessageDebounceTimers.remove(localMessageId); + _updateOutboxMessage(localMessageId, newState: OutboxMessageState.waiting); + } + + void _handleOutboxWaitPeriodExpired(int localMessageId) { + assert(!_disposed); + assert(outboxMessages.containsKey(localMessageId), + 'The timer should have been canceled when the outbox message was removed.'); + assert(!_outboxMessageDebounceTimers.containsKey(localMessageId), + 'The debounce timer should have been removed before the wait period timer expires.'); + _outboxMessageWaitPeriodTimers.remove(localMessageId); + _updateOutboxMessage(localMessageId, newState: OutboxMessageState.waitPeriodExpired); + } + + OutboxMessage takeOutboxMessage(int localMessageId) { + assert(!_disposed); + final removed = _outboxMessages.remove(localMessageId); + _outboxMessageDebounceTimers.remove(localMessageId)?.cancel(); + _outboxMessageWaitPeriodTimers.remove(localMessageId)?.cancel(); + if (removed == null) { + throw StateError( + 'Removing unknown outbox message with localMessageId: $localMessageId'); + } + if (removed.state != OutboxMessageState.failed + && removed.state != OutboxMessageState.waitPeriodExpired + ) { + throw StateError('Unexpected state when restoring draft: ${removed.state}'); + } + for (final view in _messageListViews) { + view.removeOutboxMessage(removed); + } + return removed; + } + + void _handleMessageEventOutbox(MessageEvent event) { + if (event.localMessageId != null) { + final localMessageId = int.parse(event.localMessageId!, radix: 10); + // The outbox message can be missing if the user removes it (to be + // implemented in #1441) before the event arrives. + // Nothing to do in that case. + _outboxMessages.remove(localMessageId); + _outboxMessageDebounceTimers.remove(localMessageId)?.cancel(); + _outboxMessageWaitPeriodTimers.remove(localMessageId)?.cancel(); + } + } + + /// Cancel [_OutboxMessageStore]'s timers. + void _disposeOutboxMessages() { + assert(!_disposed); + for (final timer in _outboxMessageDebounceTimers.values) { + timer.cancel(); + } + for (final timer in _outboxMessageWaitPeriodTimers.values) { + timer.cancel(); + } + } } diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index f2a45b78aa..4da9ebd3cc 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -10,6 +10,7 @@ import '../api/route/messages.dart'; import 'algorithms.dart'; import 'channel.dart'; import 'content.dart'; +import 'message.dart'; import 'narrow.dart'; import 'store.dart'; @@ -616,6 +617,20 @@ class MessageListView with ChangeNotifier, _MessageSequence { } } + /// Add [outboxMessage] if it belongs to the view. + void addOutboxMessage(OutboxMessage outboxMessage) { + // TODO(#1441) implement this + } + + /// Remove the [outboxMessage] from the view. + /// + /// This is a no-op if the message is not found. + /// + /// This should only be called from [MessageStore.takeOutboxMessage]. + void removeOutboxMessage(OutboxMessage outboxMessage) { + // TODO(#1441) implement this + } + void handleUserTopicEvent(UserTopicEvent event) { switch (_canAffectVisibility(event)) { case VisibilityEffect.none: @@ -777,6 +792,11 @@ class MessageListView with ChangeNotifier, _MessageSequence { } } + /// Notify listeners if the given outbox message is present in this view. + void notifyListenersIfOutboxMessagePresent(int localMessageId) { + // TODO(#1441) implement this + } + /// Called when the app is reassembled during debugging, e.g. for hot reload. /// /// This will redo from scratch any computations we can, such as parsing diff --git a/lib/model/store.dart b/lib/model/store.dart index 240e3ab4e4..18a09e32ce 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -501,7 +501,8 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor typingStartedExpiryPeriod: Duration(milliseconds: initialSnapshot.serverTypingStartedExpiryPeriodMilliseconds), ), channels: channels, - messages: MessageStoreImpl(core: core), + messages: MessageStoreImpl(core: core, + realmEmptyTopicDisplayName: initialSnapshot.realmEmptyTopicDisplayName), unreads: Unreads( initial: initialSnapshot.unreadMsgs, core: core, @@ -745,6 +746,8 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor @override Map get messages => _messages.messages; @override + Map get outboxMessages => _messages.outboxMessages; + @override void registerMessageList(MessageListView view) => _messages.registerMessageList(view); @override @@ -756,6 +759,9 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor return _messages.sendMessage(destination: destination, content: content); } @override + OutboxMessage takeOutboxMessage(int localMessageId) => + _messages.takeOutboxMessage(localMessageId); + @override void reconcileMessages(List messages) { _messages.reconcileMessages(messages); // TODO(#649) notify [unreads] of the just-fetched messages diff --git a/test/api/model/model_checks.dart b/test/api/model/model_checks.dart index b90238ae35..3ae106afcc 100644 --- a/test/api/model/model_checks.dart +++ b/test/api/model/model_checks.dart @@ -37,6 +37,7 @@ extension TopicNameChecks on Subject { } extension StreamConversationChecks on Subject { + Subject get topic => has((x) => x.topic, 'topic'); Subject get displayRecipient => has((x) => x.displayRecipient, 'displayRecipient'); } diff --git a/test/example_data.dart b/test/example_data.dart index d803196269..93869df37a 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -695,8 +695,8 @@ UserTopicEvent userTopicEvent( ); } -MessageEvent messageEvent(Message message) => - MessageEvent(id: 0, message: message, localMessageId: null); +MessageEvent messageEvent(Message message, {int? localMessageId}) => + MessageEvent(id: 0, message: message, localMessageId: localMessageId?.toString()); DeleteMessageEvent deleteMessageEvent(List messages) { assert(messages.isNotEmpty); diff --git a/test/fake_async_checks.dart b/test/fake_async_checks.dart new file mode 100644 index 0000000000..51c653123a --- /dev/null +++ b/test/fake_async_checks.dart @@ -0,0 +1,6 @@ +import 'package:checks/checks.dart'; +import 'package:fake_async/fake_async.dart'; + +extension FakeTimerChecks on Subject { + Subject get duration => has((t) => t.duration, 'duration'); +} diff --git a/test/model/message_checks.dart b/test/model/message_checks.dart new file mode 100644 index 0000000000..b56cd89a79 --- /dev/null +++ b/test/model/message_checks.dart @@ -0,0 +1,9 @@ +import 'package:checks/checks.dart'; +import 'package:zulip/api/model/model.dart'; +import 'package:zulip/model/message.dart'; + +extension OutboxMessageChecks on Subject> { + Subject get localMessageId => has((x) => x.localMessageId, 'localMessageId'); + Subject get state => has((x) => x.state, 'state'); + Subject get hidden => has((x) => x.hidden, 'hidden'); +} diff --git a/test/model/message_test.dart b/test/model/message_test.dart index 1809f0888b..4f8183d6d4 100644 --- a/test/model/message_test.dart +++ b/test/model/message_test.dart @@ -1,14 +1,17 @@ +import 'dart:async'; import 'dart:convert'; import 'dart:io'; import 'package:checks/checks.dart'; import 'package:crypto/crypto.dart'; +import 'package:fake_async/fake_async.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/model.dart'; import 'package:zulip/api/model/submessage.dart'; import 'package:zulip/api/route/messages.dart'; +import 'package:zulip/model/message.dart'; import 'package:zulip/model/message_list.dart'; import 'package:zulip/model/narrow.dart'; import 'package:zulip/model/store.dart'; @@ -18,12 +21,17 @@ import '../api/model/model_checks.dart'; import '../api/model/submessage_checks.dart'; import '../example_data.dart' as eg; import '../fake_async.dart'; +import '../fake_async_checks.dart'; import '../stdlib_checks.dart'; +import 'binding.dart'; +import 'message_checks.dart'; import 'message_list_test.dart'; import 'store_checks.dart'; import 'test_store.dart'; void main() { + TestZulipBinding.ensureInitialized(); + // These "late" variables are the common state operated on by each test. // Each test case calls [prepare] to initialize them. late Subscription subscription; @@ -42,10 +50,16 @@ void main() { void checkNotifiedOnce() => checkNotified(count: 1); /// Initialize [store] and the rest of the test state. - Future prepare({Narrow narrow = const CombinedFeedNarrow()}) async { - final stream = eg.stream(streamId: eg.defaultStreamMessageStreamId); + Future prepare({ + Narrow narrow = const CombinedFeedNarrow(), + ZulipStream? stream, + int? zulipFeatureLevel, + }) async { + stream ??= eg.stream(streamId: eg.defaultStreamMessageStreamId); subscription = eg.subscription(stream); - store = eg.store(); + 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); connection = store.connection as FakeApiConnection; @@ -54,8 +68,12 @@ void main() { ..addListener(() { notifiedCount++; }); + addTearDown(messageList.dispose); check(messageList).fetched.isFalse(); checkNotNotified(); + + // This cleans up possibly pending timers from [MessageStoreImpl]. + addTearDown(store.dispose); } /// Perform the initial message fetch for [messageList]. @@ -76,6 +94,314 @@ void main() { checkNotified(count: messageList.fetched ? messages.length : 0); } + test('dispose cancels pending timers', () => awaitFakeAsync((async) async { + final stream = eg.stream(); + final store = eg.store(); + await store.addStream(stream); + await store.addSubscription(eg.subscription(stream)); + + (store.connection as FakeApiConnection).prepare( + json: SendMessageResult(id: 1).toJson(), + delay: const Duration(seconds: 1)); + unawaited(store.sendMessage( + destination: StreamDestination(stream.streamId, eg.t('topic')), + content: 'content')); + check(async.pendingTimers).deepEquals(>[ + (it) => it.isA().duration.equals(kLocalEchoDebounceDuration), + (it) => it.isA().duration.equals(kSendMessageOfferRestoreWaitPeriod), + (it) => it.isA().duration.equals(const Duration(seconds: 1)), + ]); + + store.dispose(); + check(async.pendingTimers).single.duration.equals(const Duration(seconds: 1)); + })); + + group('sendMessage', () { + final stream = eg.stream(); + final streamDestination = StreamDestination(stream.streamId, eg.t('some topic')); + late StreamMessage message; + + test('outbox messages get unique localMessageId', () async { + await prepare(stream: stream); + await prepareMessages([]); + + for (int i = 0; i < 10; i++) { + connection.prepare(json: SendMessageResult(id: 1).toJson()); + await store.sendMessage(destination: streamDestination, content: 'content'); + } + // [store.outboxMessages] has the same number of keys (localMessageId) + // as the number of sent messages, which are guaranteed to be distinct. + check(store.outboxMessages).keys.length.equals(10); + }); + + Subject checkState() => + check(store.outboxMessages).values.single.state; + + Future prepareOutboxMessage({ + MessageDestination? destination, + int? zulipFeatureLevel, + }) async { + message = eg.streamMessage(stream: stream); + await prepare(stream: stream, zulipFeatureLevel: zulipFeatureLevel); + await prepareMessages([eg.streamMessage(stream: stream)]); + connection.prepare(json: SendMessageResult(id: 1).toJson()); + await store.sendMessage( + destination: destination ?? streamDestination, content: 'content'); + } + + late Future outboxMessageFailFuture; + Future prepareOutboxMessageToFailAfterDelay(Duration delay) async { + message = eg.streamMessage(stream: stream); + await prepare(stream: stream); + await prepareMessages([eg.streamMessage(stream: stream)]); + connection.prepare(httpException: SocketException('failed'), delay: delay); + outboxMessageFailFuture = store.sendMessage( + destination: streamDestination, content: 'content'); + } + + Future receiveMessage([Message? messageReceived]) async { + await store.handleEvent(eg.messageEvent(messageReceived ?? message, + localMessageId: store.outboxMessages.keys.single)); + } + + test('smoke DM: hidden -> waiting -> (delete)', () => awaitFakeAsync((async) async { + await prepareOutboxMessage(destination: DmDestination( + userIds: [eg.selfUser.userId, eg.otherUser.userId])); + checkState().equals(OutboxMessageState.hidden); + + async.elapse(kLocalEchoDebounceDuration); + checkState().equals(OutboxMessageState.waiting); + + await receiveMessage(eg.dmMessage(from: eg.selfUser, to: [eg.otherUser])); + check(store.outboxMessages).isEmpty(); + })); + + test('smoke stream message: hidden -> waiting -> (delete)', () => awaitFakeAsync((async) async { + await prepareOutboxMessage(destination: StreamDestination( + stream.streamId, eg.t('foo'))); + checkState().equals(OutboxMessageState.hidden); + + async.elapse(kLocalEchoDebounceDuration); + checkState().equals(OutboxMessageState.waiting); + + await receiveMessage(eg.streamMessage(stream: stream, topic: 'foo')); + check(store.outboxMessages).isEmpty(); + })); + + test('hidden -> waiting and never transition to waitPeriodExpired', () => awaitFakeAsync((async) async { + await prepareOutboxMessage(); + checkState().equals(OutboxMessageState.hidden); + + async.elapse(kLocalEchoDebounceDuration); + checkState().equals(OutboxMessageState.waiting); + + // Wait till we reach at least [kSendMessageOfferRestoreWaitPeriod] after + // the send request was initiated. + async.elapse( + kSendMessageOfferRestoreWaitPeriod - kLocalEchoDebounceDuration); + async.flushTimers(); + // The outbox message should stay in the waiting state; + // it should not transition to waitPeriodExpired. + checkState().equals(OutboxMessageState.waiting); + })); + + test('waiting -> waitPeriodExpired', () => awaitFakeAsync((async) async { + await prepareOutboxMessageToFailAfterDelay( + kSendMessageOfferRestoreWaitPeriod + Duration(seconds: 1)); + async.elapse(kLocalEchoDebounceDuration); + checkState().equals(OutboxMessageState.waiting); + + async.elapse(kSendMessageOfferRestoreWaitPeriod - kLocalEchoDebounceDuration); + checkState().equals(OutboxMessageState.waitPeriodExpired); + + await check(outboxMessageFailFuture).throws(); + })); + + group('… -> failed', () { + test('hidden -> failed', () => awaitFakeAsync((async) async { + await prepareOutboxMessageToFailAfterDelay(Duration.zero); + checkState().equals(OutboxMessageState.hidden); + + await check(outboxMessageFailFuture).throws(); + checkState().equals(OutboxMessageState.failed); + + // Wait till we reach at least [kSendMessageOfferRestoreWaitPeriod] after + // the send request was initiated. + async.elapse(kSendMessageOfferRestoreWaitPeriod); + async.flushTimers(); + // The outbox message should stay in the failed state; + // it should not transition to waitPeriodExpired. + checkState().equals(OutboxMessageState.failed); + })); + + test('waiting -> failed', () => awaitFakeAsync((async) async { + await prepareOutboxMessageToFailAfterDelay( + kLocalEchoDebounceDuration + Duration(seconds: 1)); + async.elapse(kLocalEchoDebounceDuration); + checkState().equals(OutboxMessageState.waiting); + + await check(outboxMessageFailFuture).throws(); + checkState().equals(OutboxMessageState.failed); + })); + + test('waitPeriodExpired -> failed', () => awaitFakeAsync((async) async { + await prepareOutboxMessageToFailAfterDelay( + kSendMessageOfferRestoreWaitPeriod + Duration(seconds: 1)); + async.elapse(kSendMessageOfferRestoreWaitPeriod); + checkState().equals(OutboxMessageState.waitPeriodExpired); + + await check(outboxMessageFailFuture).throws(); + checkState().equals(OutboxMessageState.failed); + })); + }); + + group('… -> (delete)', () { + test('hidden -> (delete) because event received', () => awaitFakeAsync((async) async { + await prepareOutboxMessage(); + checkState().equals(OutboxMessageState.hidden); + + await receiveMessage(); + check(store.outboxMessages).isEmpty(); + })); + + test('hidden -> (delete) when event arrives before send request fails', () => awaitFakeAsync((async) async { + // Set up an error to fail `sendMessage` with a delay, leaving time for + // the message event to arrive. + await prepareOutboxMessageToFailAfterDelay(const Duration(seconds: 1)); + checkState().equals(OutboxMessageState.hidden); + + // Received the message event while the message is being sent. + await receiveMessage(); + check(store.outboxMessages).isEmpty(); + + // Complete the send request. There should be no error despite + // the send request failure, because the outbox message is not + // in the store any more. + await check(outboxMessageFailFuture).completes(); + async.elapse(const Duration(seconds: 1)); + })); + + test('waiting -> (delete) because event received', () => awaitFakeAsync((async) async { + await prepareOutboxMessage(); + async.elapse(kLocalEchoDebounceDuration); + checkState().equals(OutboxMessageState.waiting); + + await receiveMessage(); + check(store.outboxMessages).isEmpty(); + })); + + test('waiting -> (delete) when event arrives before send request fails', () => awaitFakeAsync((async) async { + // Set up an error to fail `sendMessage` with a delay, leaving time for + // the message event to arrive. + await prepareOutboxMessageToFailAfterDelay( + kLocalEchoDebounceDuration + Duration(seconds: 1)); + async.elapse(kLocalEchoDebounceDuration); + checkState().equals(OutboxMessageState.waiting); + + // Received the message event while the message is being sent. + await receiveMessage(); + check(store.outboxMessages).isEmpty(); + + // Complete the send request. There should be no error despite + // the send request failure, because the outbox message is not + // in the store any more. + await check(outboxMessageFailFuture).completes(); + })); + + test('waitPeriodExpired -> (delete) when event arrives before send request fails', () => awaitFakeAsync((async) async { + // Set up an error to fail `sendMessage` with a delay, leaving time for + // the message event to arrive. + await prepareOutboxMessageToFailAfterDelay( + kSendMessageOfferRestoreWaitPeriod + Duration(seconds: 1)); + async.elapse(kSendMessageOfferRestoreWaitPeriod); + checkState().equals(OutboxMessageState.waitPeriodExpired); + + // Received the message event while the message is being sent. + await receiveMessage(); + check(store.outboxMessages).isEmpty(); + + // Complete the send request. There should be no error despite + // the send request failure, because the outbox message is not + // in the store any more. + await check(outboxMessageFailFuture).completes(); + })); + + test('waitPeriodExpired -> (delete) because outbox message was taken', () => awaitFakeAsync((async) async { + // Set up an error to fail `sendMessage` with a delay, leaving time for + // the outbox message to be taken (by the user, presumably). + await prepareOutboxMessageToFailAfterDelay( + kSendMessageOfferRestoreWaitPeriod + Duration(seconds: 1)); + async.elapse(kSendMessageOfferRestoreWaitPeriod); + checkState().equals(OutboxMessageState.waitPeriodExpired); + + store.takeOutboxMessage(store.outboxMessages.keys.single); + check(store.outboxMessages).isEmpty(); + })); + + test('failed -> (delete) because event received', () => awaitFakeAsync((async) async { + await prepareOutboxMessageToFailAfterDelay(Duration.zero); + await check(outboxMessageFailFuture).throws(); + checkState().equals(OutboxMessageState.failed); + + await receiveMessage(); + check(store.outboxMessages).isEmpty(); + })); + + test('failed -> (delete) because outbox message was taken', () => awaitFakeAsync((async) async { + await prepareOutboxMessageToFailAfterDelay(Duration.zero); + await check(outboxMessageFailFuture).throws(); + checkState().equals(OutboxMessageState.failed); + + store.takeOutboxMessage(store.outboxMessages.keys.single); + check(store.outboxMessages).isEmpty(); + })); + }); + + test('when sending to "(no topic)", process topic like the server does when creating outbox message', () => awaitFakeAsync((async) async { + await prepareOutboxMessage( + destination: StreamDestination(stream.streamId, TopicName('(no topic)')), + zulipFeatureLevel: 370); + async.elapse(kLocalEchoDebounceDuration); + check(store.outboxMessages).values.single + .conversation.isA().topic.equals(eg.t('')); + })); + + test('legacy: when sending to "(no topic)", process topic like the server does when creating outbox message', () => awaitFakeAsync((async) async { + await prepareOutboxMessage( + destination: StreamDestination(stream.streamId, TopicName('(no topic)')), + zulipFeatureLevel: 369); + async.elapse(kLocalEchoDebounceDuration); + check(store.outboxMessages).values.single + .conversation.isA().topic.equals(eg.t('(no topic)')); + })); + + test('set timestamp to now when creating outbox messages', () => awaitFakeAsync( + initialTime: eg.timeInPast, + (async) async { + await prepareOutboxMessage(); + check(store.outboxMessages).values.single + .timestamp.equals(eg.utcTimestamp(eg.timeInPast)); + }, + )); + }); + + test('takeOutboxMessage', () async { + final stream = eg.stream(); + await prepare(stream: stream); + await prepareMessages([]); + + for (int i = 0; i < 10; i++) { + connection.prepare(apiException: eg.apiBadRequest()); + await check(store.sendMessage( + destination: StreamDestination(stream.streamId, eg.t('topic')), + content: 'content')).throws(); + } + + final localMessageIds = store.outboxMessages.keys.toList(); + store.takeOutboxMessage(localMessageIds.removeAt(5)); + check(store.outboxMessages).keys.deepEquals(localMessageIds); + }); + group('reconcileMessages', () { test('from empty', () async { await prepare(); diff --git a/test/model/narrow_test.dart b/test/model/narrow_test.dart index 06c82ed117..9d68873670 100644 --- a/test/model/narrow_test.dart +++ b/test/model/narrow_test.dart @@ -2,38 +2,37 @@ import 'package:checks/checks.dart'; import 'package:test/scaffolding.dart'; import 'package:zulip/api/model/model.dart'; +import 'package:zulip/model/message.dart'; import 'package:zulip/model/narrow.dart'; import '../example_data.dart' as eg; import 'narrow_checks.dart'; -/// A [MessageBase] subclass for testing. -// TODO(#1441): switch to outbox-messages instead -sealed class _TestMessage extends MessageBase { - @override - final int? id = null; - - _TestMessage() : super(senderId: eg.selfUser.userId, timestamp: 123456789); -} - -class _TestStreamMessage extends _TestMessage { - @override - final StreamConversation conversation; - - _TestStreamMessage({required ZulipStream stream, required String topic}) - : conversation = StreamConversation( - stream.streamId, TopicName(topic), displayRecipient: null); -} - -class _TestDmMessage extends _TestMessage { - @override - final DmConversation conversation; - - _TestDmMessage({required List allRecipientIds}) - : conversation = DmConversation(allRecipientIds: allRecipientIds); -} - void main() { + int nextLocalMessageId = 1; + + StreamOutboxMessage streamOutboxMessage({ + required ZulipStream stream, + required String topic, + }) { + return OutboxMessage.fromConversation( + StreamConversation( + stream.streamId, TopicName(topic), displayRecipient: null), + localMessageId: nextLocalMessageId++, + selfUserId: eg.selfUser.userId, + timestamp: 123456789, + contentMarkdown: 'content') as StreamOutboxMessage; + } + + DmOutboxMessage dmOutboxMessage({required List allRecipientIds}) { + return OutboxMessage.fromConversation( + DmConversation(allRecipientIds: allRecipientIds), + localMessageId: nextLocalMessageId++, + selfUserId: allRecipientIds[0], + timestamp: 123456789, + contentMarkdown: 'content') as DmOutboxMessage; + } + group('SendableNarrow', () { test('ofMessage: stream message', () { final message = eg.streamMessage(); @@ -61,11 +60,11 @@ void main() { eg.streamMessage(stream: stream, topic: 'topic'))).isTrue(); check(narrow.containsMessage( - _TestDmMessage(allRecipientIds: [1]))).isFalse(); + dmOutboxMessage(allRecipientIds: [1]))).isFalse(); check(narrow.containsMessage( - _TestStreamMessage(stream: otherStream, topic: 'topic'))).isFalse(); + streamOutboxMessage(stream: otherStream, topic: 'topic'))).isFalse(); check(narrow.containsMessage( - _TestStreamMessage(stream: stream, topic: 'topic'))).isTrue(); + streamOutboxMessage(stream: stream, topic: 'topic'))).isTrue(); }); }); @@ -91,13 +90,13 @@ void main() { eg.streamMessage(stream: stream, topic: 'topic'))).isTrue(); check(narrow.containsMessage( - _TestDmMessage(allRecipientIds: [1]))).isFalse(); + dmOutboxMessage(allRecipientIds: [1]))).isFalse(); check(narrow.containsMessage( - _TestStreamMessage(stream: otherStream, topic: 'topic'))).isFalse(); + streamOutboxMessage(stream: otherStream, topic: 'topic'))).isFalse(); check(narrow.containsMessage( - _TestStreamMessage(stream: stream, topic: 'topic2'))).isFalse(); + streamOutboxMessage(stream: stream, topic: 'topic2'))).isFalse(); check(narrow.containsMessage( - _TestStreamMessage(stream: stream, topic: 'topic'))).isTrue(); + streamOutboxMessage(stream: stream, topic: 'topic'))).isTrue(); }); }); @@ -223,13 +222,13 @@ void main() { final narrow = DmNarrow(allRecipientIds: [1, 2], selfUserId: 2); check(narrow.containsMessage( - _TestStreamMessage(stream: eg.stream(), topic: 'topic'))).isFalse(); + streamOutboxMessage(stream: eg.stream(), topic: 'topic'))).isFalse(); check(narrow.containsMessage( - _TestDmMessage(allRecipientIds: [2]))).isFalse(); + dmOutboxMessage(allRecipientIds: [2]))).isFalse(); check(narrow.containsMessage( - _TestDmMessage(allRecipientIds: [2, 3]))).isFalse(); + dmOutboxMessage(allRecipientIds: [2, 3]))).isFalse(); check(narrow.containsMessage( - _TestDmMessage(allRecipientIds: [1, 2]))).isTrue(); + dmOutboxMessage(allRecipientIds: [1, 2]))).isTrue(); }); }); @@ -245,9 +244,9 @@ void main() { eg.streamMessage(flags: [MessageFlag.wildcardMentioned]))).isTrue(); check(narrow.containsMessage( - _TestStreamMessage(stream: eg.stream(), topic: 'topic'))).isFalse(); + streamOutboxMessage(stream: eg.stream(), topic: 'topic'))).isFalse(); check(narrow.containsMessage( - _TestDmMessage(allRecipientIds: [eg.selfUser.userId]))).isFalse(); + dmOutboxMessage(allRecipientIds: [eg.selfUser.userId]))).isFalse(); }); }); @@ -261,9 +260,9 @@ void main() { eg.streamMessage(flags:[MessageFlag.starred]))).isTrue(); check(narrow.containsMessage( - _TestStreamMessage(stream: eg.stream(), topic: 'topic'))).isFalse(); + streamOutboxMessage(stream: eg.stream(), topic: 'topic'))).isFalse(); check(narrow.containsMessage( - _TestDmMessage(allRecipientIds: [eg.selfUser.userId]))).isFalse(); + dmOutboxMessage(allRecipientIds: [eg.selfUser.userId]))).isFalse(); }); }); } diff --git a/test/model/store_test.dart b/test/model/store_test.dart index eba1505747..0b303b53e2 100644 --- a/test/model/store_test.dart +++ b/test/model/store_test.dart @@ -569,7 +569,8 @@ void main() { group('PerAccountStore.sendMessage', () { test('smoke', () async { - final store = eg.store(); + final store = eg.store(initialSnapshot: eg.initialSnapshot( + queueId: 'fb67bf8a-c031-47cc-84cf-ed80accacda8')); final connection = store.connection as FakeApiConnection; final stream = eg.stream(); connection.prepare(json: SendMessageResult(id: 12345).toJson()); @@ -585,6 +586,8 @@ void main() { 'topic': 'world', 'content': 'hello', 'read_by_sender': 'true', + 'queue_id': 'fb67bf8a-c031-47cc-84cf-ed80accacda8', + 'local_id': store.outboxMessages.keys.single.toString(), }); }); }); diff --git a/test/widgets/compose_box_test.dart b/test/widgets/compose_box_test.dart index 679f4de190..11467cea7d 100644 --- a/test/widgets/compose_box_test.dart +++ b/test/widgets/compose_box_test.dart @@ -15,6 +15,7 @@ import 'package:zulip/api/model/model.dart'; import 'package:zulip/api/route/channels.dart'; import 'package:zulip/api/route/messages.dart'; import 'package:zulip/model/localizations.dart'; +import 'package:zulip/model/message.dart'; import 'package:zulip/model/narrow.dart'; import 'package:zulip/model/store.dart'; import 'package:zulip/model/typing_status.dart'; @@ -295,6 +296,8 @@ void main() { Future prepareWithContent(WidgetTester tester, String content) async { TypingNotifier.debugEnable = false; addTearDown(TypingNotifier.debugReset); + MessageStoreImpl.debugOutboxEnable = false; + addTearDown(MessageStoreImpl.debugReset); final narrow = ChannelNarrow(channel.streamId); await prepareComposeBox(tester, narrow: narrow, streams: [channel]); @@ -332,6 +335,8 @@ void main() { Future prepareWithTopic(WidgetTester tester, String topic) async { TypingNotifier.debugEnable = false; addTearDown(TypingNotifier.debugReset); + MessageStoreImpl.debugOutboxEnable = false; + addTearDown(MessageStoreImpl.debugReset); final narrow = ChannelNarrow(channel.streamId); await prepareComposeBox(tester, narrow: narrow, streams: [channel]); @@ -723,6 +728,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 checkStartTyping(tester, narrow); @@ -829,6 +836,8 @@ void main() { }) async { TypingNotifier.debugEnable = false; addTearDown(TypingNotifier.debugReset); + MessageStoreImpl.debugOutboxEnable = false; + addTearDown(MessageStoreImpl.debugReset); final zulipLocalizations = GlobalLocalizations.zulipLocalizations; await prepareComposeBox(tester, narrow: eg.topicNarrow(123, 'some topic'), @@ -883,6 +892,8 @@ void main() { }) async { TypingNotifier.debugEnable = false; addTearDown(TypingNotifier.debugReset); + MessageStoreImpl.debugOutboxEnable = false; + addTearDown(MessageStoreImpl.debugReset); channel = eg.stream(); final narrow = ChannelNarrow(channel.streamId); @@ -1419,6 +1430,8 @@ void main() { int msgIdInNarrow(Narrow narrow) => msgInNarrow(narrow).id; Future prepareEditMessage(WidgetTester tester, {required Narrow narrow}) async { + MessageStoreImpl.debugOutboxEnable = false; + addTearDown(MessageStoreImpl.debugReset); await prepareComposeBox(tester, narrow: narrow, streams: [channel]); diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index df05b4f0cc..88c4cdb64b 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -943,7 +943,8 @@ void main() { connection.prepare(json: SendMessageResult(id: 1).toJson()); await tester.tap(find.byIcon(ZulipIcons.send)); - await tester.pump(); + await tester.pump(Duration.zero); + final localMessageId = store.outboxMessages.keys.single; check(connection.lastRequest).isA() ..method.equals('POST') ..url.path.equals('/api/v1/messages') @@ -952,8 +953,12 @@ void main() { 'to': '${otherChannel.streamId}', 'topic': 'new topic', 'content': 'Some text', - 'read_by_sender': 'true'}); - await tester.pumpAndSettle(); + 'read_by_sender': 'true', + 'queue_id': store.queueId, + 'local_id': localMessageId.toString()}); + // Remove the outbox message and its timers created when sending message. + await store.handleEvent( + eg.messageEvent(message, localMessageId: localMessageId)); }); testWidgets('Move to narrow with existing messages', (tester) async { From 61e343b9d8ca35b207a53e14789fb3c8e92267ca Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Wed, 21 May 2025 16:49:28 -0400 Subject: [PATCH 3/3] message: Avoid double-sends after send-message request succeeds This implements the waitPeriodExpired -> waiting state transition. GitHub discussion: https://github.com/zulip/zulip-flutter/pull/1472#discussion_r2099285217 --- lib/model/message.dart | 28 ++++++++++++++++++++-------- test/model/message_test.dart | 27 +++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 8 deletions(-) diff --git a/lib/model/message.dart b/lib/model/message.dart index 3fffdfc4a4..719d0704f6 100644 --- a/lib/model/message.dart +++ b/lib/model/message.dart @@ -530,12 +530,14 @@ const kSendMessageOfferRestoreWaitPeriod = Duration(seconds: 10); // TODO(#1441 /// [MessageStore.sendMessage] call and before its eventual deletion. /// /// ``` -/// Got an [ApiRequestException]. -/// ┌──────┬──────────┬─────────────► failed -/// (create) │ │ │ │ -/// └► hidden waiting waitPeriodExpired ──┴──────────────► (delete) -/// │ ▲ │ ▲ User restores -/// └──────┘ └──────┘ the draft. +/// Got an [ApiRequestException]. +/// ┌──────┬────────────────────────────┬──────────► failed +/// │ │ │ │ +/// │ │ [sendMessage] │ │ +/// (create) │ │ request succeeds. │ │ +/// └► hidden waiting ◄─────────────── waitPeriodExpired ──┴─────► (delete) +/// │ ▲ │ ▲ User restores +/// └──────┘ └─────────────────────┘ the draft. /// Debounce [sendMessage] request /// timed out. not finished when /// wait period timed out. @@ -559,7 +561,8 @@ enum OutboxMessageState { /// outbox message is shown to the user. /// /// This state can be reached after staying in [hidden] for - /// [kLocalEchoDebounceDuration]. + /// [kLocalEchoDebounceDuration], or when the request succeeds after the + /// outbox message reaches [OutboxMessageState.waitPeriodExpired]. waiting, /// The [sendMessage] HTTP request did not finish in time and the user is @@ -717,7 +720,8 @@ mixin _OutboxMessageStore on PerAccountStoreBase { final isStateTransitionValid = switch (newState) { OutboxMessageState.hidden => false, OutboxMessageState.waiting => - oldState == OutboxMessageState.hidden, + oldState == OutboxMessageState.hidden + || oldState == OutboxMessageState.waitPeriodExpired, OutboxMessageState.waitPeriodExpired => oldState == OutboxMessageState.waiting, OutboxMessageState.failed => @@ -803,6 +807,14 @@ mixin _OutboxMessageStore on PerAccountStoreBase { // Cancel the timer that would have had us start presuming that the // send might have failed. _outboxMessageWaitPeriodTimers.remove(localMessageId)?.cancel(); + if (_outboxMessages[localMessageId]!.state + == OutboxMessageState.waitPeriodExpired) { + // The user was offered to restore the message since the request did not + // complete for a while. Since the request was successful, we expect the + // message event to arrive eventually. Stop inviting the the user to + // retry, to avoid double-sends. + _updateOutboxMessage(localMessageId, newState: OutboxMessageState.waiting); + } } TopicName _processTopicLikeServer(TopicName topic, { diff --git a/test/model/message_test.dart b/test/model/message_test.dart index 4f8183d6d4..7dff077b1d 100644 --- a/test/model/message_test.dart +++ b/test/model/message_test.dart @@ -217,6 +217,33 @@ void main() { await check(outboxMessageFailFuture).throws(); })); + test('waiting -> waitPeriodExpired -> waiting and never return to waitPeriodExpired', () => awaitFakeAsync((async) async { + await prepare(stream: stream); + await prepareMessages([eg.streamMessage(stream: stream)]); + // Set up a [sendMessage] request that succeeds after enough delay, + // for the outbox message to reach the waitPeriodExpired state. + // TODO extract helper to add prepare an outbox message with a delayed + // successful [sendMessage] request if we have more tests like this + connection.prepare(json: SendMessageResult(id: 1).toJson(), + delay: kSendMessageOfferRestoreWaitPeriod + Duration(seconds: 1)); + final future = store.sendMessage( + destination: streamDestination, content: 'content'); + async.elapse(kSendMessageOfferRestoreWaitPeriod); + checkState().equals(OutboxMessageState.waitPeriodExpired); + + // Wait till the [sendMessage] request succeeds. + await future; + checkState().equals(OutboxMessageState.waiting); + + // Wait till we reach at least [kSendMessageOfferRestoreWaitPeriod] after + // returning to the waiting state. + async.elapse(kSendMessageOfferRestoreWaitPeriod); + async.flushTimers(); + // The outbox message should stay in the waiting state; + // it should not transition to waitPeriodExpired. + checkState().equals(OutboxMessageState.waiting); + })); + group('… -> failed', () { test('hidden -> failed', () => awaitFakeAsync((async) async { await prepareOutboxMessageToFailAfterDelay(Duration.zero);