msglist: Separate messages after 10-min gap, with a sender row #1815

rajveermalviya · 2025-08-19T12:35:45Z

Before	After

Fixes: #1773

chrisbobbe

Thanks, this works great in my manual testing! Small comments below.

chrisbobbe · 2025-08-20T18:27:19Z

test/model/message_list_test.dart

+      streamMessage(5, t2, eg.otherUser), // same sender, but within 10 mins of last message
+      streamMessage(6, t3, eg.otherUser), // same sender, after 10 mins of last message
+      dmMessage(7,     t3, eg.otherUser), // same sender, but new recipient
+      dmMessage(8,     t4, eg.otherUser), // same sender/recipient, but new day


Can t4 i.e. clock.now() be between 11:48:59 and midnight? In that case "new day" might not be accurate, I think, and the test will flake.

chrisbobbe · 2025-08-20T18:33:10Z

test/model/message_list_test.dart

-      ..showSender.equals(
-        forcedShowSender || allMessages[j].senderId != allMessages[j-1].senderId)
+      ..showSender.equals(forcedShowSender)


nit: maybe rename forcedShowSender to just showSender; the old name drew a distinction between the natural behavior (senders don't match) and an override (from various causes), but now the same variable represents both

rajveermalviya · 2025-08-20T22:06:04Z

Thanks for the review @chrisbobbe! Pushed an update, PTAL.

chrisbobbe · 2025-08-20T22:18:41Z

Thanks, LGTM! Marking for Greg's review.

gnprice

Thanks @rajveermalviya, and thanks @chrisbobbe for the previous review! Comments below.

gnprice · 2025-08-20T23:50:48Z

lib/model/message_list.dart

@@ -459,8 +459,10 @@ mixin _MessageSequence {
      if (!messagesSameDay(prevMessageItem.message, message)) {
        items.add(MessageListDateSeparatorItem(message));
        canShareSender = false;
+      } else if (prevMessageItem.message.senderId == message.senderId) {
+        canShareSender = messagesWithinTenMinutes(prevMessage, message);


nit: give this a more conceptual name

Suggested change

canShareSender = messagesWithinTenMinutes(prevMessage, message);

canShareSender = messagesCloseInTime(prevMessage, message);

Otherwise we have to rename this function any time we decide to tweak the threshold 🙂

gnprice · 2025-08-20T23:51:39Z

lib/model/message_list.dart

+  if (diffSeconds <= 10 * 60) return true;
+  return false;


nit: just return the boolean rather than put through if

gnprice · 2025-08-20T23:52:30Z

test/model/message_list_test.dart

@@ -2946,9 +2946,21 @@ void main() {
    // whether the sender should be shown, but the difference between
    // fetchInitial and handleMessageEvent etc. doesn't matter.

+    // Elapse test's clock to a specific time, to avoid any flaky-ness
+    // that may be caused by a specific local time of the day.
+    final initialTime = DateTime(9000, 1, 1);


This is a pretty odd date. 🙂 The year 9000, is that right?

Let's keep test data realistic, wherever practical. So pick a date sometime this decade, say. (I typically go for the day I'm writing the test.)

gnprice · 2025-08-20T23:55:27Z

test/model/message_list_test.dart

+    final t2 = eg.utcTimestamp(
+      now.subtract(Duration(days: 1))
+        .add(Duration(minutes: 1)));
+    final t3 = eg.utcTimestamp(
+      now.subtract(Duration(days: 1))
+        .add(Duration(minutes: 1))
+        .add(Duration(minutes: 10, seconds: 1)));


How about:

Suggested change

final t2 = eg.utcTimestamp(

now.subtract(Duration(days: 1))

.add(Duration(minutes: 1)));

final t3 = eg.utcTimestamp(

now.subtract(Duration(days: 1))

.add(Duration(minutes: 1))

.add(Duration(minutes: 10, seconds: 1)));

final t2 = t1 + 60;

final t3 = t2 + 601;

Or could say Duration(minutes: 1).inSeconds instead of 60, if you want to put it that way.

gnprice · 2025-08-21T00:01:15Z

test/model/message_list_test.dart

      streamMessage(3, t1, eg.otherUser), // no recipient header, but new sender
-      dmMessage(4,     t1, eg.otherUser), // same sender, but new recipient
-      dmMessage(5,     t2, eg.otherUser), // same sender/recipient, but new day
+      streamMessage(5, t2, eg.otherUser), // same sender, but within 10 mins of last message


nit: skipping ID 4

Actually I think you can have a prep commit remove these message IDs. It looks like this test was written before eg.streamMessage etc. had the cleverness to auto-generate distinct, increasing IDs.

gnprice · 2025-08-21T00:13:19Z

test/model/message_list_test.dart

+    doTest('2021-01-01 00:19:59', time, false);
+    doTest('2021-01-01 00:20:00', time, true);
+    doTest('2021-01-01 00:20:01', time, true);
+    doTest('2021-01-01 00:25:00', time, true);
+    doTest('2021-01-01 00:29:59', time, true);


Out of these last three, let's keep at most one — they're basically redundant with the 00:20:00 case.

OTOH let's include a couple of cases that would catch potential bugs one can imagine having, like ignoring the date in favor of the hour/minute/second:

doTest('2020-01-01 00:30:00', time, true);

gnprice · 2025-08-21T00:14:04Z

test/model/message_list_test.dart

+    doTest(time, '2021-01-01 00:30:01', true);
+    doTest(time, '2021-01-01 00:35:00', true);
+    doTest(time, '2021-01-01 00:39:59', true);
+    doTest(time, '2021-01-01 00:40:00', true);
+    doTest(time, '2021-01-01 00:40:01', false);


These do basically the same thing as the cases above, right? The very same durations, just later.

gnprice · 2025-08-21T00:16:06Z

test/model/message_list_test.dart

-      ..showSender.equals(
-        forcedShowSender || allMessages[j].senderId != allMessages[j-1].senderId)
+      ..showSender.equals(showSender)


Moving this logic up to the showSender initializer seems fine, but let's do it as an NFC prep commit. That'd make it easier to see specifically the details that are changing in the main commit.

gnprice · 2025-08-21T00:17:25Z

test/model/message_list_test.dart

+    } else if (allMessages[j-1].senderId == allMessages[j].senderId) {
+      if (!messagesWithinTenMinutes(allMessages[j-1], allMessages[j])) {
+        showSender = true;
+      }
+    } else if (allMessages[j-1].senderId != allMessages[j].senderId) {
+      showSender = true;
    }


This feels like it can be simplified — the same condition appears twice.

For a start, the last else-if can be just else, right? The condition is always true if we reach that point.

gnprice · 2025-08-21T00:18:33Z

test/model/message_list_test.dart

@@ -3223,17 +3279,23 @@ void checkInvariants(MessageListView model) {

  int i = 0;
  for (int j = 0; j < allMessages.length; j++) {
-    bool forcedShowSender = false;
+    bool showSender = false;


I think this will also help toward simplifying this logic:

Suggested change

bool showSender = false;

final bool showSender;

Then adjust the if/else chain below so that it always gets set; it's already very close.

gnprice · 2025-08-21T01:06:40Z

The bulk of my comments above — all but the name of a method and a small other nit — are for the tests. So given the timing, and in order to get this out in a release today, I'll merge a version of this without the tests.

This area of the codebase is one where it's critical that we have tests, though. I've filed #1825 to follow up with those tests, and @rajveermalviya please pick that up as a priority, starting from the version here. (If we weren't doing a release today, and me going on vacation Friday, I expect we'd finish revising the tests within this PR and get it merged before the end of this week.)

Fixes: zulip#1773

rajveermalviya · 2025-08-21T16:10:35Z

Thanks for merging this @gnprice! Sent #1826 as a follow-up for tests.

rajveermalviya added the maintainer review PR ready for review by Zulip maintainers label Aug 19, 2025

rajveermalviya assigned chrisbobbe Aug 19, 2025

rajveermalviya requested a review from chrisbobbe August 19, 2025 12:35

chrisbobbe reviewed Aug 20, 2025

View reviewed changes

rajveermalviya force-pushed the pr-message-gap branch from f180667 to e76fe71 Compare August 20, 2025 22:05

rajveermalviya requested a review from chrisbobbe August 20, 2025 22:06

chrisbobbe requested a review from gnprice August 20, 2025 22:18

chrisbobbe assigned gnprice and unassigned chrisbobbe Aug 20, 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 Aug 20, 2025

gnprice reviewed Aug 21, 2025

View reviewed changes

gnprice mentioned this pull request Aug 21, 2025

Write tests for separating messages after a short time gap #1825

Closed

msglist: Separate messages after 10-min gap, with a sender row

999b089

Fixes: zulip#1773

gnprice force-pushed the pr-message-gap branch from e76fe71 to 999b089 Compare August 21, 2025 01:18

gnprice merged commit 999b089 into zulip:main Aug 21, 2025
1 check passed

gnprice mentioned this pull request Aug 21, 2025

msglist test: Add test for messagesCloseInTime, and update a related test #1826

Merged

	canShareSender = messagesWithinTenMinutes(prevMessage, message);
	canShareSender = messagesCloseInTime(prevMessage, message);

msglist: Separate messages after 10-min gap, with a sender row #1815

msglist: Separate messages after 10-min gap, with a sender row #1815

Uh oh!

Conversation

rajveermalviya commented Aug 19, 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!

rajveermalviya commented Aug 20, 2025

Uh oh!

chrisbobbe commented Aug 20, 2025

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice commented Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

rajveermalviya commented Aug 21, 2025

Uh oh!

Uh oh!

gnprice commented Aug 21, 2025 •

edited

Loading