model: Prevent duplicate flags when processing update_message_flags. #2033

sarafarajnasardi · 2025-12-11T08:28:26Z

Previously, the event handler would unconditionally add flags to the message's flag list. If a flag (like 'read') was already present, this created duplicates, which broke subsequent removal operations.

This commit adds a check to ensure the flag is not already present before adding it, making the operation idempotent.

Fixes #1986

chrisbobbe

Thanks! Small comments below.

Also some commit-message nits:

Please keep the body of your commit message word-wrapped to 68 columns.

In this case the Fixes line suffices to explain the change, so you don't need the two paragraphs above that.

Also the Fixes line should read Fixes #1986. (note the period).

chrisbobbe · 2025-12-12T00:08:25Z

test/model/message_test.dart

+        // Should verify no notifications since no actual change happened
+        // But current implementation might notify anyway if it doesn't check for change
+        // optimization. The main point is flags list shouldn't have duplicates.
+        // Let's check the flags first.


I appreciate wanting to explain this 🙂 but I think we can cut this comment; the test is about an edge case that should be rare (in the fetch-event race) and an extra notifyListeners isn't a significant concern here.

sarafarajnasardi · 2025-12-12T03:11:31Z

Thanks! I've removed the test comments and updated the commit message formatting

chrisbobbe

Thanks! Small comments below, otherwise LGTM.

chrisbobbe · 2025-12-12T03:10:35Z

