From f38d8eb4aed97e569cd88ab278e133ee412e88f5 Mon Sep 17 00:00:00 2001
From: Scott Nonnenberg <scott@nonnenberg.com>
Date: Tue, 6 Jun 2017 17:28:32 -0700
Subject: [PATCH] Fix: Ensure that attachments are always rendered

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
---
 js/views/message_view.js | 36 +++++++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/js/views/message_view.js b/js/views/message_view.js
index d87f214123..db3f4825ec 100644
--- a/js/views/message_view.js
+++ b/js/views/message_view.js
@@ -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;
             }
 
@@ -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));
         }
     });