Skip to content

Commit

Permalink
Fix: Ensure that attachments are always rendered
Browse files Browse the repository at this point in the history
Because we only attach AttachmentViews to the DOM when they fire their
'update' event, we were subject to a race condition. If that event fired
after the final Message.render(), then it would be properly attached to
the final DOM node. If it fired early, it would end up missing from
the visible DOM entirely, attached to the old, discarded version of
the message.

This change updates our handling of a second call to loadAttachments().
Instead of bailing out if we've been called before, we attempt to
re-add our child AttachmentViews to the current DOM. But only if the
'update' event has been fired, and if their current parent node is not
what is in the DOM.

FREEBIE
  • Loading branch information
scottnonnenberg committed Jun 7, 2017
1 parent da8d49b commit f38d8eb
Showing 1 changed file with 27 additions and 9 deletions.
36 changes: 27 additions & 9 deletions js/views/message_view.js
Original file line number Diff line number Diff line change
Expand Up @@ -260,12 +260,32 @@
}))();
this.$('.avatar').replaceWith(avatarView.render().$('.avatar'));
},
appendAttachmentView: function(view) {
// We check for a truthy 'updated' here to ensure that a race condition in a
// multi-fetch() scenario doesn't add an AttachmentView to the DOM before
// its 'update' event is triggered.
var parent = this.$('.attachments')[0];
if (view.updated && parent !== view.el.parentNode) {
if (view.el.parentNode) {
parent.removeChild(view.el);
}

this.trigger('beforeChangeHeight');
this.$('.attachments').append(view.el);
this.trigger('afterChangeHeight');
}
},
loadAttachments: function() {
this.loadedAttachments = this.loadedAttachments || [];

// Messages can go from no attachments to some attachments (via key approval),
// but they can't go from some attachments to a different set of them.
// If we're called a second time, render() has replaced the DOM out from under
// us with $el.html(). We'll need to reattach our AttachmentViews to the new
// parent DOM nodes if the 'update' event has already fired.
if (this.loadedAttachments.length) {
for (var i = 0, max = this.loadedAttachments.length; i < max; i += 1) {
var view = this.loadedAttachments[i];
this.appendAttachmentView(view);
}
return;
}

Expand All @@ -274,15 +294,13 @@
model: attachment,
timestamp: this.model.get('sent_at')
});
this.listenTo(view, 'update', function() {
if (!view.el.parentNode) {
this.trigger('beforeChangeHeight');
this.$('.attachments').append(view.el);
this.trigger('afterChangeHeight');
}
});
view.render();
this.loadedAttachments.push(view);

this.listenTo(view, 'update', function() {
view.updated = true;
this.appendAttachmentView(view);
});
}.bind(this));
}
});
Expand Down

0 comments on commit f38d8eb

Please sign in to comment.