When replying to a threaded inline, put the new inline in the right place in the thread

Summary:
Fixes T10563. If you have a thread like this:

```
> A
  > B
  > C
```

...and you reply to "B", we should put the new inline below "B".

We currently do when you reload the page, but the initial edit goes at the bottom always (below "C").

Test Plan: {F4963015}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10563

Differential Revision: https://secure.phabricator.com/D17938

resources/celerity/map.php

@@ -13,7 +13,7 @@ return array(
     'core.pkg.js' => '0f87a6eb',
     'darkconsole.pkg.js' => '1f9a31bc',
     'differential.pkg.css' => 'ea471cb0',
-    'differential.pkg.js' => '4a466790',
+    'differential.pkg.js' => '31e1b646',
     'diffusion.pkg.css' => 'b93d9b8c',
     'diffusion.pkg.js' => '84c8f8fd',
     'favicon.ico' => '30672e08',
@@ -390,9 +390,9 @@ return array(
     'rsrc/js/application/dashboard/behavior-dashboard-move-panels.js' => '408bf173',
     'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '453c5375',
     'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => 'd4eecc63',
-    'rsrc/js/application/diff/DiffChangeset.js' => '68758d99',
+    'rsrc/js/application/diff/DiffChangeset.js' => 'ec32b333',
     'rsrc/js/application/diff/DiffChangesetList.js' => '796922e0',
-    'rsrc/js/application/diff/DiffInline.js' => '1afe9760',
+    'rsrc/js/application/diff/DiffInline.js' => '3337c065',
     'rsrc/js/application/diff/behavior-preview-link.js' => '051c7832',
     'rsrc/js/application/differential/behavior-comment-preview.js' => 'b064af76',
     'rsrc/js/application/differential/behavior-diff-radios.js' => 'e1ff79b1',
@@ -777,9 +777,9 @@ return array(
     'phabricator-darklog' => 'c8e1ffe3',
     'phabricator-darkmessage' => 'c48cccdd',
     'phabricator-dashboard-css' => 'fe5b1869',
-    'phabricator-diff-changeset' => '68758d99',
+    'phabricator-diff-changeset' => 'ec32b333',
     'phabricator-diff-changeset-list' => '796922e0',
-    'phabricator-diff-inline' => '1afe9760',
+    'phabricator-diff-inline' => '3337c065',
     'phabricator-drag-and-drop-file-upload' => '58dea2fa',
     'phabricator-draggable-list' => 'bea6e7f4',
     'phabricator-fatal-config-template-css' => '8f18fa41',
@@ -1028,9 +1028,6 @@ return array(
       'javelin-util',
       'phabricator-keyboard-shortcut-manager',
     ),
-    '1afe9760' => array(
-      'javelin-dom',
-    ),
     '1bd28176' => array(
       'javelin-install',
       'javelin-dom',
@@ -1130,6 +1127,9 @@ return array(
       'javelin-dom',
       'javelin-workflow',
     ),
+    '3337c065' => array(
+      'javelin-dom',
+    ),
     '3ab51e2c' => array(
       'javelin-behavior',
       'javelin-behavior-device',
@@ -1403,17 +1403,6 @@ return array(
       'javelin-dom',
       'phabricator-notification',
     ),
-    '68758d99' => array(
-      'javelin-dom',
-      'javelin-util',
-      'javelin-stratcom',
-      'javelin-install',
-      'javelin-workflow',
-      'javelin-router',
-      'javelin-behavior-device',
-      'javelin-vector',
-      'phabricator-diff-inline',
-    ),
     '6882e80a' => array(
       'javelin-dom',
     ),
@@ -2142,6 +2131,17 @@ return array(
       'javelin-dom',
       'phabricator-draggable-list',
     ),
+    'ec32b333' => array(
+      'javelin-dom',
+      'javelin-util',
+      'javelin-stratcom',
+      'javelin-install',
+      'javelin-workflow',
+      'javelin-router',
+      'javelin-behavior-device',
+      'javelin-vector',
+      'phabricator-diff-inline',
+    ),
     'eded9ee8' => array(
       'javelin-behavior',
       'javelin-typeahead-ondemand-source',

webroot/rsrc/js/application/diff/DiffChangeset.js

@@ -581,8 +581,16 @@ JX.install('DiffChangeset', {
     },
 
     getInlineByID: function(id) {
+      return this._queryInline('id', id);
+    },
+
+    getInlineByPHID: function(phid) {
+      return this._queryInline('phid', phid);
+    },
+
+    _queryInline: function(field, value) {
       // First, look for the inline in the objects we've already built.
-      var inline = this._findInlineByID(id);
+      var inline = this._findInline(field, value);
       if (inline) {
         return inline;
       }
@@ -590,13 +598,24 @@ JX.install('DiffChangeset', {
       // If we haven't found a matching inline yet, rebuild all the inlines
       // present in the document, then look again.
       this._rebuildAllInlines();
-      return this._findInlineByID(id);
+      return this._findInline(field, value);
     },
 
-    _findInlineByID: function(id) {
+    _findInline: function(field, value) {
       for (var ii = 0; ii < this._inlines.length; ii++) {
         var inline = this._inlines[ii];
-        if (inline.getID() == id) {
+
+        var target;
+        switch (field) {
+          case 'id':
+            target = inline.getID();
+            break;
+          case 'phid':
+            target = inline.getPHID();
+            break;
+        }
+
+        if (target == value) {
           return inline;
         }
       }

webroot/rsrc/js/application/diff/DiffInline.js

@@ -63,6 +63,8 @@ JX.install('DiffInline', {
         (this.getDisplaySide() == 'right') ||
         (data.left != data.right);
 
+      this._replyToCommentPHID = data.replyToCommentPHID;
+
       this.setInvisible(false);
 
       return this;
@@ -100,11 +102,45 @@ JX.install('DiffInline', {
 
       this._replyToCommentPHID = inline._phid;
 
-      // TODO: This should insert correctly into the thread, not just at the
-      // bottom.
+      var changeset = this.getChangeset();
+
+      // We're going to figure out where in the document to position the new
+      // inline. Normally, it goes after any existing inline rows (so if
+      // several inlines reply to the same line, they appear in chronological
+      // order).
+
+      // However: if inlines are threaded, we want to put the new inline in
+      // the right place in the thread. This might be somewhere in the middle,
+      // so we need to do a bit more work to figure it out.
+
+      // To find the right place in the thread, we're going to look for any
+      // inline which is at or above the level of the comment we're replying
+      // to. This means we've reached a new fork of the thread, and should
+      // put our new inline before the comment we found.
+      var ancestor_map = {};
+      var ancestor = inline;
+      var reply_phid;
+      while (ancestor) {
+        reply_phid = ancestor.getReplyToCommentPHID();
+        if (!reply_phid) {
+          break;
+        }
+        ancestor_map[reply_phid] = true;
+        ancestor = changeset.getInlineByPHID(reply_phid);
+      }
+
       var parent_row = inline._row;
       var target_row = parent_row.nextSibling;
       while (target_row && JX.Stratcom.hasSigil(target_row, 'inline-row')) {
+        var target = changeset.getInlineForRow(target_row);
+        reply_phid = target.getReplyToCommentPHID();
+
+        // If we found an inline which is replying directly to some ancestor
+        // of this new comment, this is where the new rows go.
+        if (ancestor_map.hasOwnProperty(reply_phid)) {
+          break;
+        }
+
         target_row = target_row.nextSibling;
       }
 
@@ -318,6 +354,10 @@ JX.install('DiffInline', {
       return this._id;
     },
 
+    getPHID: function() {
+      return this._phid;
+    },
+
     getChangesetID: function() {
       return this._changesetID;
     },