paulmataruso/zulip
1
0
Fork 0
mirror of https://github.com/zulip/zulip.git synced 2025-11-04 14:03:30 +00:00
Code Issues Packages Projects Releases Wiki Activity

message_list: Replace buggy rerender_the_whole_thing.

Browse Source 
As it turns out, our rerender_the_whole_thing function (used whenever
we were adding messages and discovered that the resulting message list
would be out-of-order) was just broken and scrolled the browser to a
random location.

This caused two user-facing bugs:

* On very fast networks, if two users sent messages at very close to
  the same time, we could end up with out-of-order message deliveries,
  triggering this code path, which was intended to silently correct
  the situation, but failed.

* In some narrows to streams with muted topics in the history but some
  recent traffic, the user's browser-cached history might have some
  gaps that mean the server fetch we do after narrowing discovers the
  history is out-of-order, again triggering the
  rerender_the_whole_thing code path.

The fix is to just remove that function, adding a new option to the
well-tested rerender_preserving_scrolltop (which has explicit logic to
preserve the scroll position) instead.

Fixes #12067.  Likely also fixes #12498.
This commit is contained in:
Tim Abbott
2019-09-18 11:38:07 -07:00
parent 0815a9bd53
commit edee1251c8
3 changed files with 10 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
frontend_tests/node_tests/message_list.js
View File

@@ -182,7 +182,7 @@ run_test('message_range', () => {
run_test('updates', () => { run_test('updates', () => {
var list = new MessageList({}); var list = new MessageList({});
list.view.rerender_the_whole_thing = noop; list.view.rerender_preserving_scrolltop = noop;
var messages = [ var messages = [
{ {

2
static/js/message_list.js
View File

@@ -57,7 +57,7 @@ exports.MessageList.prototype = {
var render_info; var render_info;
if (interior_messages.length > 0) { if (interior_messages.length > 0) {
self.view.rerender_the_whole_thing(); self.view.rerender_preserving_scrolltop(true);
return true; return true;
} }
if (top_messages.length > 0) { if (top_messages.length > 0) {

16
static/js/message_list_view.js
View File

@@ -1084,7 +1084,7 @@ MessageListView.prototype = {
return true; return true;
}, },
rerender_preserving_scrolltop: function () { rerender_preserving_scrolltop: function (discard_rendering_state) {
// old_offset is the number of pixels between the top of the // old_offset is the number of pixels between the top of the
// viewable window and the selected message // viewable window and the selected message
var old_offset; var old_offset;
@@ -1093,6 +1093,13 @@ MessageListView.prototype = {
if (selected_in_view) { if (selected_in_view) {
old_offset = selected_row.offset().top; old_offset = selected_row.offset().top;
} }
if (discard_rendering_state) {
// If we know that the existing render is invalid way
// (typically because messages appear out-of-order), then
// we discard the message_list rendering state entirely.
this.clear_rendering_state(true);
this.update_render_window(this.list.selected_idx(), false);
}
return this.rerender_with_target_scrolltop(selected_row, old_offset); return this.rerender_with_target_scrolltop(selected_row, old_offset);
}, },
@@ -1271,13 +1278,6 @@ MessageListView.prototype = {
this.maybe_rerender(); this.maybe_rerender();
}, },
rerender_the_whole_thing: function () {
// TODO: Figure out if we can unify this with this.list.rerender().
this.clear_rendering_state(true);
this.update_render_window(this.list.selected_idx(), false);
this.render(this.list.all_messages().slice(this._render_win_start, this._render_win_end), 'bottom');
},
clear_table: function () { clear_table: function () {
// We do not want to call .empty() because that also clears // We do not want to call .empty() because that also clears
// jQuery data. This does mean, however, that we need to be // jQuery data. This does mean, however, that we need to be