lib/model/message.dart

    if (isAdd && (event as UpdateMessageFlagsAddEvent).all) {
      for (final message in messages.values) {
-        message.flags.add(event.flag);
+        if (!message.flags.contains(event.flag)) {
+          message.flags.add(event.flag);
+        }


Please also write a test exercising this case.

chrisbobbe · 2025-12-12T03:11:39Z

test/model/message_test.dart

+      test('avoid duplicate flags', () async {
+        await prepare();


Suggested change

test('avoid duplicate flags', () async {

await prepare();

test('avoid duplicate flags', () async {

// Regression test for https://github.com/zulip/zulip-flutter/issues/1986

await prepare();

should I add this same comment in new test also?

Yes, I think so; it's applies equally there.

done @chrisbobbe

sarafarajnasardi · 2025-12-12T03:45:24Z

Done! I have added the new test and your suggested comment.

chrisbobbe

Thanks! One small comment below, and I'll also mark this for Greg's review.

chrisbobbe · 2025-12-15T19:53:13Z

test/model/message_test.dart

+        check(store).messages.values.single.flags.deepEquals([MessageFlag.read]);
+      });
+
+      test('handle update_message_flags: add flag "all" is idempotent', () async {


I'd like to give the two new tests more similar descriptions, so it's more obvious that they're testing very similar logic. How about:

'prevent duplicate flags, when all: false'

'prevent duplicate flags, when all: true'

or similar.

gnprice

Thanks @sarafarajnasardi for taking care of this, and thanks @chrisbobbe for the previous reviews!

Generally this looks good. Some small comments, in addition to the one Chris makes above.

gnprice · 2025-12-15T23:23:01Z

test/model/message_test.dart

+        final m1 = eg.streamMessage(flags: [MessageFlag.read]);
+        final m2 = eg.streamMessage(flags: []);


nit:

Suggested change

final m1 = eg.streamMessage(flags: [MessageFlag.read]);

final m2 = eg.streamMessage(flags: []);

final message1 = eg.streamMessage(flags: [MessageFlag.read]);

final message2 = eg.streamMessage(flags: []);

This point about avoiding abbreviations, in the Flutter upstream style guide, applies equally in Zulip:
https://github.com/flutter/flutter/blob/master/docs/contributing/Style-guide-for-Flutter-repo.md#avoid-abbreviations

(And not only in the mobile repo — in fact I think Tim applies it more strongly than I do, and we've been doing so since long before we used Flutter.)

gnprice · 2025-12-15T23:24:20Z

test/model/message_test.dart

+        check(store.messages[m1.id]!.flags).deepEquals([MessageFlag.read]);
+        check(store.messages[m2.id]!.flags).deepEquals([MessageFlag.read]);


nit: Conversely, for making this a bit more compact again, see the test cases with "message1" and "message2" earlier in this group. Those give examples of how the package:checks API enables some handy shorthands.

gnprice · 2025-12-15T23:25:26Z

test/model/message_test.dart

+        await store.handleEvent(UpdateMessageFlagsAddEvent(
+          id: 1,


nit: like the other cases in this group, use mkAddEvent for brevity and clarity

gnprice · 2025-12-15T23:30:55Z

Oh and a commit-message nit:

model: Prevent duplicate flags when processing update_message_flags.

This should have a more specific prefix (vs "model:" which is quite general). To see examples, try git log --oneline on this file.

Plus one very small nit: in this repo we don't use a period at the end of the summary line.

gnprice · 2026-01-14T23:05:01Z

@sarafarajnasardi Do you think you'll have time to come back and finish addressing the feedback on this PR? It'd be great to have it completed.

sarafarajnasardi · 2026-01-14T23:08:06Z

@gnprice Yes, I’ll get back to it and finish addressing the feedback shortly. I’ll update the PR once it’s done. Thanks for the reminder!

sarafarajnasardi · 2026-01-22T08:23:35Z

Hi @gnprice and @chrisbobbe ,

Apologies for the delay in addressing the feedback. I encountered some personal matters that slowed my progress, and I accidentally closed the PR due to an incorrect force push while updating the changes.

I've now addressed all the review comments:

1.Fixed the duplicate flags issue for both all: true and all: false cases
2.Renamed variables to avoid abbreviations (message1, message2)
3.Used mkAddEvent helper and package:checks cascade shorthand
4.Updated commit message with proper prefix and formatting
The PR is now ready for review. Thank you for your patience and detailed feedback!

gnprice · 2026-01-24T01:45:08Z

Thanks for the revision. This looks good, with two comments:

The second commit is messy: it leaves stray whitespace, and the commit message is just "--amend" which doesn't say anything clear.

Fortunately this commit is unnecessary after a rebase, so I'll just drop it.
The commit message should have its summary line all on one line. Fixed that like so:

    -    message: Prevent duplicate flags when processing
    -    update_message_flags
    +    message: Prevent duplicate flags when processing update_message_flags

(It's fine for the summary line to go a bit long, like up to 80 columns, unlike the paragraphs that make up the body of the commit message which should stick to 68 columns.)

Merging with those two tweaks.

Fixes zulip#1986.

sarafarajnasardi · 2026-01-24T02:19:29Z

@gnprice thanks for the merge...I"ll keep your suggestions in mind next time...

chrisbobbe reviewed Dec 12, 2025

View reviewed changes

chrisbobbe self-assigned this Dec 12, 2025

chrisbobbe added the maintainer review PR ready for review by Zulip maintainers label Dec 12, 2025

chrisbobbe reviewed Dec 12, 2025

View reviewed changes

chrisbobbe reviewed Dec 15, 2025

View reviewed changes

chrisbobbe requested a review from gnprice December 15, 2025 19:53

chrisbobbe assigned gnprice and unassigned chrisbobbe Dec 15, 2025

chrisbobbe added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Dec 15, 2025

gnprice reviewed Dec 15, 2025

View reviewed changes

gnprice force-pushed the fix-duplicate-message-flags branch from 484381e to 690a991 Compare January 24, 2026 01:45

message: Prevent duplicate flags when processing update_message_flags

f04a542

Fixes zulip#1986.

gnprice force-pushed the fix-duplicate-message-flags branch from 690a991 to f04a542 Compare January 24, 2026 02:04

gnprice merged commit f04a542 into zulip:main Jan 24, 2026
1 check passed

		final m1 = eg.streamMessage(flags: [MessageFlag.read]);
		final m2 = eg.streamMessage(flags: []);

		check(store.messages[m1.id]!.flags).deepEquals([MessageFlag.read]);
		check(store.messages[m2.id]!.flags).deepEquals([MessageFlag.read]);

model: Prevent duplicate flags when processing update_message_flags. #2033

model: Prevent duplicate flags when processing update_message_flags. #2033

Uh oh!

Conversation

sarafarajnasardi commented Dec 11, 2025

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarafarajnasardi commented Dec 12, 2025

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarafarajnasardi commented Dec 12, 2025

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice commented Dec 15, 2025

Uh oh!

gnprice commented Jan 14, 2026

Uh oh!

sarafarajnasardi commented Jan 14, 2026

Uh oh!

sarafarajnasardi commented Jan 22, 2026

Uh oh!

gnprice commented Jan 24, 2026

Uh oh!

Uh oh!

sarafarajnasardi commented Jan 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants