Handle moved messages for unreads #1311

PIG208 · 2025-01-27T23:16:50Z

Work toward #901

I prioritized updating unreads data for moved messages because it is more visible after supporting the resolve/unresolve topic actions. Updating recent senders data will be a follow-up to this.

chrisbobbe · 2025-02-08T01:52:05Z

Actually @gnprice, I'd appreciate any thoughts you have from a skim of the first three commits:

model [nfc]: Add PropagateMode.fromRawString
api [nfc]: Extract and parse UpdateMessageMoveData from UpdateMessageEvent
api: Parse UpdateMessageMoveData with inter-related constraints

before I start my review, if that's OK; I think you designed/implemented similar logic in zulip-mobile.

gnprice · 2025-02-19T01:57:38Z

Didn't catch the GitHub notification for this earlier, sorry — thanks for the ping in chat today.

The general strategy in those commits looks good. The high-level comments I have are:

Here in the API parsing code, when the response is malformed, instead of returning null, let's just throw — for example if something shouldn't be null, then just the ! operator is enough.

That's what already happens for all kinds of other ways it can be malformed, like if json['stream_id'] is a string value (instead of a number or absent/null). Such exceptions get turned into MalformedServerResponseException by the generic code in ApiConnection.send, and in the case of events those get handled appropriately by the long-poll loop. (They cause a reload of the store, because if there's an event we weren't able to handle then we may no longer have accurate state.)
Let's have an invariant that we don't wind up with an UpdateMessageMoveData that doesn't represent a move: i.e. where the old and new channel ID are equal and the old and new topic are equal. In our code that updates data structures we might rely on that assumption (for example so that we're not mutating the same data structure twice concurrently as both the source and target of a move).

That can be enforced by both an assert on the constructor, and the try-parse method either throwing or returning null before getting to the constructor in that case. (Compare the zulip-mobile logic quoted below.)
This logic about in what circumstances the various "orig" and "new" fields might be null, or are required to be present, feels pretty unsatisfying. Even though it's meant to accurately reflect guarantees that are in the API docs (https://zulip.com/api/get-events#update_message), and probably it does, it doesn't feel solid.

In particular, facts like this one feel like quirks:

orig_subject (aka origTopic) is documented to be present when either the stream or topic changed.

and are asymmetric between the topic and the channel fields. Indeed this sort of fact has changed in the past (origStreamId aka stream_id became more widely present in Zulip Server 5), and just last year we had to edit the logic because the docs were wrong (020bfcb) about a fact of this kind which we had relied on.

This logic in the zulip-mobile version feels more robust, because it's based on natural facts about the semantics the event is about:
```
if (new_topic === orig_topic && new_stream_id === orig_stream_id) {
  // Stream and topic didn't change.
  return null;
```
Perhaps better yet, though: we could just look to see if propagate_mode is present. (The zulip-mobile version didn't consider propagate_mode as part of this logic.) That's something that is necessary if there was a move, and clearly meaningless if there wasn't — so it seems like a very crisp and reliable indicator of whether there was a move. (And the API docs agree that it, too, is present just if there was a move.)

gnprice · 2025-02-19T23:20:46Z

(So probably the right next steps are: @PIG208 go ahead and act on the feedback above, and then after that revision @chrisbobbe will do maintainer review.)

This drops the separate if-statements in favor of null-checks and tries to rely on the natural semantics of the API. See Greg's comment on this: zulip#1311 (comment) Signed-off-by: Zixuan James Li <[email protected]>

PIG208 · 2025-02-20T04:03:23Z

Thanks for the review! The PR has been updated.

This drops the separate if-statements in favor of null-checks and tries to rely on the natural semantics of the API. This drops an expcetion that would have been thrown when propagate mode is not present but the topic name/stream id changes. See Greg's comment on this: zulip#1311 (comment) Signed-off-by: Zixuan James Li <[email protected]>

chrisbobbe

Thanks, I'm excited for more correctness in the Unreads model! Small comments below.

chrisbobbe · 2025-02-21T21:45:41Z

test/model/message_list_test.dart

        await store.handleEvent(eg.updateMessageEventMoveFrom(
          origMessages: otherChannelMovedMessages,
-          newStreamId: otherStream.streamId,
+          newStreamId: thirdStream.streamId,
        ));


What do you think about adding an assert to eg.updateMessageEventMoveFrom, to catch invalid input?:

final origStreamId = origMessage.streamId; final origTopic = origMessage.topic; assert(() { final streamChanged = newStreamId != null && newStreamId != origStreamId; final topicChanged = newTopic != null && newTopic != origTopic; return streamChanged || topicChanged; }());

It would be helpful before we pull out UpdateMessageMoveData, which actually helped me discover this issue in the first place. Having the assertions and check in one place (UpdateMessageMoveData) appears to be easier to maintain.

chrisbobbe · 2025-02-21T21:52:06Z

test/api/model/events_test.dart

    };

-    test('stream_id -> origStreamId', () {
+    test('stream_id -> origStreamId, subject = orig_subject', () {


events test: Make test data more realistic

Could please you explain this commit a bit more? I don't understand the changes to the test descriptions. Their current descriptions are "A -> B" pairs, where A is input and B is expected output. I don't understand what the added pair "subject = orig_subject" means, and it doesn't follow that pattern. If I look at 'orig_subject' and 'subject' in the input, I see they're not equal; I guess that's not what the "=" is for, then.

It's meant to represent that the topic does not change in this move. Changing it to a short phrase should help make it clear.

This new check seems like it'd be clearest as a new separate test case. I'm not sure how it relates to the existing test, or really what it's meant to check.

chrisbobbe · 2025-02-21T22:42:50Z

lib/api/model/events.dart

+  static Object? _readMoveData(Map<Object?, Object?> json, String key) {
+    // Parsing [UpdateMessageMoveData] requires `json`, not the default `json[key]`.
+    return json as Object?;
+  }


At this point we know json is a Map<String, dynamic>, because it was passed as the argument to UpdateMessageEvent.fromJson, which has that type. I think it would be neater to have _readMoveData encapsulate this, having Map<String, dynamic> for its return type instead of Object?.

For readValue, we're asked to implement a function that takes any Map, which is why we can't say Map<String, dynamic> json for _readMoveData. But that's fine; we can still have an assert:

--- lib/api/model/events.dart +++ lib/api/model/events.dart @@ -723,8 +723,7 @@ class UpdateMessageMoveData { /// [UpdateMessageEvent]. /// /// Throws an error if the data is malformed. - factory UpdateMessageMoveData.fromJson(Object? json) { - json as Map<String, Object?>; + factory UpdateMessageMoveData.fromJson(Map<String, dynamic> json) { final origStreamId = (json['stream_id'] as num?)?.toInt(); final newStreamId = (json['new_stream_id'] as num?)?.toInt(); final propagateModeString = json['propagate_mode'] as String?; @@ -793,9 +792,10 @@ class UpdateMessageEvent extends Event { required this.isMeMessage, }); - static Object? _readMoveData(Map<Object?, Object?> json, String key) { + static Map<String, dynamic> _readMoveData(Map<dynamic, dynamic> json, String key) { // Parsing [UpdateMessageMoveData] requires `json`, not the default `json[key]`. - return json as Object?; + assert(json is Map<String, dynamic>); // value came through `fromJson` with this type + return json as Map<String, dynamic>; } factory UpdateMessageEvent.fromJson(Map<String, dynamic> json) =>

I know that the assert is redundant with the json as Map<String, dynamic>. My thought is to be clear that this is an invariant we own in our code; it's not about what we expect from the server (which is the case for most casts here, like json['propagate_mode'] as String?).

What do you think?

chrisbobbe · 2025-02-21T23:01:48Z

lib/api/model/events.dart

+/// Data structure representing a message move.
+class UpdateMessageMoveData {


api [nfc]: Extract and parse UpdateMessageMoveData from UpdateMessageEvent

At this commit the UpdateMessageMoveData dartdoc isn't accurate, because we still instantiate the class when there was no move, or when the fields have the correct types but are incoherent about whether there was a move (e.g. propagate_mode present but the stream/topic fields all absent).

How about:

/// Data structure holding the fields about a message move.

(or similar), until the "representing a message move" becomes accurate in a later commit.

chrisbobbe · 2025-02-21T23:30:00Z

lib/api/model/events.dart

+    if (propagateMode == null) {
      // There was no move.


api: Simplify move data parsing This drops the separate if-statements in favor of null-checks and tries to rely on the natural semantics of the API. This drops an expcetion that would have been thrown when propagate mode is not present but the topic name/stream id changes. See Greg's comment on this: https://github.com/zulip/zulip-flutter/pull/1311#issuecomment-2667327271 Signed-off-by: Zixuan James Li <[email protected]>

This new implementation follows the documented API, right? From that linked comment from Greg:

Perhaps better yet, though: we could just look to see if propagate_mode is present. (The zulip-mobile version didn't consider propagate_mode as part of this logic.) That's something that is necessary if there was a move, and clearly meaningless if there wasn't — so it seems like a very crisp and reliable indicator of whether there was a move. (And the API docs agree that it, too, is present just if there was a move.)

So let's explain it more confidently than "tries to rely on the natural semantics of the API". 🙂 Can just briefly say why we're not directly transcribing zulip-mobile code, but that this way is valid and more crisp, and link to Greg's comment.

Also how about squashing this commit into the previous:

api: Parse UpdateMessageMoveData with inter-related constraints

and perhaps even squash that into the one before:

api [nfc]: Extract and parse UpdateMessageMoveData from UpdateMessageEvent

For this part:

This drops an expcetion that would have been thrown when propagate mode is not present but the topic name/stream id changes.

That would be easy to add back in, right, in the if (propagateMode == null)? Just that we'd throw if any of the four stream / topic fields are present.

That would be easy to add back in, right, in the if (propagateMode == null)? Just that we'd throw if any of the four stream / topic fields are present.

When there isn't a move, some of those four fields may still be present and it's complicated what exact subset is expected. (See "feel like quirks" and "changed in the past" in this comment: #1311 (comment)) So such a check would have to be more complicated than that.

We can have a check like:

// pseudo representation of the types of the fields TopicName? origTopic = ...; int? origStreamId = ...; final newTopic ??= origTopic; final newStreamId ??= origStreamId; if (propagateMode == null) { if (origTopic != newTopic || origStreamId != newStreamId) { throw FormatException('move but no propagateMode'); // or "maybe move but no propagateMode" } // There was no move. return null; }

This still throws if we have origTopic == null or origStreamId == null with a non-null newTopic or newStreamId, but that's malformed data anyway.

It is the negation of origStreamId == newStreamId && origTopic == newTopic, which is used later in this method.

Yeah, that could be fine if it doesn't involve making the other logic any more complicated to support it.

chrisbobbe · 2025-02-22T00:13:55Z

lib/model/unreads.dart

-    if (topics == null) return;
+    if (topics == null) return QueueList();


The only thing we do with this is an .isEmpty check, right? How about returning null instead of creating a new QueueList (seems more efficient).

chrisbobbe · 2025-02-22T00:16:12Z

lib/model/unreads.dart

+  bool _handleMessageMove(UpdateMessageEvent event) {
+    final messageMove = event.moveData;
+    if (messageMove == null) {
+      // No moved messages or malformed event.


I think the "or malformed event" part is from an old revision—we throw if it's malformed, right?

chrisbobbe · 2025-02-22T00:21:32Z

lib/model/unreads.dart

+    final topics = streams[messageMove.origStreamId];
+    if (topics == null || topics[messageMove.origTopic] == null) {
+      // No known unreads affected by move; nothing to do.
+      return false;
+    }


This is redundant with the early returns at the top of _removeAllInStreamTopic, right?

chrisbobbe · 2025-02-22T00:22:33Z

lib/model/unreads.dart

+      // No moved messages or malformed event.
+      return false;
+    }
+


Could say

final UpdateMessageMoveData( :origStreamId, :newStreamId, :origTopic, :newTopic) = messageMove;

to avoid repetition of messageMove. several times.

chrisbobbe · 2025-02-22T00:27:28Z

lib/model/unreads.dart

-    // TODO(#901) handle moved messages
+    madeAnyUpdate |= _handleMessageMove(event);


unreads: Handle updates when there are moved messages

The commit message can get a "fixes" line. 🎉

We will get there soon! #901 also requires updates to recent senders data model.

Oh oops! Indeed!

This data structure encapsulates some checks so that we can make all fields non-nullable, with reasonable fallback values. As of writing, we do not use origStreamId (a.k.a.: 'stream_id') when there was no message move, even though it is present if there were content edits This makes dropping 'stream_id' when parsing `moveData` into `null` acceptable for now. This also allows us to drop the `assert`'s and "TODO(log)"'s, because the stacktrace we need can be retrieved after throwing these `FormatException`'s. This is similar to zulip-mobile code for parsing move data. The main difference is that we check the value of `propagate_mode`, which is documented to be present on message moves. With this single indicator, the logic is crisp for ruling out non-move message update events. See Greg's comment on this: zulip#1311 (comment) Signed-off-by: Zixuan James Li <[email protected]>

chrisbobbe · 2025-02-25T00:39:20Z

It turns out there's a move-message edge case that we'll need to handle, either here or as a followup issue.

It's this TODO in Unreads.handleUpdateMessageEvent:

    // We assume this event can't signal a change in a message's 'read' flag.
    // TODO can it actually though, when it's about messages being moved into an
    //   unsubscribed stream?
    //   https://chat.zulip.org/#narrow/stream/378-api-design/topic/mark-as-read.20events.20with.20message.20moves.3F/near/1639957

The conclusion from an API-design discussion is that clients need to check if the messages were moved into a channel that is unsubscribed, and if so, drop them from unreads, since we won't be getting a separate event about that. (This is currently undocumented on update_message but should be soon.)

PIG208 · 2025-02-25T03:48:10Z

Just read the discussion. Yeah, it seems straightforward enough to include in the main implementation commit. I removed the TODO and kept a link referring to that discussion at where this is handled.

From my understanding, the isRead flag gets stale and the claim that " We assume this event can't signal a change in a message's 'read' flag." holds, so there is nothing else to do at the TODO site; we handle this in _handleMessageMove.

gnprice

Thanks @PIG208 for building this, and @chrisbobbe for the previous reviews!

Comments below. For this round I've read all but the tests of the last/main commit.

gnprice · 2025-03-04T02:59:01Z

lib/api/model/model.dart

+  ///
+  /// Example:
+  ///   'change_one' -> PropagateMode.changeOne
+  static PropagateMode fromRawString(String raw) => _byRawString[raw]!;


nit: use "api" for prefix

model [nfc]: Add PropagateMode.fromRawString

gnprice · 2025-03-04T03:00:58Z

test/model/message_list_test.dart

          origMessages: otherChannelMovedMessages,
-          newStreamId: otherStream.streamId,
+          newStreamId: thirdStream.streamId,


Oops. Glad the new structure catches this bug — that's a good sign.

gnprice · 2025-03-04T03:05:56Z

test/api/model/events_test.dart

@@ -99,26 +99,120 @@ void main() {
      'message_ids': [message.id],
      'flags': <String>[],
      'edit_timestamp': 1718741351,
-      'stream_id': eg.stream().streamId,


Both orig_subject and stream_id are documented to be present on message moves even if the message is not moved to a new topic/channel, respectively;

That describes a reason why stream_id should remain present on this "base JSON" data. If all the events this is used for building are going to have stream_id, then having it here allows those tests to have some boring value there even when it isn't specifically relevant to what the test is about.

Similarly this reasoning means that this map should gain orig_subject too.

gnprice · 2025-03-04T03:07:10Z

test/api/model/events_test.dart

    test('orig_subject -> origTopic, subject -> newTopic', () {
      check(Event.fromJson({ ...baseJson,
+        'stream_id': 1,
+        'new_stream_id': null,


Neither of these are part of what this test case is about, so it's best for it to not have to mention them. Describing the data that needs to be there in order for things to be well-formed, but whose details are irrelevant to the given test, is what shared helpers like baseJson (and more broadly example_data.dart) are for.

gnprice · 2025-03-04T03:09:14Z

test/api/model/events_test.dart

        'orig_subject': 'foo',
        'subject': 'bar',
-      }) as UpdateMessageEvent)
+        'propagate_mode': 'change_all',


To handle items like this one that are present for all moves, but none of the non-moves, it may be cleanest to introduce shared locals like baseMoveJson and potentially baseEditJson. That way baseMoveJson can have this item in it.

gnprice · 2025-03-04T04:34:55Z

test/api/model/events_checks.dart

  Subject<String?> get origContent => has((e) => e.origContent, 'origContent');
  Subject<String?> get origRenderedContent => has((e) => e.origRenderedContent, 'origRenderedContent');
  Subject<String?> get content => has((e) => e.content, 'content');
  Subject<String?> get renderedContent => has((e) => e.renderedContent, 'renderedContent');
  Subject<bool?> get isMeMessage => has((e) => e.isMeMessage, 'isMeMessage');
 }

+
+extension UpdateMessageMoveDataChecks on Subject<UpdateMessageMoveData> {


nit: match ordering

Ordered UpdateMessageMoveData after UpdateMessageEvent instead (similar to how UpdateMessageFlagsMessageDetail comes after UpdateMessageFlagsRemoveEvent).

gnprice · 2025-03-04T04:37:02Z

test/api/model/events_test.dart

+    test('orig_subject -> newTopic if no subject', () {
+      check(Event.fromJson({ ...baseJson,
+        'stream_id': 1,
+        'new_stream_id': 2,
+        'orig_subject': 'foo',
+        'subject': null,
+        'propagate_mode': 'change_all',
+      })).isA<UpdateMessageEvent>().moveData.isNotNull()
+        ..origTopic.equals(const TopicName('foo'))
+        ..newTopic.equals(const TopicName('foo'));


The "->" in the names of other tests above are all about fields which we've renamed relative to the API. This is testing something that feels pretty different — the use of the value of origTopic as a fallback for newTopic when the latter isn't explicitly given. So let's give it a distinct name.

It's also nonobvious that this is a real scenario — it has the look of something one might make up for just malformed data, with a field randomly gone missing. (And we don't ordinarily cater to or test such data.) But the point is that this is what the server actually does for a move between channels that didn't change the topic name.

So perhaps:

test('new channel, same topic: fill in newTopic', () { // The server omits 'subject' in this situation.

gnprice · 2025-03-04T04:47:56Z

test/api/model/events_test.dart

+        ..newTopic.equals(const TopicName('foo'));
+    });
+
+    test('stream_id -> newStreamId if no new_stream_id', () {


gnprice · 2025-03-04T04:58:03Z

lib/model/unreads.dart

-  void _removeAllInStreamTopic(Set<int> incomingMessageIds, int streamId, TopicName topic) {
+  QueueList<int>? _removeAllInStreamTopic(Set<int> incomingMessageIds, int streamId, TopicName topic) {


This should get a bit of dartdoc saying what the return value means — I don't think it's obvious what it would mean.

gnprice · 2025-03-04T05:17:23Z

lib/model/unreads.dart

+    final removedMessageIds = QueueList<int>();
+    messageIds.removeWhere((id) {
+      if (incomingMessageIds.contains(id)) {
+        removedMessageIds.add(id);


In the case where the whole conversation got moved — a very common case — this will end up unnecessarily making a new copy of the list. Let's adjust so that in that case it can just return the existing list, without copying it.

Even more common: this code path also occurs every time the user marks some messages as read. That case doesn't need a list of the removed message IDs at all, so let's have it avoid making such a copy even when it's only some of the messages. (Which is common when reading on web, or reading in this app in the future after #81.)

That probably means these two cases should be separate methods, since they basically want different return types. It's OK that they'll effectively duplicate each other's first and last few lines.

I think shortcuting to removing all unreads for the original topic when propagate mode is 'change_all' is subject to the message fetch race, which will cause some unreads to be incorrectly removed.

Or maybe not. The unreads data model does not actively fetch messages; it deals with events.

Yeah, to clarify, I don't mean to say we should start interpreting propagateMode here. (This model should be responding to fetches, and it's a bug that it doesn't yet: #649. So the fetch/event race will apply to it in the future.) I'm just referring to the case where all the message IDs we have for the conversation turn out to be in the list of message IDs to move.

It may be cleanest to replace removeWhere with an explicit loop. If you click through to its definition, you'll see an example of what that loop can look like. (And we don't need complications like ConcurrentModificationError, because unlike the general removeWhere this code isn't calling out to arbitrary other code.)

Because of this simplification, it is easier to make sense of the constraints that apply to the inter-related fields. This also allows us to drop the `assert`'s and "TODO(log)"'s, because the stacktrace we need can be retrieved after throwing these `FormatException`'s. This is similar to zulip-mobile code for parsing move data. The main difference is that we now also check the value of `propagate_mode`, which is documented to be present on message moves. See Greg's comment on this: zulip#1311 (comment) Signed-off-by: Zixuan James Li <[email protected]>

PIG208 · 2025-03-06T04:16:51Z

Thanks for the review! This should be ready again. I made a new revision of tests in the last commit.

PIG208 · 2025-03-06T22:46:03Z

I'm planning to update the test cases for the main implementation again, after having a call with Greg.

PIG208 · 2025-03-07T02:32:16Z

This should be ready now.

gnprice

Thanks for the revision! Comments below. In this round I've now read the whole thing.

gnprice · 2025-03-10T20:24:27Z

lib/model/message.dart


-    final wasResolveOrUnresolve = (newStreamId == null


message [nfc]: Remove redundant parentheses

This change is trivial and self-explanatory, so it can just get squashed into the substantive change that's touchinig the same code.

gnprice · 2025-03-10T20:44:54Z

lib/model/message.dart

-    final wasResolveOrUnresolve = newStreamId == null
-      && MessageEditState.topicMoveWasResolveOrUnresolve(origTopic, newTopic!);
+    final wasResolveOrUnresolve = origStreamId == newStreamId
+      && MessageEditState.topicMoveWasResolveOrUnresolve(origTopic!, newTopic!);


message: Enhance message move checks Because of this simplification, it is easier to make sense of the constraints that apply to the inter-related fields.

This commit doesn't seem to yet make sense on its own.

For example, the ! operators here don't seem justified. Why are these non-null?

I think the answer is "because they're documented to both be present if there was a move". That needs to be more explicit, though — otherwise it just looks like we potentially have a case where we crash. (When the logic lives inside our API parsing, it no longer needs to be so explicit because that's routine for that parsing code.)

In fact let's avoid throwing here in this handle-event method for malformed events; instead let's ignore such events (like the existing code does), and reserve the throwing for the parsing code. That's why at #1311 (comment) I requested:

In particular it'd be helpful if that second commit is NFC for debug builds — i.e. if its only functional change is that in some conditions where the old code would assert, the new code will throw while parsing.

That means that after the first commit, while this logic still lives in this handle-event method, it still asserts for malformed events rather than (in non-debug builds) throwing.

gnprice · 2025-03-10T20:57:10Z

lib/model/unreads.dart

+    final UpdateMessageMoveData(
+      :origStreamId, :newStreamId, :propagateMode, :origTopic, :newTopic,
+    ) = event.moveData!;


nit: leave out unused variable

Suggested change

final UpdateMessageMoveData(

:origStreamId, :newStreamId, :propagateMode, :origTopic, :newTopic,

) = event.moveData!;

final UpdateMessageMoveData(

:origStreamId, :newStreamId, :origTopic, :newTopic) = event.moveData!;

gnprice · 2025-03-10T21:00:43Z

lib/model/unreads.dart

+    final retainedMessageIds = <int>[];
+    for (final id in messageIds) {
+      if (!incomingMessageIds.contains(id)) {
+        retainedMessageIds.add(id);
+      }
+    }


This is just messageIds.where(…).toList(), right?

gnprice · 2025-03-10T21:01:56Z

lib/model/unreads.dart

+      // conversation are removed, which avoids making a copy of `messageId`
+      // unnecessarily.


Suggested change

// conversation are removed, which avoids making a copy of `messageId`

// unnecessarily.

// conversation are removed, which avoids making a copy of `messageIds`

// unnecessarily.

right?

gnprice · 2025-03-10T21:13:56Z

test/model/unreads_test.dart

+        (_) => eg.streamMessage(
+          stream: origChannel, topic: origTopic, flags: [MessageFlag.read]));
+      final unreadMessages = List<StreamMessage>.generate(10,
+        (_) => eg.streamMessage(stream: origChannel, topic: origTopic));


nit: make the parallel parts visibly parallel:

Suggested change

(_) => eg.streamMessage(

stream: origChannel, topic: origTopic, flags: [MessageFlag.read]));

final unreadMessages = List<StreamMessage>.generate(10,

(_) => eg.streamMessage(stream: origChannel, topic: origTopic));

(_) => eg.streamMessage(stream: origChannel, topic: origTopic,

flags: [MessageFlag.read]));

final unreadMessages = List<StreamMessage>.generate(10,

(_) => eg.streamMessage(stream: origChannel, topic: origTopic));

gnprice · 2025-03-10T21:18:13Z

test/model/unreads_test.dart

+      final readMessages = List<StreamMessage>.generate(10,
+        (_) => eg.streamMessage(


Messages can get mutated to handle events. So let's have these get generated fresh by each test that needs them.

(In particular if we refactored so that instead of calling Unreads.handleUpdateMessageEvent like other tests in this file, these called PerAccountStore.handleEvent like we do in some other areas of test/model/, I believe these would break as written.)

gnprice · 2025-03-10T21:25:42Z

test/model/unreads_test.dart

+        )).toList();
+      }
+
+      test('smoke', () async {


Suggested change

test('smoke', () async {

test('moving a conversation exactly', () async {

I think this can also check we have the optimization at #1311 (comment) , using identicalTo.

gnprice · 2025-03-10T21:30:22Z

test/model/unreads_test.dart

@@ -469,6 +469,145 @@ void main() {
        }
      }
    });
+
+    group('moves', () {


Other cases to test here, in order to properly exercise _popAllInStreamTopic:

the moved messages are an (inhabited) proper subset of the existing unreads in the conversation

the moved messages are a proper superset of the (inhabited) set of existing unreads in the conversation

Because of this simplification, it is easier to make sense of the constraints that apply to the inter-related fields. This also allows us to drop the `assert`'s and "TODO(log)"'s, because the stacktrace we need can be retrieved after throwing these `FormatException`'s. This is similar to zulip-mobile code for parsing move data. The main difference is that we now also check the value of `propagate_mode`, which is documented to be present on message moves. See Greg's comment on this: zulip#1311 (comment) Signed-off-by: Zixuan James Li <[email protected]>

…hecks This throws `AssertionError`s on debug builds when the event data is malformed, and ignores it for release builds, ensuring that none of the move-related fields are `null` by the time we look at their values. Because of this simplification, it is easier to make sense of the constraints between the fields when there is a move. This is similar to zulip-mobile code for parsing move data. The main difference is that we now also check the value of `propagate_mode`, which is documented to be present on message moves. See Greg's comment on this: zulip#1311 (comment) Signed-off-by: Zixuan James Li <[email protected]>

PIG208 · 2025-03-12T22:21:55Z

Thanks for the review! Reorganized the move data change into 1 commit followed by 3 nfc ones, and addressed the comments on the main commit.

eeb89b4 message test [nfc]: Move tests to a new home
cf816ef api [nfc]: Simplify move data parsing with null-checks
40ccd50 api [nfc]: Extract and parse UpdateMessageMoveData from UpdateMessageEvent
b9eafee message: Preface message move handling with more comprehensive null-checks

gnprice

Thanks!

Here's a partial review focused on that stretch of refactoring commits. I decided some of this would be most efficient for me to write instead of trying to describe — so I've just pushed a revision where those 4 commits you mentioned above are now these 5:
95fbd04 message: Identify moves by inequality new vs orig, not just non-nullity
4392beb message [nfc]: In moves, give newStreamId/newTopic their fallback values sooner
6b75bae wip api [nfc]: Extract and parse UpdateMessageMoveData from UpdateMessageEvent
5d2a8bb api [nfc]: Simplify move data parsing with null-checks
e79ea1e message test [nfc]: Move tests to a new home

The first two are new commits I just wrote (and one of them has you as a co-author), replacing your commit:
b9eafee message: Preface message move handling with more comprehensive null-checks

The third one I marked as "wip" because I'm not sure it's entirely coherent after I rebased it atop the other two.

The two after that I didn't touch.

I think this revision handles my comments below, except for the last one. So the open items for you are that last small comment, plus please take a look at the "wip" commit 6b75bae and see what adjustments it needs.

gnprice · 2025-03-13T05:03:56Z

lib/model/message.dart

-      assert(debugLog('Malformed UpdateMessageEvent: move but no propagateMode')); // TODO(log)
+      assert(false, 'Malformed UpdateMessageEvent: move but no propagateMode'); // TODO(log)
      return;


Mmm I see, I guess in #1311 (comment) when I wrote:

In particular it'd be helpful if that second commit is NFC for debug builds — i.e. if its only functional change is that in some conditions where the old code would assert, the new code will throw while parsing.

that was a bit misleading because I'd read the existing code too quickly. Sorry.

When I said "where the old code would assert", I meant what the existing code already does here — but in fact I see now that it's not really asserting, it's just logging a message in debug mode, and then ignoring the event.

This change from log-and-ignore to assert-or-ignore is fine, I guess, but isn't really necessary. The key thing I was going for here is just that the second change is NFC for the events that either its old or new code accepts; so old and new accept the same set of events, and have the same effect for those events, and the only functional change is in the specifics of what it looks like when the code rejects an event. (Plus that it's easy to tell by reading the change that it is indeed NFC in that respect, because the logic matches.)

The reason it's useful to separate that functional change is that it's something which should ideally happen in the same commit as where this logic gets moved from this file to the API parsing, because the old way (just ignore the event and maybe log) is appropriate here where we're already deep into processing the event, and the new way (throw) is appropriate in the API parsing code where it's not too late to reject the whole event up front.

(I did in the later review at #1311 (comment) say:

That means that after the first commit, while this logic still lives in this handle-event method, it still asserts for malformed events

so there at least I indicated (with "still") that I expected this aspect wouldn't change in the first commit of this split.)

gnprice · 2025-03-13T05:14:56Z

lib/model/message.dart

      return;
    }

-    if (newStreamId == null && newTopic == null) {


message: Preface message move handling with more comprehensive null-checks

Does this really make the null-checks more comprehensive? The old null-checks look pretty comprehensive to me — there's the one on this line, and two more below it.

In particular, is there a case where this version rejects an event which the old code would have acted on?

It seems to me like the main point of this commit is the way it moves the two fallbacks like ?? origTopic up to the top of this logic, in order to make it closer to how it'll be after we move all the "find if and how the messages were moved" logic out from here into the API parsing code.

gnprice · 2025-03-13T05:30:44Z

test/model/message_test.dart

+        'stream_id': 1,
+        'new_stream_id': 2,
+        'orig_subject': null,
+      })).throws<AssertionError>();


The fact that these exception types keep changing in subsequent commits seems like a good sign that they're a detail we don't actually care about — so best to leave the specific types out.

Signed-off-by: Zixuan James Li <[email protected]>

The streamId of the moved messages does not change as intended, and the topic remains the same; this is not a valid message move. We will later introduce a data structure that captures inconsistencies of this kind. Signed-off-by: Zixuan James Li <[email protected]>

The fields orig_subject and propagate_mode are always present on message moves. See also API documentation: https://zulip.com/api/get-events#update_message Signed-off-by: Zixuan James Li <[email protected]>

This is NFC except in certain malformed cases: where either newStreamId is present and equals origStreamId, or newTopic is present and equals origTopic. The API docs say neither of those should happen. This version is useful because it brings this code closer to how it will look when we move this logic of "were the messages moved, and how" into the API parsing code so that other parts of the data model can reuse the results.

…ues sooner To see that this is NFC, first observe that the only possible behavior change this could cause is to change some event from being ignored (hitting a `return` in this section of code) to being processed by the code below, or vice versa. (It might also change what particular message gets printed to the debug log; but that doesn't count as a behavior change. As a part of that, we add an additional check for propagate mode, to succeed the block of assertion.) Then both before and after this change, the events that this overall set of conditions accepts are those that (a) have all of origTopic, origStreamId, and propagateMode non-null, and (b) have at least one of newTopic or newStreamId with a non-null value different from origTopic or origStreamId respectively. This change is useful because it brings this logic closer to how it will look when we move it into the API parsing code. Co-authored-by: Zixuan James Li <[email protected]>

This is mostly NFC except that we were logging and ignoring malformed event data before, and now start to throw errors with the parsing code. This data structure encapsulates some checks so that we can make all fields non-nullable, with reasonable fallback values. As of writing, we do not use origStreamId (a.k.a.: 'stream_id') when there was no message move, even though it is present if there were content edits This makes dropping 'stream_id' when parsing `moveData` into `null` acceptable for now. Signed-off-by: Zixuan James Li <[email protected]>

Signed-off-by: Zixuan James Li <[email protected]>

This does not correctly catch the case when `origTopic='topic'`, and `newTopic=origStreamId=newStreamId=null`. Instead, remove it and rely on the assertions from UpdateMessageMoveData's constructor. Signed-off-by: Zixuan James Li <[email protected]>

Signed-off-by: Zixuan James Li <[email protected]>

We could have extend `_removeAllInStreamTopic` to fit the needs of handling moved unreads, but it is used in a code path where messages are marked as read (which will become hotter after zulip#81). The new helper is designed to avoid making copies of message IDs when possible. As for the name, we use "pop" (inspired by Python) to indicate that the removed items are returned, since "removeAll" in Dart doesn't carry that meaning. Signed-off-by: Zixuan James Li <[email protected]>

PIG208 · 2025-03-13T21:53:00Z

Thanks for helping with this! Pushed an update to the PR.

PIG208 force-pushed the pr-unreads branch 5 times, most recently from 683c713 to dad5aa0 Compare January 30, 2025 23:23

PIG208 marked this pull request as ready for review January 30, 2025 23:24

PIG208 added the maintainer review PR ready for review by Zulip maintainers label Jan 30, 2025

PIG208 requested a review from chrisbobbe January 30, 2025 23:24

PIG208 assigned chrisbobbe Jan 30, 2025

PIG208 requested a review from gnprice February 12, 2025 19:03

PIG208 force-pushed the pr-unreads branch from dad5aa0 to 97dc744 Compare February 12, 2025 20:34

PIG208 force-pushed the pr-unreads branch from 97dc744 to 6cbc7aa Compare February 20, 2025 04:03

PIG208 force-pushed the pr-unreads branch from 6cbc7aa to ea08384 Compare February 20, 2025 21:44

chrisbobbe reviewed Feb 22, 2025

View reviewed changes

PIG208 force-pushed the pr-unreads branch from ea08384 to ca94f61 Compare February 24, 2025 22:51

PIG208 force-pushed the pr-unreads branch from ca94f61 to 830ab64 Compare February 24, 2025 23:01

PIG208 force-pushed the pr-unreads branch from 830ab64 to d75392d Compare February 25, 2025 00:12

PIG208 force-pushed the pr-unreads branch from d75392d to 5369c36 Compare February 25, 2025 03:44

PIG208 requested a review from chrisbobbe February 25, 2025 03:48

PIG208 force-pushed the pr-unreads branch from be550c4 to 8ab9902 Compare March 3, 2025 22:41

gnprice reviewed Mar 4, 2025

View reviewed changes

PIG208 force-pushed the pr-unreads branch 2 times, most recently from 410de59 to 9ed40a6 Compare March 6, 2025 03:56

PIG208 force-pushed the pr-unreads branch from 9ed40a6 to 709714b Compare March 7, 2025 02:31

gnprice reviewed Mar 10, 2025

View reviewed changes

PIG208 force-pushed the pr-unreads branch from 709714b to bce75df Compare March 12, 2025 22:18

gnprice force-pushed the pr-unreads branch from bce75df to e5be842 Compare March 13, 2025 06:29

gnprice reviewed Mar 13, 2025

View reviewed changes

PIG208 and others added 10 commits March 13, 2025 15:49

api [nfc]: Add PropagateMode.fromRawString

5f9b53e

Signed-off-by: Zixuan James Li <[email protected]>

events test: Make test data more realistic

a1cf12b

The fields orig_subject and propagate_mode are always present on message moves. See also API documentation: https://zulip.com/api/get-events#update_message Signed-off-by: Zixuan James Li <[email protected]>

api [nfc]: Simplify move data parsing with null-checks

9f4eccc

Signed-off-by: Zixuan James Li <[email protected]>

unreads [nfc]: Add assertions for _addAllInStreamTopic

c6d6aad

Signed-off-by: Zixuan James Li <[email protected]>

PIG208 force-pushed the pr-unreads branch from e5be842 to 9ab5f3b Compare March 13, 2025 20:55

		/// Data structure representing a message move.
		class UpdateMessageMoveData {

		if (topics == null) return;
		if (topics == null) return QueueList();

		// TODO(#901) handle moved messages
		madeAnyUpdate \|= _handleMessageMove(event);

		void _removeAllInStreamTopic(Set<int> incomingMessageIds, int streamId, TopicName topic) {
		QueueList<int>? _removeAllInStreamTopic(Set<int> incomingMessageIds, int streamId, TopicName topic) {

		// conversation are removed, which avoids making a copy of `messageId`
		// unnecessarily.

		final readMessages = List<StreamMessage>.generate(10,
		(_) => eg.streamMessage(

	test('smoke', () async {
	test('moving a conversation exactly', () async {

Handle moved messages for unreads #1311

Are you sure you want to change the base?

Handle moved messages for unreads #1311

Conversation

PIG208 commented Jan 27, 2025 • edited Loading

chrisbobbe commented Feb 8, 2025 • edited Loading

gnprice commented Feb 19, 2025

gnprice commented Feb 19, 2025

PIG208 commented Feb 20, 2025

chrisbobbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Feb 25, 2025

PIG208 commented Feb 25, 2025

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 Mar 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Mar 6, 2025

PIG208 commented Mar 6, 2025

PIG208 commented Mar 7, 2025

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Mar 12, 2025

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Mar 13, 2025

PIG208 commented Jan 27, 2025 •

edited

Loading

chrisbobbe commented Feb 8, 2025 •

edited

Loading

PIG208 Mar 4, 2025 •

edited

Loading