message list: Render new messages only after "newest" is found.

If a user sends a message while the latest batch of
messages are being fetched, the new message recieved
from `server_events` gets displayed temporarily out of
order (just after the the current batch of messages)
for the current narrow.

We could just discard the new message events if we havent
recieved the last message i.e. when `found_newest` = False,
since we would recieve them on furthur fetching of that
narrow.
But this would create another bug where the new messages
sent while fetching the last batch of messages would not
get rendered. Because, `found_newest` = True and we would
no longer fetch messages for that narrow, thus the new
messages would not get fetched and are also discarded from
the events codepath.

Thus to resolve both these bugs we use the following approach:

* We do not add the new batch of messages for the current narrow
  while `has_found_newest` = False.
* We store the latest message id which should be displayed at the
  bottom of the narrow in `fetch_status`.
* Ideally `expected_max_message_id`'s value should be equal to the
  last item's id in `MessageListData`.
* So the messages received while `has_found_newest` = False,
  will be fetched later and also the `expected_max_message_id`
  value gets updated.
* And after fetching the last batch where `has_found_newest` = True,
  we would again fetch messages if the `expected_max_message_id` is
  greater than the last message's id found on fetching by refusing to
  update the server provided `has_found_newest` = True in `fetch_status`.

Another benefit of not discarding the events is that the
message gets processed not rendered i.e. we still get desktop
notifications and unread count updates.

Fixes #14017
This commit is contained in:
Ryan Rehman
2020-05-30 21:04:07 +05:30
committed by Tim Abbott
parent 59b68aaa98
commit 6637f2dbb7
5 changed files with 51 additions and 7 deletions

View File

@@ -70,7 +70,7 @@ run_test('basics', () => {
found_newest: true, found_newest: true,
history_limited: true, history_limited: true,
}; };
fetch_status.finish_newer_batch(data); fetch_status.finish_newer_batch([], data);
fetch_status.finish_older_batch(data); fetch_status.finish_older_batch(data);
has_found_oldest(); has_found_oldest();
@@ -94,7 +94,7 @@ run_test('basics', () => {
found_newest: false, found_newest: false,
history_limited: false, history_limited: false,
}; };
fetch_status.finish_newer_batch(data); fetch_status.finish_newer_batch([], data);
fetch_status.finish_older_batch(data); fetch_status.finish_older_batch(data);
can_load_older(); can_load_older();
@@ -147,7 +147,7 @@ run_test('basics', () => {
can_load_older(); can_load_older();
blocked_newer(); blocked_newer();
fetch_status.finish_newer_batch({ fetch_status.finish_newer_batch([], {
update_loading_indicator: true, update_loading_indicator: true,
found_newest: false, found_newest: false,
}); });
@@ -160,7 +160,7 @@ run_test('basics', () => {
can_load_older(); can_load_older();
blocked_newer(); blocked_newer();
fetch_status.finish_newer_batch({ fetch_status.finish_newer_batch([], {
update_loading_indicator: true, update_loading_indicator: true,
found_newest: true, found_newest: true,
}); });

View File

@@ -345,6 +345,23 @@ run_test('loading_newer', () => {
}); });
assert.equal(msg_list.data.fetch_status.can_load_newer_messages(), true); assert.equal(msg_list.data.fetch_status.can_load_newer_messages(), true);
// The server successfully responded with messages having id's from 500-599.
// We test for the case that this was the last batch of messages for the narrow
// so no more fetching should occur.
// And also while fetching for the above condition the server received a new message
// event, updating the last message's id for that narrow to 600 from 599.
data.resp.found_newest = true;
msg_list.data.fetch_status.update_expected_max_message_id([{id: 600}]);
test_happy_path({
msg_list: msg_list,
data: data,
});
// To handle this special case we should allow another fetch to occur,
// since the last message event's data had been discarded.
assert.equal(msg_list.data.fetch_status.can_load_newer_messages(), true);
}()); }());
(function test_home() { (function test_home() {

View File

@@ -7,6 +7,15 @@ const FetchStatus = function () {
let found_oldest = false; let found_oldest = false;
let found_newest = false; let found_newest = false;
let history_limited = false; let history_limited = false;
let expected_max_message_id = 0;
function max_id_for_messages(messages) {
let max_id = 0;
for (const msg of messages) {
max_id = Math.max(max_id, msg.id);
}
return max_id;
}
self.start_older_batch = function (opts) { self.start_older_batch = function (opts) {
loading_older = true; loading_older = true;
@@ -43,7 +52,11 @@ const FetchStatus = function () {
} }
}; };
self.finish_newer_batch = function (opts) { self.finish_newer_batch = function (messages, opts) {
const found_max_message_id = max_id_for_messages(messages);
if (opts.found_newest && expected_max_message_id > found_max_message_id) {
opts.found_newest = false;
}
loading_newer = false; loading_newer = false;
found_newest = opts.found_newest; found_newest = opts.found_newest;
if (opts.update_loading_indicator) { if (opts.update_loading_indicator) {
@@ -59,6 +72,11 @@ const FetchStatus = function () {
return found_newest; return found_newest;
}; };
self.update_expected_max_message_id = function (messages) {
expected_max_message_id = Math.max(expected_max_message_id,
max_id_for_messages(messages));
};
return self; return self;
}; };

View File

@@ -83,7 +83,7 @@ function get_messages_success(data, opts) {
} }
if (opts.num_after > 0) { if (opts.num_after > 0) {
opts.msg_list.data.fetch_status.finish_newer_batch({ opts.msg_list.data.fetch_status.finish_newer_batch(data.messages, {
update_loading_indicator: update_loading_indicator, update_loading_indicator: update_loading_indicator,
found_newest: data.found_newest, found_newest: data.found_newest,
}); });
@@ -92,7 +92,7 @@ function get_messages_success(data, opts) {
// the fetch_status data structure for message_list.all, // the fetch_status data structure for message_list.all,
// which is never rendered (and just used for // which is never rendered (and just used for
// prepopulating narrowed views). // prepopulating narrowed views).
message_list.all.data.fetch_status.finish_newer_batch({ message_list.all.data.fetch_status.finish_newer_batch(data.messages, {
update_loading_indicator: false, update_loading_indicator: false,
found_newest: data.found_newest, found_newest: data.found_newest,
}); });

View File

@@ -19,7 +19,16 @@ function add_messages(messages, msg_list, opts) {
exports.add_old_messages = function (messages, msg_list) { exports.add_old_messages = function (messages, msg_list) {
return add_messages(messages, msg_list, {messages_are_new: false}); return add_messages(messages, msg_list, {messages_are_new: false});
}; };
exports.add_new_messages = function (messages, msg_list) { exports.add_new_messages = function (messages, msg_list) {
if (!msg_list.data.fetch_status.has_found_newest()) {
// We don't render newly received messages for the message list,
// if we haven't found the latest messages to be displayed in the
// narrow. Otherwise the new message would be rendered just after
// the previously fetched messages when that's inaccurate.
msg_list.data.fetch_status.update_expected_max_message_id(messages);
return;
}
return add_messages(messages, msg_list, {messages_are_new: true}); return add_messages(messages, msg_list, {messages_are_new: true});
}; };