mirror of https://github.com/zulip/zulip.git
unread: Fix process_visible race with fetching.
The previous batch of improvements to this code path in
6562ea94e4
introduced a race bug where:
- You navigate to a narrowed view; Zulip renders cached data from
`all_messages_data` that we know is current, but
`fetch_status.has_found_newest` is initialized to `false`
nonetheless.
- The bottom is visible, triggering the check for whether the view
should be marked as read.
- `fetch_status.has_found_newest` is still `false`, and so we
incorrectly refuse to mark as read.
- We finish fetching data from the server, do the background rerender
for that (with no changes), but that doesn't trigger a re-check for
whether the bottom is visible.
There's several ways to address this, but most correct to me is to not
check fetch_status in this particular code path.
The same reasoning applies to the navigate.js call sites.
This commit is contained in:
parent
b8562bf7b4
commit
b9af5ce86e
|
@ -1439,16 +1439,21 @@ export class MessageListView {
|
|||
this.message_containers.clear();
|
||||
}
|
||||
|
||||
is_fetched_end_rendered() {
|
||||
return this._render_win_end === this.list.num_items();
|
||||
}
|
||||
|
||||
is_end_rendered() {
|
||||
// Used as a helper in checks for whether a given scroll
|
||||
// position is actually the very end of this view. It could
|
||||
// fail to be for two reasons: Either some newer messages are
|
||||
// not rendered due to a render window, or we haven't finished
|
||||
// fetching the newest messages for this view from the server.
|
||||
return (
|
||||
this._render_win_end === this.list.num_items() &&
|
||||
this.list.data.fetch_status.has_found_newest()
|
||||
);
|
||||
return this.is_fetched_end_rendered() && this.list.data.fetch_status.has_found_newest();
|
||||
}
|
||||
|
||||
is_fetched_start_rendered() {
|
||||
return this._render_win_start === 0;
|
||||
}
|
||||
|
||||
is_start_rendered() {
|
||||
|
@ -1457,7 +1462,7 @@ export class MessageListView {
|
|||
// fail to be for two reasons: Either some older messages are
|
||||
// not rendered due to a render window, or we haven't finished
|
||||
// fetching the oldest messages for this view from the server.
|
||||
return this._render_win_start === 0 && this.list.data.fetch_status.has_found_oldest();
|
||||
return this.is_fetched_start_rendered() && this.list.data.fetch_status.has_found_oldest();
|
||||
}
|
||||
|
||||
get_row(id) {
|
||||
|
|
|
@ -103,7 +103,7 @@ export function page_up() {
|
|||
if (
|
||||
message_viewport.at_rendered_top() &&
|
||||
!message_lists.current.visibly_empty() &&
|
||||
message_lists.current.view.is_start_rendered()
|
||||
message_lists.current.view.is_fetched_start_rendered()
|
||||
) {
|
||||
message_lists.current.select_id(message_lists.current.first().id, {then_scroll: false});
|
||||
} else {
|
||||
|
@ -115,7 +115,7 @@ export function page_down() {
|
|||
if (
|
||||
message_viewport.at_rendered_bottom() &&
|
||||
!message_lists.current.visibly_empty() &&
|
||||
message_lists.current.view.is_end_rendered()
|
||||
message_lists.current.view.is_fetched_end_rendered()
|
||||
) {
|
||||
message_lists.current.select_id(message_lists.current.last().id, {then_scroll: false});
|
||||
unread_ops.process_visible();
|
||||
|
|
|
@ -442,7 +442,14 @@ function process_scrolled_to_bottom() {
|
|||
}
|
||||
|
||||
if (message_lists.current.can_mark_messages_read()) {
|
||||
mark_current_list_as_read();
|
||||
// Mark all the messages in this message feed as read.
|
||||
//
|
||||
// Important: We have not checked definitively whether there
|
||||
// are further messages that we're waiting on the server to
|
||||
// return that would appear below the visible part of the
|
||||
// feed, so it would not be correct to instead ask the server
|
||||
// to mark all messages matching this entire narrow as read.
|
||||
notify_server_messages_read(message_lists.current.all_messages());
|
||||
return;
|
||||
}
|
||||
|
||||
|
@ -460,16 +467,12 @@ export function process_visible() {
|
|||
if (
|
||||
viewport_is_visible_and_focused() &&
|
||||
message_viewport.bottom_rendered_message_visible() &&
|
||||
message_lists.current.view.is_end_rendered()
|
||||
message_lists.current.view.is_fetched_end_rendered()
|
||||
) {
|
||||
process_scrolled_to_bottom();
|
||||
}
|
||||
}
|
||||
|
||||
export function mark_current_list_as_read(options) {
|
||||
notify_server_messages_read(message_lists.current.all_messages(), options);
|
||||
}
|
||||
|
||||
export function mark_stream_as_read(stream_id) {
|
||||
bulk_update_read_flags_for_narrow(
|
||||
[
|
||||
|
|
|
@ -125,7 +125,7 @@ run_test("unread_ops", ({override}) => {
|
|||
// data setup).
|
||||
override(message_lists.current, "can_mark_messages_read", () => can_mark_messages_read);
|
||||
override(message_lists.current, "has_unread_messages", () => true);
|
||||
override(message_lists.current.view, "is_end_rendered", () => true);
|
||||
override(message_lists.current.view, "is_fetched_end_rendered", () => true);
|
||||
|
||||
// First, test for a message list that cannot read messages.
|
||||
can_mark_messages_read = false;
|
||||
|
@ -136,11 +136,11 @@ run_test("unread_ops", ({override}) => {
|
|||
// Now flip the boolean, and get to the main thing we are testing.
|
||||
can_mark_messages_read = true;
|
||||
// Don't mark messages as read until all messages in the narrow are fetched and rendered.
|
||||
override(message_lists.current.view, "is_end_rendered", () => false);
|
||||
override(message_lists.current.view, "is_fetched_end_rendered", () => false);
|
||||
unread_ops.process_visible();
|
||||
assert.deepEqual(channel_post_opts, undefined);
|
||||
|
||||
override(message_lists.current.view, "is_end_rendered", () => true);
|
||||
override(message_lists.current.view, "is_fetched_end_rendered", () => true);
|
||||
unread_ops.process_visible();
|
||||
|
||||
// The most important side effect of the above call is that
|
||||
|
|
Loading…
Reference in New Issue