From 5b8b5f2141485415999eb9dd129dbb84d98c5eae Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Sat, 27 Mar 2021 09:16:12 -0700
Subject: [PATCH 01/14] Correct issue with "bindings" conduit attachment

Summary: Ref T13641. These conditions are swapped, and "activeBindings" loads more data than necessary while "bindings" doesn't load enough.

Test Plan: Called method with each attachment, got good results instead of an exception.

Maniphest Tasks: T13641

Differential Revision: https://secure.phabricator.com/D21657
---
 .../engineextension/AlmanacBindingsSearchEngineAttachment.php | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/applications/almanac/engineextension/AlmanacBindingsSearchEngineAttachment.php b/src/applications/almanac/engineextension/AlmanacBindingsSearchEngineAttachment.php
index c4f6b823ee..903d85e564 100644
--- a/src/applications/almanac/engineextension/AlmanacBindingsSearchEngineAttachment.php
+++ b/src/applications/almanac/engineextension/AlmanacBindingsSearchEngineAttachment.php
@@ -26,9 +26,9 @@ final class AlmanacBindingsSearchEngineAttachment
     $query->needProperties(true);
 
     if ($this->getIsActive()) {
-      $query->needBindings(true);
-    } else {
       $query->needActiveBindings(true);
+    } else {
+      $query->needBindings(true);
     }
   }
 

From aa70b008f3a168da5289ac63b87aec6fcf517d6c Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Sun, 28 Mar 2021 10:52:29 -0700
Subject: [PATCH 02/14] Skip "git for-each-ref" when identifying deleted
 commits

Summary:
Ref T13647. The ref discovery process prunes commits that no longer exist in the repository before executing "git log <new heads> --not <old heads>" to identify newly published commits.

If we don't do this, the "git log" command will fail if any old head has been pruned from the repository.

Currently, this test for missing commits starts with a call to "git for-each-ref" to attempt to resolve symbols as tag or branch names, but:

  - this is painfully slow in repositories with many refs; and
  - this is incorrect (not consistent with "git" behavior) for 40-character hex strings, which Git will never resolve as symbolic names.

Instead, when a symbol is a 40-character hex string, skip "git for-each-ref" and jump directly to "git cat-file --batch-check".

Test Plan:
  - Ran `bin/repository update` in a repository with 65K refs and extra debugging info.
    - Before: took ~30s, three calls to `git for-each-ref`.
    - After: took ~20s, two calls to `git for-each-ref`. Same resolution result on queries.

Maniphest Tasks: T13647

Differential Revision: https://secure.phabricator.com/D21658
---
 .../DiffusionLowLevelResolveRefsQuery.php     | 82 +++++++++++--------
 1 file changed, 50 insertions(+), 32 deletions(-)

diff --git a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelResolveRefsQuery.php b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelResolveRefsQuery.php
index 197fa285dc..0bbba1dc18 100644
--- a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelResolveRefsQuery.php
+++ b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelResolveRefsQuery.php
@@ -63,48 +63,66 @@ final class DiffusionLowLevelResolveRefsQuery
     $unresolved = array_fuse($this->refs);
     $results = array();
 
-    // First, resolve branches and tags.
-    $ref_map = id(new DiffusionLowLevelGitRefQuery())
-      ->setRepository($repository)
-      ->withRefTypes(
-        array(
-          PhabricatorRepositoryRefCursor::TYPE_BRANCH,
-          PhabricatorRepositoryRefCursor::TYPE_TAG,
-        ))
-      ->execute();
-    $ref_map = mgroup($ref_map, 'getShortName');
-
-    $tag_prefix = 'refs/tags/';
+    $possible_symbols = array();
     foreach ($unresolved as $ref) {
-      if (empty($ref_map[$ref])) {
+
+      // See T13647. If this symbol is exactly 40 hex characters long, it may
+      // never resolve as a branch or tag name. Filter these symbols out for
+      // consistency with Git behavior -- and to avoid an expensive
+      // "git for-each-ref" when resolving only commit hashes, which happens
+      // during repository updates.
+
+      if (preg_match('(^[a-f0-9]{40}\z)', $ref)) {
         continue;
       }
 
-      foreach ($ref_map[$ref] as $result) {
-        $fields = $result->getRawFields();
-        $objectname = idx($fields, 'refname');
-        if (!strncmp($objectname, $tag_prefix, strlen($tag_prefix))) {
-          $type = 'tag';
-        } else {
-          $type = 'branch';
+      $possible_symbols[$ref] = $ref;
+    }
+
+    // First, resolve branches and tags.
+    if ($possible_symbols) {
+      $ref_map = id(new DiffusionLowLevelGitRefQuery())
+        ->setRepository($repository)
+        ->withRefTypes(
+          array(
+            PhabricatorRepositoryRefCursor::TYPE_BRANCH,
+            PhabricatorRepositoryRefCursor::TYPE_TAG,
+          ))
+        ->execute();
+      $ref_map = mgroup($ref_map, 'getShortName');
+
+      $tag_prefix = 'refs/tags/';
+      foreach ($possible_symbols as $ref) {
+        if (empty($ref_map[$ref])) {
+          continue;
         }
 
-        $info = array(
-          'type' => $type,
-          'identifier' => $result->getCommitIdentifier(),
-        );
-
-        if ($type == 'tag') {
-          $alternate = idx($fields, 'objectname');
-          if ($alternate) {
-            $info['alternate'] = $alternate;
+        foreach ($ref_map[$ref] as $result) {
+          $fields = $result->getRawFields();
+          $objectname = idx($fields, 'refname');
+          if (!strncmp($objectname, $tag_prefix, strlen($tag_prefix))) {
+            $type = 'tag';
+          } else {
+            $type = 'branch';
           }
+
+          $info = array(
+            'type' => $type,
+            'identifier' => $result->getCommitIdentifier(),
+          );
+
+          if ($type == 'tag') {
+            $alternate = idx($fields, 'objectname');
+            if ($alternate) {
+              $info['alternate'] = $alternate;
+            }
+          }
+
+          $results[$ref][] = $info;
         }
 
-        $results[$ref][] = $info;
+        unset($unresolved[$ref]);
       }
-
-      unset($unresolved[$ref]);
     }
 
     // If we resolved everything, we're done.

From 87c6c270b43599988fe7e2bb3563475d89f4b516 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 18 Mar 2021 11:29:59 -0700
Subject: [PATCH 03/14] Fix an issue where inlines could be duplicated in the
 client list

Summary:
Ref T13559. D21261 added caching here, but the logic in rebuilding inlines wasn't quite correct, and could lead to us double-appending.

Instead, when rebuilding, unconditionally discard the old list.

Test Plan:
  - Added inline comments to a file in Differential.
  - Marked some done.
  - Scrolled so the inline comment header was visible, saw "X / Y Comments" button in header.
  - Clicked "Show 20 more lines" on the changeset with inlines (or toggle "View Unified" / "View Side-by-Side", or other interactions likely work too).
    - Before: saw "X / Y" change improperly (because inlines in that file were double-counted).
    - After: saw stable count.
  - Grepped for "differential-inline-comment-refresh", got no hits, concluded this event has no listeners.

Maniphest Tasks: T13559

Differential Revision: https://secure.phabricator.com/D21642
---
 resources/celerity/map.php                    | 34 +++++++++----------
 .../rsrc/js/application/diff/DiffChangeset.js |  7 +---
 2 files changed, 18 insertions(+), 23 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index c7ac86ab3c..b400dfec29 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -13,7 +13,7 @@ return array(
     'core.pkg.js' => 'ab3502fe',
     'dark-console.pkg.js' => '187792c2',
     'differential.pkg.css' => 'ffb69e3d',
-    'differential.pkg.js' => '5986f349',
+    'differential.pkg.js' => 'd1150814',
     'diffusion.pkg.css' => '42c75c37',
     'diffusion.pkg.js' => '78c9885d',
     'maniphest.pkg.css' => '35995d6d',
@@ -383,7 +383,7 @@ return array(
     'rsrc/js/application/dashboard/behavior-dashboard-move-panels.js' => 'a2ab19be',
     'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '1e413dc9',
     'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '0116d3e8',
-    'rsrc/js/application/diff/DiffChangeset.js' => '3b6e1fde',
+    'rsrc/js/application/diff/DiffChangeset.js' => 'd7d3ba75',
     'rsrc/js/application/diff/DiffChangesetList.js' => 'cc2c5de5',
     'rsrc/js/application/diff/DiffInline.js' => '511a1315',
     'rsrc/js/application/diff/DiffPathView.js' => '8207abf9',
@@ -785,7 +785,7 @@ return array(
     'phabricator-darklog' => '3b869402',
     'phabricator-darkmessage' => '26cd4b73',
     'phabricator-dashboard-css' => '5a205b9d',
-    'phabricator-diff-changeset' => '3b6e1fde',
+    'phabricator-diff-changeset' => 'd7d3ba75',
     'phabricator-diff-changeset-list' => 'cc2c5de5',
     'phabricator-diff-inline' => '511a1315',
     'phabricator-diff-path-view' => '8207abf9',
@@ -1248,20 +1248,6 @@ return array(
       'javelin-behavior',
       'phabricator-prefab',
     ),
-    '3b6e1fde' => array(
-      'javelin-dom',
-      'javelin-util',
-      'javelin-stratcom',
-      'javelin-install',
-      'javelin-workflow',
-      'javelin-router',
-      'javelin-behavior-device',
-      'javelin-vector',
-      'phabricator-diff-inline',
-      'phabricator-diff-path-view',
-      'phuix-button-view',
-      'javelin-external-editor-link-engine',
-    ),
     '3dc5ad43' => array(
       'javelin-behavior',
       'javelin-stratcom',
@@ -2129,6 +2115,20 @@ return array(
     'd4cc2d2a' => array(
       'javelin-install',
     ),
+    'd7d3ba75' => array(
+      'javelin-dom',
+      'javelin-util',
+      'javelin-stratcom',
+      'javelin-install',
+      'javelin-workflow',
+      'javelin-router',
+      'javelin-behavior-device',
+      'javelin-vector',
+      'phabricator-diff-inline',
+      'phabricator-diff-path-view',
+      'phuix-button-view',
+      'javelin-external-editor-link-engine',
+    ),
     'd8a86cfb' => array(
       'javelin-behavior',
       'javelin-dom',
diff --git a/webroot/rsrc/js/application/diff/DiffChangeset.js b/webroot/rsrc/js/application/diff/DiffChangeset.js
index 668634cb30..af25b59c6b 100644
--- a/webroot/rsrc/js/application/diff/DiffChangeset.js
+++ b/webroot/rsrc/js/application/diff/DiffChangeset.js
@@ -671,9 +671,6 @@ JX.install('DiffChangeset', {
       // Code shared by autoload and context responses.
 
       this._loadChangesetState(response);
-
-      JX.Stratcom.invoke('differential-inline-comment-refresh');
-
       this._rebuildAllInlines();
 
       JX.Stratcom.invoke('resize');
@@ -846,9 +843,7 @@ JX.install('DiffChangeset', {
     },
 
     _rebuildAllInlines: function() {
-      if (this._inlines === null) {
-        this._inlines = [];
-      }
+      this._inlines = [];
 
       var rows = JX.DOM.scry(this._node, 'tr');
       var ii;

From b964731b6a6bb10c734086375ec14db51de9ec82 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 23 Mar 2021 11:48:58 -0700
Subject: [PATCH 04/14] Make inline "ContentState" a client object, and track
 "hasSuggestion" on it

Summary:
Ref T13559. In an effort to ultimately fix the "quote + cancel" bug, begin formalizing content states on the client.

This creates a "ContentState" client object and moves the authoritative storage for the "hasSuggestion" property to it.

Test Plan: Created inlines, etc. See followups.

Maniphest Tasks: T13559

Differential Revision: https://secure.phabricator.com/D21645
---
 resources/celerity/map.php                    | 19 +++++++++++-----
 resources/celerity/packages.php               |  1 +
 .../rsrc/js/application/diff/DiffInline.js    | 15 ++++++++++---
 .../diff/DiffInlineContentState.js            | 22 +++++++++++++++++++
 4 files changed, 48 insertions(+), 9 deletions(-)
 create mode 100644 webroot/rsrc/js/application/diff/DiffInlineContentState.js

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index b400dfec29..a55a0986a6 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -13,7 +13,7 @@ return array(
     'core.pkg.js' => 'ab3502fe',
     'dark-console.pkg.js' => '187792c2',
     'differential.pkg.css' => 'ffb69e3d',
-    'differential.pkg.js' => 'd1150814',
+    'differential.pkg.js' => '5e0c7197',
     'diffusion.pkg.css' => '42c75c37',
     'diffusion.pkg.js' => '78c9885d',
     'maniphest.pkg.css' => '35995d6d',
@@ -385,7 +385,8 @@ return array(
     'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '0116d3e8',
     'rsrc/js/application/diff/DiffChangeset.js' => 'd7d3ba75',
     'rsrc/js/application/diff/DiffChangesetList.js' => 'cc2c5de5',
-    'rsrc/js/application/diff/DiffInline.js' => '511a1315',
+    'rsrc/js/application/diff/DiffInline.js' => '09e0c6e5',
+    'rsrc/js/application/diff/DiffInlineContentState.js' => 'cb9e5396',
     'rsrc/js/application/diff/DiffPathView.js' => '8207abf9',
     'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b',
     'rsrc/js/application/differential/behavior-diff-radios.js' => '925fe8cd',
@@ -787,7 +788,8 @@ return array(
     'phabricator-dashboard-css' => '5a205b9d',
     'phabricator-diff-changeset' => 'd7d3ba75',
     'phabricator-diff-changeset-list' => 'cc2c5de5',
-    'phabricator-diff-inline' => '511a1315',
+    'phabricator-diff-inline' => '09e0c6e5',
+    'phabricator-diff-inline-content-state' => 'cb9e5396',
     'phabricator-diff-path-view' => '8207abf9',
     'phabricator-diff-tree-view' => '5d83623b',
     'phabricator-drag-and-drop-file-upload' => '4370900d',
@@ -998,6 +1000,10 @@ return array(
       'herald-rule-editor',
       'javelin-behavior',
     ),
+    '09e0c6e5' => array(
+      'javelin-dom',
+      'phabricator-diff-inline-content-state',
+    ),
     '0ad8d31f' => array(
       'javelin-behavior',
       'javelin-stratcom',
@@ -1399,9 +1405,6 @@ return array(
       'javelin-stratcom',
       'javelin-dom',
     ),
-    '511a1315' => array(
-      'javelin-dom',
-    ),
     '5202e831' => array(
       'javelin-install',
       'javelin-dom',
@@ -2086,6 +2089,9 @@ return array(
       'javelin-workflow',
       'javelin-json',
     ),
+    'cb9e5396' => array(
+      'javelin-dom',
+    ),
     'cc2c5de5' => array(
       'javelin-install',
       'phuix-button-view',
@@ -2441,6 +2447,7 @@ return array(
       'javelin-behavior-phabricator-object-selector',
       'javelin-behavior-repository-crossreference',
       'javelin-behavior-aphront-more',
+      'phabricator-diff-inline-content-state',
       'phabricator-diff-inline',
       'phabricator-diff-changeset',
       'phabricator-diff-changeset-list',
diff --git a/resources/celerity/packages.php b/resources/celerity/packages.php
index 47e13932de..fc5a4f11b6 100644
--- a/resources/celerity/packages.php
+++ b/resources/celerity/packages.php
@@ -212,6 +212,7 @@ return array(
 
     'javelin-behavior-aphront-more',
 
+    'phabricator-diff-inline-content-state',
     'phabricator-diff-inline',
     'phabricator-diff-changeset',
     'phabricator-diff-changeset-list',
diff --git a/webroot/rsrc/js/application/diff/DiffInline.js b/webroot/rsrc/js/application/diff/DiffInline.js
index 705d95e70a..4701919eef 100644
--- a/webroot/rsrc/js/application/diff/DiffInline.js
+++ b/webroot/rsrc/js/application/diff/DiffInline.js
@@ -1,12 +1,14 @@
 /**
  * @provides phabricator-diff-inline
  * @requires javelin-dom
+ *           phabricator-diff-inline-content-state
  * @javelin
  */
 
 JX.install('DiffInline', {
 
   construct : function() {
+    this._activeContentState = new JX.DiffInlineContentState();
   },
 
   members: {
@@ -53,6 +55,8 @@ JX.install('DiffInline', {
     _isSelected: false,
     _canSuggestEdit: false,
 
+    _activeContentState: null,
+
     bindToRow: function(row) {
       this._row = row;
 
@@ -785,8 +789,13 @@ JX.install('DiffInline', {
       this.triggerDraft();
     },
 
+    _getActiveContentState: function() {
+      return this._activeContentState;
+    },
+
     setHasSuggestion: function(has_suggestion) {
-      this._hasSuggestion = has_suggestion;
+      var state = this._getActiveContentState();
+      state.setHasSuggestion(has_suggestion);
 
       var button = this._getSuggestionButton();
       var pht = this.getChangeset().getChangesetList().getTranslations();
@@ -806,7 +815,7 @@ JX.install('DiffInline', {
     },
 
     getHasSuggestion: function() {
-      return this._hasSuggestion;
+      return this._getActiveContentState().getHasSuggestion();
     },
 
     save: function() {
@@ -920,7 +929,7 @@ JX.install('DiffInline', {
         state.suggestionText = node.value;
       }
 
-      state.hasSuggestion = this.getHasSuggestion();
+      state.hasSuggestion = this._getActiveContentState().getHasSuggestion();
 
       return state;
     },
diff --git a/webroot/rsrc/js/application/diff/DiffInlineContentState.js b/webroot/rsrc/js/application/diff/DiffInlineContentState.js
new file mode 100644
index 0000000000..2641b10bb4
--- /dev/null
+++ b/webroot/rsrc/js/application/diff/DiffInlineContentState.js
@@ -0,0 +1,22 @@
+/**
+ * @provides phabricator-diff-inline-content-state
+ * @requires javelin-dom
+ * @javelin
+ */
+
+JX.install('DiffInlineContentState', {
+
+  construct : function() {
+
+  },
+
+  properties: {
+    text: null,
+    suggestionText: null,
+    hasSuggestion: false
+  },
+
+  members: {
+  }
+
+});

From cb00cb99e2445b12345c7a9adc2107ceb08652f4 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 23 Mar 2021 12:02:32 -0700
Subject: [PATCH 05/14] Make client inlines track an "active" state

Summary:
Ref T13559. Rather than reading from the document, make client inlines actively track their current "active" state.

The "active" state is what the user currently sees in the client UI.

Test Plan: Created inlines, etc. See followups.

Maniphest Tasks: T13559

Differential Revision: https://secure.phabricator.com/D21646
---
 resources/celerity/map.php                    | 24 ++++-----
 .../rsrc/js/application/diff/DiffInline.js    | 29 +++++------
 .../diff/DiffInlineContentState.js            | 49 +++++++++++++++++++
 3 files changed, 74 insertions(+), 28 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index a55a0986a6..c95b0e350c 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -13,7 +13,7 @@ return array(
     'core.pkg.js' => 'ab3502fe',
     'dark-console.pkg.js' => '187792c2',
     'differential.pkg.css' => 'ffb69e3d',
-    'differential.pkg.js' => '5e0c7197',
+    'differential.pkg.js' => '68a2e7be',
     'diffusion.pkg.css' => '42c75c37',
     'diffusion.pkg.js' => '78c9885d',
     'maniphest.pkg.css' => '35995d6d',
@@ -385,8 +385,8 @@ return array(
     'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '0116d3e8',
     'rsrc/js/application/diff/DiffChangeset.js' => 'd7d3ba75',
     'rsrc/js/application/diff/DiffChangesetList.js' => 'cc2c5de5',
-    'rsrc/js/application/diff/DiffInline.js' => '09e0c6e5',
-    'rsrc/js/application/diff/DiffInlineContentState.js' => 'cb9e5396',
+    'rsrc/js/application/diff/DiffInline.js' => 'a5f196da',
+    'rsrc/js/application/diff/DiffInlineContentState.js' => '68e6339d',
     'rsrc/js/application/diff/DiffPathView.js' => '8207abf9',
     'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b',
     'rsrc/js/application/differential/behavior-diff-radios.js' => '925fe8cd',
@@ -788,8 +788,8 @@ return array(
     'phabricator-dashboard-css' => '5a205b9d',
     'phabricator-diff-changeset' => 'd7d3ba75',
     'phabricator-diff-changeset-list' => 'cc2c5de5',
-    'phabricator-diff-inline' => '09e0c6e5',
-    'phabricator-diff-inline-content-state' => 'cb9e5396',
+    'phabricator-diff-inline' => 'a5f196da',
+    'phabricator-diff-inline-content-state' => '68e6339d',
     'phabricator-diff-path-view' => '8207abf9',
     'phabricator-diff-tree-view' => '5d83623b',
     'phabricator-drag-and-drop-file-upload' => '4370900d',
@@ -1000,10 +1000,6 @@ return array(
       'herald-rule-editor',
       'javelin-behavior',
     ),
-    '09e0c6e5' => array(
-      'javelin-dom',
-      'phabricator-diff-inline-content-state',
-    ),
     '0ad8d31f' => array(
       'javelin-behavior',
       'javelin-stratcom',
@@ -1544,6 +1540,9 @@ return array(
       'javelin-install',
       'javelin-dom',
     ),
+    '68e6339d' => array(
+      'javelin-dom',
+    ),
     '6a1583a8' => array(
       'javelin-behavior',
       'javelin-history',
@@ -1872,6 +1871,10 @@ return array(
       'javelin-install',
       'javelin-dom',
     ),
+    'a5f196da' => array(
+      'javelin-dom',
+      'phabricator-diff-inline-content-state',
+    ),
     'a77e2cbd' => array(
       'javelin-behavior',
       'javelin-stratcom',
@@ -2089,9 +2092,6 @@ return array(
       'javelin-workflow',
       'javelin-json',
     ),
-    'cb9e5396' => array(
-      'javelin-dom',
-    ),
     'cc2c5de5' => array(
       'javelin-install',
       'phuix-button-view',
diff --git a/webroot/rsrc/js/application/diff/DiffInline.js b/webroot/rsrc/js/application/diff/DiffInline.js
index 4701919eef..d5e87ed431 100644
--- a/webroot/rsrc/js/application/diff/DiffInline.js
+++ b/webroot/rsrc/js/application/diff/DiffInline.js
@@ -416,25 +416,12 @@ JX.install('DiffInline', {
         .send();
     },
 
-    _getContentState: function() {
-      var state;
-
-      if (this._editRow) {
-        state = this._readFormState(this._editRow);
-      } else {
-        state = this._originalState;
-      }
-
-      return state;
-    },
-
     reply: function(with_quote) {
       this._closeMenu();
 
       var content_state = this._newContentState();
       if (with_quote) {
-        var text = this._getContentState().text;
-        text = '> ' + text.replace(/\n/g, '\n> ') + '\n\n';
+        var text = this._getActiveContentState().getTextForQuote();
         content_state.text = text;
       }
 
@@ -607,6 +594,9 @@ JX.install('DiffInline', {
     _readInlineState: function(state) {
       this._id = state.id;
       this._originalState = state.contentState;
+
+      this._getActiveContentState().readWireFormat(state.contentState);
+
       this._canSuggestEdit = state.canSuggestEdit;
     },
 
@@ -790,7 +780,13 @@ JX.install('DiffInline', {
     },
 
     _getActiveContentState: function() {
-      return this._activeContentState;
+      var state = this._activeContentState;
+
+      if (this._editRow) {
+        state.readForm(this._editRow);
+      }
+
+      return state;
     },
 
     setHasSuggestion: function(has_suggestion) {
@@ -819,12 +815,13 @@ JX.install('DiffInline', {
     },
 
     save: function() {
+      var state = this._getActiveContentState();
       var handler = JX.bind(this, this._onsubmitresponse);
 
       this.setLoading(true);
 
       var uri = this._getInlineURI();
-      var data = this._newRequestData('save', this._getContentState());
+      var data = this._newRequestData('save', state.getWireFormat());
 
       new JX.Request(uri, handler)
         .setData(data)
diff --git a/webroot/rsrc/js/application/diff/DiffInlineContentState.js b/webroot/rsrc/js/application/diff/DiffInlineContentState.js
index 2641b10bb4..b1668f962c 100644
--- a/webroot/rsrc/js/application/diff/DiffInlineContentState.js
+++ b/webroot/rsrc/js/application/diff/DiffInlineContentState.js
@@ -17,6 +17,55 @@ JX.install('DiffInlineContentState', {
   },
 
   members: {
+    readForm: function(row) {
+      var node;
+
+      try {
+        node = JX.DOM.find(row, 'textarea', 'inline-content-text');
+        this.setText(node.value);
+      } catch (ex) {
+        this.setText(null);
+      }
+
+      node = this._getSuggestionNode(row);
+      if (node) {
+        this.setSuggestionText(node.value);
+      } else {
+        this.setSuggestionText(null);
+      }
+
+      return this;
+    },
+
+    getWireFormat: function() {
+      return {
+        text: this.getText(),
+        suggestionText: this.getSuggestionText(),
+        hasSuggestion: this.getHasSuggestion()
+      };
+    },
+
+    readWireFormat: function(map) {
+      this.setText(map.text || null);
+      this.setSuggestionText(map.suggestionText || null);
+      this.setHasSuggestion(!!map.hasSuggestion);
+
+      return this;
+    },
+
+    getTextForQuote: function() {
+      var text = this.getText();
+      text = '> ' + text.replace(/\n/g, '\n> ') + '\n\n';
+      return text;
+    },
+
+    _getSuggestionNode: function(row) {
+      try {
+        return JX.DOM.find(row, 'textarea', 'inline-content-suggestion');
+      } catch (ex) {
+        return null;
+      }
+    }
   }
 
 });

From 0f04d9e58458dbd0319381549539ef47dad2d15b Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 23 Mar 2021 12:11:21 -0700
Subject: [PATCH 06/14] Remove direct reads of form state from main Inline
 client code

Summary:
Ref T13559. Instead of directly reading form state, make all callers use the "active" state instead. The state reads the form.

No functional changes, just clarifying responsiblites.

Test Plan: Created inlines, etc. See followup changes.

Maniphest Tasks: T13559

Differential Revision: https://secure.phabricator.com/D21647
---
 resources/celerity/map.php                    | 14 +++++-----
 .../rsrc/js/application/diff/DiffInline.js    | 28 ++-----------------
 2 files changed, 10 insertions(+), 32 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index c95b0e350c..d09a30d901 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -13,7 +13,7 @@ return array(
     'core.pkg.js' => 'ab3502fe',
     'dark-console.pkg.js' => '187792c2',
     'differential.pkg.css' => 'ffb69e3d',
-    'differential.pkg.js' => '68a2e7be',
+    'differential.pkg.js' => '442567d7',
     'diffusion.pkg.css' => '42c75c37',
     'diffusion.pkg.js' => '78c9885d',
     'maniphest.pkg.css' => '35995d6d',
@@ -385,7 +385,7 @@ return array(
     'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '0116d3e8',
     'rsrc/js/application/diff/DiffChangeset.js' => 'd7d3ba75',
     'rsrc/js/application/diff/DiffChangesetList.js' => 'cc2c5de5',
-    'rsrc/js/application/diff/DiffInline.js' => 'a5f196da',
+    'rsrc/js/application/diff/DiffInline.js' => 'fdebbba6',
     'rsrc/js/application/diff/DiffInlineContentState.js' => '68e6339d',
     'rsrc/js/application/diff/DiffPathView.js' => '8207abf9',
     'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b',
@@ -788,7 +788,7 @@ return array(
     'phabricator-dashboard-css' => '5a205b9d',
     'phabricator-diff-changeset' => 'd7d3ba75',
     'phabricator-diff-changeset-list' => 'cc2c5de5',
-    'phabricator-diff-inline' => 'a5f196da',
+    'phabricator-diff-inline' => 'fdebbba6',
     'phabricator-diff-inline-content-state' => '68e6339d',
     'phabricator-diff-path-view' => '8207abf9',
     'phabricator-diff-tree-view' => '5d83623b',
@@ -1871,10 +1871,6 @@ return array(
       'javelin-install',
       'javelin-dom',
     ),
-    'a5f196da' => array(
-      'javelin-dom',
-      'phabricator-diff-inline-content-state',
-    ),
     'a77e2cbd' => array(
       'javelin-behavior',
       'javelin-stratcom',
@@ -2235,6 +2231,10 @@ return array(
     'fdc13e4e' => array(
       'javelin-install',
     ),
+    'fdebbba6' => array(
+      'javelin-dom',
+      'phabricator-diff-inline-content-state',
+    ),
     'ff688a7a' => array(
       'owners-path-editor',
       'javelin-behavior',
diff --git a/webroot/rsrc/js/application/diff/DiffInline.js b/webroot/rsrc/js/application/diff/DiffInline.js
index d5e87ed431..32d47562e3 100644
--- a/webroot/rsrc/js/application/diff/DiffInline.js
+++ b/webroot/rsrc/js/application/diff/DiffInline.js
@@ -612,7 +612,7 @@ JX.install('DiffInline', {
       // read and preserve the text so "Undo" restores it.
       var state = null;
       if (this._editRow) {
-        state = this._readFormState(this._editRow);
+        state = this._getActiveContentState().getWireFormat();
         JX.DOM.remove(this._editRow);
         this._editRow = null;
       }
@@ -856,7 +856,7 @@ JX.install('DiffInline', {
     },
 
     cancel: function() {
-      var state = this._readFormState(this._editRow);
+      var state = this._getActiveContentState().getWireFormat();
 
       JX.DOM.remove(this._editRow);
       this._editRow = null;
@@ -909,28 +909,6 @@ JX.install('DiffInline', {
       }
     },
 
-    _readFormState: function(row) {
-      var state = this._newContentState();
-
-      var node;
-
-      try {
-        node = JX.DOM.find(row, 'textarea', 'inline-content-text');
-        state.text = node.value;
-      } catch (ex) {
-        // Ignore.
-      }
-
-      node = this._getSuggestionNode(row);
-      if (node) {
-        state.suggestionText = node.value;
-      }
-
-      state.hasSuggestion = this._getActiveContentState().getHasSuggestion();
-
-      return state;
-    },
-
     _getSuggestionNode: function(row) {
       try {
         return JX.DOM.find(row, 'textarea', 'inline-content-suggestion');
@@ -1043,7 +1021,7 @@ JX.install('DiffInline', {
         return null;
       }
 
-      var state = this._readFormState(this._editRow);
+      var state = this._getActiveContentState().getWireFormat();
       if (this._isVoidContentState(state)) {
         return null;
       }

From 60e869f4114f84dc1c68a5beed69d0b577640446 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 23 Mar 2021 12:24:55 -0700
Subject: [PATCH 07/14] Make the client authoritative for "Save" actions

Summary:
Ref T13559. When you click "Save" on an inline comment and it's empty, we may actually delete the comment.

Currently, the client and server both make decisions about whether the comment should be deleted. These decisions may not agree, causing the client state to fall out of sync.

Make the client authoritative about whether it wants to handle the user clicking the "Save" button as an intent to save or an intent to delete.

Test Plan: Saved empty and nonempty inlines. See followup changes.

Maniphest Tasks: T13559

Differential Revision: https://secure.phabricator.com/D21648
---
 resources/celerity/map.php                    | 14 +--
 .../PhabricatorInlineCommentController.php    | 22 ++---
 .../rsrc/js/application/diff/DiffInline.js    | 92 ++++++++++++-------
 3 files changed, 73 insertions(+), 55 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index d09a30d901..8e94ccd067 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -13,7 +13,7 @@ return array(
     'core.pkg.js' => 'ab3502fe',
     'dark-console.pkg.js' => '187792c2',
     'differential.pkg.css' => 'ffb69e3d',
-    'differential.pkg.js' => '442567d7',
+    'differential.pkg.js' => '7747755e',
     'diffusion.pkg.css' => '42c75c37',
     'diffusion.pkg.js' => '78c9885d',
     'maniphest.pkg.css' => '35995d6d',
@@ -385,7 +385,7 @@ return array(
     'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '0116d3e8',
     'rsrc/js/application/diff/DiffChangeset.js' => 'd7d3ba75',
     'rsrc/js/application/diff/DiffChangesetList.js' => 'cc2c5de5',
-    'rsrc/js/application/diff/DiffInline.js' => 'fdebbba6',
+    'rsrc/js/application/diff/DiffInline.js' => '34ccdeda',
     'rsrc/js/application/diff/DiffInlineContentState.js' => '68e6339d',
     'rsrc/js/application/diff/DiffPathView.js' => '8207abf9',
     'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b',
@@ -788,7 +788,7 @@ return array(
     'phabricator-dashboard-css' => '5a205b9d',
     'phabricator-diff-changeset' => 'd7d3ba75',
     'phabricator-diff-changeset-list' => 'cc2c5de5',
-    'phabricator-diff-inline' => 'fdebbba6',
+    'phabricator-diff-inline' => '34ccdeda',
     'phabricator-diff-inline-content-state' => '68e6339d',
     'phabricator-diff-path-view' => '8207abf9',
     'phabricator-diff-tree-view' => '5d83623b',
@@ -1220,6 +1220,10 @@ return array(
       'javelin-stratcom',
       'javelin-workflow',
     ),
+    '34ccdeda' => array(
+      'javelin-dom',
+      'phabricator-diff-inline-content-state',
+    ),
     '34e2a838' => array(
       'aphront-typeahead-control-css',
       'phui-tag-view-css',
@@ -2231,10 +2235,6 @@ return array(
     'fdc13e4e' => array(
       'javelin-install',
     ),
-    'fdebbba6' => array(
-      'javelin-dom',
-      'phabricator-diff-inline-content-state',
-    ),
     'ff688a7a' => array(
       'owners-path-editor',
       'javelin-behavior',
diff --git a/src/infrastructure/diff/PhabricatorInlineCommentController.php b/src/infrastructure/diff/PhabricatorInlineCommentController.php
index 693f66066f..2076c1964e 100644
--- a/src/infrastructure/diff/PhabricatorInlineCommentController.php
+++ b/src/infrastructure/diff/PhabricatorInlineCommentController.php
@@ -186,23 +186,15 @@ abstract class PhabricatorInlineCommentController
         if ($op === 'save') {
           $this->updateCommentContentState($inline);
 
-          $inline->setIsEditing(false);
+          $inline
+            ->setIsEditing(false)
+            ->setIsDeleted(0);
 
-          if (!$inline->isVoidComment($viewer)) {
-            $inline->setIsDeleted(0);
+          $this->saveComment($inline);
 
-            $this->saveComment($inline);
-
-            return $this->buildRenderedCommentResponse(
-              $inline,
-              $this->getIsOnRight());
-          } else {
-            $inline->setIsDeleted(1);
-
-            $this->saveComment($inline);
-
-            return $this->buildEmptyResponse();
-          }
+          return $this->buildRenderedCommentResponse(
+            $inline,
+            $this->getIsOnRight());
         } else {
           // NOTE: At time of writing, the "editing" state of inlines is
           // preserved by simulating a click on "Edit" when the inline loads.
diff --git a/webroot/rsrc/js/application/diff/DiffInline.js b/webroot/rsrc/js/application/diff/DiffInline.js
index 32d47562e3..c17217adfe 100644
--- a/webroot/rsrc/js/application/diff/DiffInline.js
+++ b/webroot/rsrc/js/application/diff/DiffInline.js
@@ -45,6 +45,7 @@ JX.install('DiffInline', {
     _undoRow: null,
     _undoType: null,
     _undoState: null,
+    _preventUndo: false,
 
     _draftRequest: null,
     _skipFocus: false,
@@ -161,6 +162,14 @@ JX.install('DiffInline', {
       return this._endOffset;
     },
 
+    _setPreventUndo: function(prevent_undo) {
+      this._preventUndo = prevent_undo;
+    },
+
+    _getPreventUndo: function() {
+      return this._preventUndo;
+    },
+
     setIsSelected: function(is_selected) {
       this._isSelected = is_selected;
 
@@ -617,7 +626,9 @@ JX.install('DiffInline', {
         this._editRow = null;
       }
 
-      this._drawUndeleteRows(state);
+      if (!this._getPreventUndo()) {
+        this._drawUndeleteRows(state);
+      }
 
       this.setLoading(false);
       this.setDeleted(true);
@@ -815,17 +826,53 @@ JX.install('DiffInline', {
     },
 
     save: function() {
+      if (this._shouldDeleteOnSave()) {
+        this._setPreventUndo(true);
+        this._applyDelete();
+      } else {
+        this._applySave();
+      }
+    },
+
+    _shouldDeleteOnSave: function() {
       var state = this._getActiveContentState();
-      var handler = JX.bind(this, this._onsubmitresponse);
+
+      // TODO: This is greatly simplified because we don't track all the
+      // state we need yet.
+
+      return !state.getText().length;
+    },
+
+    _applySave: function() {
+      var handler = JX.bind(this, this._onsaveresponse);
+
+      var state = this._getActiveContentState();
+      var data = this._newRequestData('save', state.getWireFormat());
+
+      this._applyCall(handler, data);
+    },
+
+    _applyDelete: function() {
+      var handler = JX.bind(this, this._ondeleteresponse);
+
+      var data = this._newRequestData('delete');
+
+      this._applyCall(handler, data);
+    },
+
+    _applyCall: function(handler, data) {
+      var uri = this._getInlineURI();
+
+      var callback = JX.bind(this, function() {
+        this.setLoading(false);
+        handler.apply(null, arguments);
+      });
 
       this.setLoading(true);
 
-      var uri = this._getInlineURI();
-      var data = this._newRequestData('save', state.getWireFormat());
-
-      new JX.Request(uri, handler)
-        .setData(data)
-        .send();
+      new JX.Workflow(uri, data)
+        .setHandler(callback)
+        .start();
     },
 
     undo: function() {
@@ -917,7 +964,7 @@ JX.install('DiffInline', {
       }
     },
 
-    _onsubmitresponse: function(response) {
+    _onsaveresponse: function(response) {
       if (this._editRow) {
         JX.DOM.remove(this._editRow);
         this._editRow = null;
@@ -927,30 +974,9 @@ JX.install('DiffInline', {
       this.setInvisible(false);
       this.setEditing(false);
 
-      this._onupdate(response);
-    },
-
-    _onupdate: function(response) {
-      var new_row;
-      if (response.view) {
-        new_row = this._drawContentRows(JX.$H(response.view).getNode());
-      }
-
-      // TODO: Save the old row so the action it's undo-able if it was a
-      // delete.
-      var remove_old = true;
-      if (remove_old) {
-        JX.DOM.remove(this._row);
-      }
-
-      // If you delete the content on a comment and save it, it acts like a
-      // delete: the server does not return a new row.
-      if (new_row) {
-        this.bindToRow(new_row);
-      } else {
-        this.setDeleted(true);
-        this._row = null;
-      }
+      var new_row = this._drawContentRows(JX.$H(response.view).getNode());
+      JX.DOM.remove(this._row);
+      this.bindToRow(new_row);
 
       this._didUpdate();
     },

From d30c3a961cd9b11ef5f41c2859e4f825bb92fcb2 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 23 Mar 2021 12:39:08 -0700
Subject: [PATCH 08/14] Make the client authoritative for "Cancel" actions

Summary:
Ref T13559. When the user clicks the "Cancel" button, we sometimes take it to mean "delete" (when the comment is empty).

Both the client and server make a decision about this, and they may not agree, which causes the client to fall out of sync.

Make the client responsible for deciding whether it wants to interpret a click on the "Cancel" button as a "revert" or a "delete".

Test Plan: Cancelled empty and nonempty comments, etc. See followup changes.

Maniphest Tasks: T13559

Differential Revision: https://secure.phabricator.com/D21649
---
 resources/celerity/map.php                    | 14 ++--
 .../PhabricatorInlineCommentController.php    |  4 -
 .../rsrc/js/application/diff/DiffInline.js    | 84 ++++++++++---------
 3 files changed, 53 insertions(+), 49 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index 8e94ccd067..99a8b77714 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -13,7 +13,7 @@ return array(
     'core.pkg.js' => 'ab3502fe',
     'dark-console.pkg.js' => '187792c2',
     'differential.pkg.css' => 'ffb69e3d',
-    'differential.pkg.js' => '7747755e',
+    'differential.pkg.js' => 'fdec5f60',
     'diffusion.pkg.css' => '42c75c37',
     'diffusion.pkg.js' => '78c9885d',
     'maniphest.pkg.css' => '35995d6d',
@@ -385,7 +385,7 @@ return array(
     'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '0116d3e8',
     'rsrc/js/application/diff/DiffChangeset.js' => 'd7d3ba75',
     'rsrc/js/application/diff/DiffChangesetList.js' => 'cc2c5de5',
-    'rsrc/js/application/diff/DiffInline.js' => '34ccdeda',
+    'rsrc/js/application/diff/DiffInline.js' => '2a6fac17',
     'rsrc/js/application/diff/DiffInlineContentState.js' => '68e6339d',
     'rsrc/js/application/diff/DiffPathView.js' => '8207abf9',
     'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b',
@@ -788,7 +788,7 @@ return array(
     'phabricator-dashboard-css' => '5a205b9d',
     'phabricator-diff-changeset' => 'd7d3ba75',
     'phabricator-diff-changeset-list' => 'cc2c5de5',
-    'phabricator-diff-inline' => '34ccdeda',
+    'phabricator-diff-inline' => '2a6fac17',
     'phabricator-diff-inline-content-state' => '68e6339d',
     'phabricator-diff-path-view' => '8207abf9',
     'phabricator-diff-tree-view' => '5d83623b',
@@ -1166,6 +1166,10 @@ return array(
       'javelin-stratcom',
       'javelin-behavior',
     ),
+    '2a6fac17' => array(
+      'javelin-dom',
+      'phabricator-diff-inline-content-state',
+    ),
     '2a8b62d9' => array(
       'multirow-row-manager',
       'javelin-install',
@@ -1220,10 +1224,6 @@ return array(
       'javelin-stratcom',
       'javelin-workflow',
     ),
-    '34ccdeda' => array(
-      'javelin-dom',
-      'phabricator-diff-inline-content-state',
-    ),
     '34e2a838' => array(
       'aphront-typeahead-control-css',
       'phui-tag-view-css',
diff --git a/src/infrastructure/diff/PhabricatorInlineCommentController.php b/src/infrastructure/diff/PhabricatorInlineCommentController.php
index 2076c1964e..353a8d4430 100644
--- a/src/infrastructure/diff/PhabricatorInlineCommentController.php
+++ b/src/infrastructure/diff/PhabricatorInlineCommentController.php
@@ -248,10 +248,6 @@ abstract class PhabricatorInlineCommentController
         // to set the stored state back to "A".
         $this->updateCommentContentState($inline);
 
-        if ($inline->isVoidComment($viewer)) {
-          $inline->setIsDeleted(1);
-        }
-
         $this->saveComment($inline);
 
         return $this->buildEmptyResponse();
diff --git a/webroot/rsrc/js/application/diff/DiffInline.js b/webroot/rsrc/js/application/diff/DiffInline.js
index c17217adfe..0255b00ed9 100644
--- a/webroot/rsrc/js/application/diff/DiffInline.js
+++ b/webroot/rsrc/js/application/diff/DiffInline.js
@@ -829,9 +829,10 @@ JX.install('DiffInline', {
       if (this._shouldDeleteOnSave()) {
         this._setPreventUndo(true);
         this._applyDelete();
-      } else {
-        this._applySave();
+        return;
       }
+
+      this._applySave();
     },
 
     _shouldDeleteOnSave: function() {
@@ -843,6 +844,29 @@ JX.install('DiffInline', {
       return !state.getText().length;
     },
 
+    _shouldDeleteOnCancel: function() {
+      var state = this._getActiveContentState();
+
+      // TODO: This is greatly simplified, too.
+
+      return !state.getText().length;
+    },
+
+    _shouldUndoOnCancel: function() {
+      var state = this._getActiveContentState().getWireFormat();
+
+      // TODO: This is also simplified.
+
+      var is_empty = this._isVoidContentState(state);
+      var is_same = this._isSameContentState(state, this._originalState);
+
+      if (!is_empty && !is_same) {
+        return true;
+      }
+
+      return false;
+    },
+
     _applySave: function() {
       var handler = JX.bind(this, this._onsaveresponse);
 
@@ -860,6 +884,14 @@ JX.install('DiffInline', {
       this._applyCall(handler, data);
     },
 
+    _applyCancel: function(state) {
+      var handler = JX.bind(this, this._onCancelResponse);
+
+      var data = this._newRequestData('cancel', state);
+
+      this._applyCall(handler, data);
+    },
+
     _applyCall: function(handler, data) {
       var uri = this._getInlineURI();
 
@@ -903,57 +935,34 @@ JX.install('DiffInline', {
     },
 
     cancel: function() {
-      var state = this._getActiveContentState().getWireFormat();
-
       JX.DOM.remove(this._editRow);
       this._editRow = null;
 
-      var is_empty = this._isVoidContentState(state);
-      var is_same = this._isSameContentState(state, this._originalState);
-      if (!is_empty && !is_same) {
-        this._drawUneditRows(state);
+      if (this._shouldDeleteOnCancel()) {
+        this._setPreventUndo(true);
+        this._applyDelete();
+        return;
       }
 
-      // If this was an empty box and we typed some text and then hit cancel,
-      // don't show the empty concrete inline.
-      if (this._isVoidContentState(this._originalState)) {
-        this.setInvisible(true);
-      } else {
-        this.setInvisible(false);
+      if (this._shouldUndoOnCancel()) {
+        var state = this._getActiveContentState().getWireFormat();
+        this._drawUneditRows(state);
       }
 
       // If you "undo" to restore text ("AB") and then "Cancel", we put you
       // back in the original text state ("A"). We also send the original
       // text ("A") to the server as the current persistent state.
 
-      var uri = this._getInlineURI();
-      var data = this._newRequestData('cancel', this._originalState);
-      var handler = JX.bind(this, this._onCancelResponse);
+      this.setEditing(false);
+      this.setInvisible(false);
 
-      this.setLoading(true);
-
-      new JX.Request(uri, handler)
-        .setData(data)
-        .send();
+      this._applyCancel(this._originalState);
 
       this._didUpdate(true);
     },
 
     _onCancelResponse: function(response) {
-      this.setEditing(false);
-      this.setLoading(false);
-
-      // If the comment was empty when we started editing it (there's no
-      // original text) and empty when we finished editing it (there's no
-      // undo row), just delete the comment.
-      if (this._isVoidContentState(this._originalState) && !this.isUndo()) {
-        this.setDeleted(true);
-
-        JX.DOM.remove(this._row);
-        this._row = null;
-
-        this._didUpdate();
-      }
+      // Nothing to do.
     },
 
     _getSuggestionNode: function(row) {
@@ -970,9 +979,8 @@ JX.install('DiffInline', {
         this._editRow = null;
       }
 
-      this.setLoading(false);
-      this.setInvisible(false);
       this.setEditing(false);
+      this.setInvisible(false);
 
       var new_row = this._drawContentRows(JX.$H(response.view).getNode());
       JX.DOM.remove(this._row);

From 428fff2e58f35b7c3fed22ad3953f7c80f22c1e7 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 25 Mar 2021 10:31:51 -0700
Subject: [PATCH 09/14] Fix an issue when undoing mutiple inline comment
 deletions

Summary:
Ref T13559. If you create comments A and B, then delete comments A and B, then undo the deletion of A, the UI undoes the deletion of B instead.

This is becasue the undo rows are shipped down with a static scalar metadata reference. When copied multiple times to create multiple undo rows, they reference the same data object.

Preventing this in the general case is a problem with greater scope. For now, just avoid rendering these rows with any metadata so they don't alias a single data object.

Test Plan:
  - Created comments A, B.
  - Deleted comments A, B.
  - Clicked "Undo" on A.
    - Before: Deletion of "B" undone.
    - After: Deletion of "A" undone.

Maniphest Tasks: T13559

Differential Revision: https://secure.phabricator.com/D21650
---
 .../render/DifferentialChangesetRenderer.php  |  3 ++
 .../view/PHUIDiffInlineCommentRowScaffold.php | 37 ++++++++++++++++---
 .../view/PHUIDiffInlineCommentUndoView.php    |  2 +-
 3 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/src/applications/differential/render/DifferentialChangesetRenderer.php b/src/applications/differential/render/DifferentialChangesetRenderer.php
index dcae3b979b..711d7d574b 100644
--- a/src/applications/differential/render/DifferentialChangesetRenderer.php
+++ b/src/applications/differential/render/DifferentialChangesetRenderer.php
@@ -718,6 +718,9 @@ abstract class DifferentialChangesetRenderer extends Phobject {
 
     foreach ($views as $key => $view) {
       $scaffold = $this->getRowScaffoldForInline($view);
+
+      $scaffold->setIsUndoTemplate(true);
+
       $views[$key] = id(new PHUIDiffInlineCommentTableScaffold())
         ->addRowScaffold($scaffold);
     }
diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentRowScaffold.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentRowScaffold.php
index 7c6b1c54b5..9bb68d63ba 100644
--- a/src/infrastructure/diff/view/PHUIDiffInlineCommentRowScaffold.php
+++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentRowScaffold.php
@@ -10,6 +10,16 @@
 abstract class PHUIDiffInlineCommentRowScaffold extends AphrontView {
 
   private $views = array();
+  private $isUndoTemplate;
+
+  final public function setIsUndoTemplate($is_undo_template) {
+    $this->isUndoTemplate = $is_undo_template;
+    return $this;
+  }
+
+  final public function getIsUndoTemplate() {
+    return $this->isUndoTemplate;
+  }
 
   public function getInlineViews() {
     return $this->views;
@@ -21,11 +31,28 @@ abstract class PHUIDiffInlineCommentRowScaffold extends AphrontView {
   }
 
   protected function getRowAttributes() {
+    $is_undo_template = $this->getIsUndoTemplate();
+
     $is_hidden = false;
-    foreach ($this->getInlineViews() as $view) {
-      if ($view->isHidden()) {
-        $is_hidden = true;
+    if ($is_undo_template) {
+
+      // NOTE: When this scaffold is turned into an "undo" template, it is
+      // important it not have any metadata: the metadata reference will be
+      // copied to each instance of the row. This is a complicated mess; for
+      // now, just sneak by without generating metadata when rendering undo
+      // templates.
+
+      $metadata = null;
+    } else {
+      foreach ($this->getInlineViews() as $view) {
+        if ($view->isHidden()) {
+          $is_hidden = true;
+        }
       }
+
+      $metadata = array(
+        'hidden' => $is_hidden,
+      );
     }
 
     $classes = array();
@@ -37,9 +64,7 @@ abstract class PHUIDiffInlineCommentRowScaffold extends AphrontView {
     $result = array(
       'class' => implode(' ', $classes),
       'sigil' => 'inline-row',
-      'meta' => array(
-        'hidden' => $is_hidden,
-      ),
+      'meta' => $metadata,
     );
 
     return $result;
diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentUndoView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentUndoView.php
index 4abdb00e0b..a5fd7036d9 100644
--- a/src/infrastructure/diff/view/PHUIDiffInlineCommentUndoView.php
+++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentUndoView.php
@@ -27,7 +27,7 @@ final class PHUIDiffInlineCommentUndoView
       array(
         'class' => 'differential-inline-undo',
       ),
-      array(pht('Changes discarded. '), $link));
+      array(pht('Changes discarded.'), ' ', $link));
   }
 
 }

From 5efe7fb4c1812c949a48d3691ce446be637aa354 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 24 Mar 2021 10:11:02 -0700
Subject: [PATCH 10/14] On inline comments, track an explicit "committed"
 content state

Summary:
Ref T13559. To allow the client to make correct decisions about what buttons mean, track an explicit "Committed" content state.

This is the last version of the comment that has been saved on the server, and does not exist if the comment has never been saved.

Test Plan: Created comments, etc. See followups.

Maniphest Tasks: T13559

Differential Revision: https://secure.phabricator.com/D21651
---
 resources/celerity/map.php                    | 30 ++++++++---------
 .../rsrc/externals/javelin/lib/Resource.js    |  4 +++
 .../rsrc/js/application/diff/DiffInline.js    | 32 +++++++++++++------
 3 files changed, 42 insertions(+), 24 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index 99a8b77714..67887ef683 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -10,10 +10,10 @@ return array(
     'conpherence.pkg.css' => '0e3cf785',
     'conpherence.pkg.js' => '020aebcf',
     'core.pkg.css' => '0ae696de',
-    'core.pkg.js' => 'ab3502fe',
+    'core.pkg.js' => '68f29322',
     'dark-console.pkg.js' => '187792c2',
     'differential.pkg.css' => 'ffb69e3d',
-    'differential.pkg.js' => 'fdec5f60',
+    'differential.pkg.js' => 'bbf6d742',
     'diffusion.pkg.css' => '42c75c37',
     'diffusion.pkg.js' => '78c9885d',
     'maniphest.pkg.css' => '35995d6d',
@@ -253,7 +253,7 @@ return array(
     'rsrc/externals/javelin/lib/Mask.js' => '7c4d8998',
     'rsrc/externals/javelin/lib/Quicksand.js' => 'd3799cb4',
     'rsrc/externals/javelin/lib/Request.js' => '84e6891f',
-    'rsrc/externals/javelin/lib/Resource.js' => '740956e1',
+    'rsrc/externals/javelin/lib/Resource.js' => '20514cc2',
     'rsrc/externals/javelin/lib/Routable.js' => '6a18c42e',
     'rsrc/externals/javelin/lib/Router.js' => '32755edb',
     'rsrc/externals/javelin/lib/Scrollbar.js' => 'a43ae2ae',
@@ -385,7 +385,7 @@ return array(
     'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '0116d3e8',
     'rsrc/js/application/diff/DiffChangeset.js' => 'd7d3ba75',
     'rsrc/js/application/diff/DiffChangesetList.js' => 'cc2c5de5',
-    'rsrc/js/application/diff/DiffInline.js' => '2a6fac17',
+    'rsrc/js/application/diff/DiffInline.js' => 'c794b624',
     'rsrc/js/application/diff/DiffInlineContentState.js' => '68e6339d',
     'rsrc/js/application/diff/DiffPathView.js' => '8207abf9',
     'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b',
@@ -734,7 +734,7 @@ return array(
     'javelin-reactor-node-calmer' => '225bbb98',
     'javelin-reactornode' => '72960bc1',
     'javelin-request' => '84e6891f',
-    'javelin-resource' => '740956e1',
+    'javelin-resource' => '20514cc2',
     'javelin-routable' => '6a18c42e',
     'javelin-router' => '32755edb',
     'javelin-scrollbar' => 'a43ae2ae',
@@ -788,7 +788,7 @@ return array(
     'phabricator-dashboard-css' => '5a205b9d',
     'phabricator-diff-changeset' => 'd7d3ba75',
     'phabricator-diff-changeset-list' => 'cc2c5de5',
-    'phabricator-diff-inline' => '2a6fac17',
+    'phabricator-diff-inline' => 'c794b624',
     'phabricator-diff-inline-content-state' => '68e6339d',
     'phabricator-diff-path-view' => '8207abf9',
     'phabricator-diff-tree-view' => '5d83623b',
@@ -1106,6 +1106,11 @@ return array(
       'javelin-behavior',
       'javelin-request',
     ),
+    '20514cc2' => array(
+      'javelin-util',
+      'javelin-uri',
+      'javelin-install',
+    ),
     '225bbb98' => array(
       'javelin-install',
       'javelin-reactor',
@@ -1166,10 +1171,6 @@ return array(
       'javelin-stratcom',
       'javelin-behavior',
     ),
-    '2a6fac17' => array(
-      'javelin-dom',
-      'phabricator-diff-inline-content-state',
-    ),
     '2a8b62d9' => array(
       'multirow-row-manager',
       'javelin-install',
@@ -1597,11 +1598,6 @@ return array(
       'javelin-stratcom',
       'phabricator-tooltip',
     ),
-    '740956e1' => array(
-      'javelin-util',
-      'javelin-uri',
-      'javelin-install',
-    ),
     74446546 => array(
       'javelin-behavior',
       'javelin-dom',
@@ -2092,6 +2088,10 @@ return array(
       'javelin-workflow',
       'javelin-json',
     ),
+    'c794b624' => array(
+      'javelin-dom',
+      'phabricator-diff-inline-content-state',
+    ),
     'cc2c5de5' => array(
       'javelin-install',
       'phuix-button-view',
diff --git a/webroot/rsrc/externals/javelin/lib/Resource.js b/webroot/rsrc/externals/javelin/lib/Resource.js
index 4e7d528740..68219e0389 100644
--- a/webroot/rsrc/externals/javelin/lib/Resource.js
+++ b/webroot/rsrc/externals/javelin/lib/Resource.js
@@ -149,6 +149,10 @@ JX.install('Resource', {
         }
       }
 
+      for (var jj = 0; jj < errors.length; jj++) {
+        JX.log(errors[jj]);
+      }
+
       if (errors.length) {
         throw errors[0];
       }
diff --git a/webroot/rsrc/js/application/diff/DiffInline.js b/webroot/rsrc/js/application/diff/DiffInline.js
index 0255b00ed9..399f159864 100644
--- a/webroot/rsrc/js/application/diff/DiffInline.js
+++ b/webroot/rsrc/js/application/diff/DiffInline.js
@@ -9,6 +9,7 @@ JX.install('DiffInline', {
 
   construct : function() {
     this._activeContentState = new JX.DiffInlineContentState();
+    this._committedContentState = new JX.DiffInlineContentState();
   },
 
   members: {
@@ -21,7 +22,6 @@ JX.install('DiffInline', {
     _displaySide: null,
     _isNewFile: null,
     _replyToCommentPHID: null,
-    _originalState: null,
     _snippet: null,
     _menuItems: null,
     _documentEngineKey: null,
@@ -56,6 +56,7 @@ JX.install('DiffInline', {
     _isSelected: false,
     _canSuggestEdit: false,
 
+    _committedContentState: null,
     _activeContentState: null,
 
     bindToRow: function(row) {
@@ -336,8 +337,6 @@ JX.install('DiffInline', {
       this._phid = null;
       this._isCollapsed = false;
 
-      this._originalState = null;
-
       return row;
     },
 
@@ -602,7 +601,10 @@ JX.install('DiffInline', {
 
     _readInlineState: function(state) {
       this._id = state.id;
-      this._originalState = state.contentState;
+
+      // TODO: This is not the correct content state after a reload: it is
+      // the draft state.
+      this._getCommittedContentState().readWireFormat(state.contentState);
 
       this._getActiveContentState().readWireFormat(state.contentState);
 
@@ -673,7 +675,10 @@ JX.install('DiffInline', {
       this._editRow = this._drawRows(rows, null, 'edit');
 
       this._drawSuggestionState(this._editRow);
-      this.setHasSuggestion(this._originalState.hasSuggestion);
+
+      // TODO: We're just doing this for the rendering side effect of drawing
+      // the button text.
+      this.setHasSuggestion(this.getHasSuggestion());
     },
 
     _drawRows: function(rows, cursor, type) {
@@ -800,6 +805,10 @@ JX.install('DiffInline', {
       return state;
     },
 
+    _getCommittedContentState: function() {
+      return this._committedContentState;
+    },
+
     setHasSuggestion: function(has_suggestion) {
       var state = this._getActiveContentState();
       state.setHasSuggestion(has_suggestion);
@@ -853,12 +862,13 @@ JX.install('DiffInline', {
     },
 
     _shouldUndoOnCancel: function() {
-      var state = this._getActiveContentState().getWireFormat();
+      var new_state = this._getActiveContentState().getWireFormat();
+      var old_state = this._getCommittedContentState().getWireFormat();
 
       // TODO: This is also simplified.
 
-      var is_empty = this._isVoidContentState(state);
-      var is_same = this._isSameContentState(state, this._originalState);
+      var is_empty = this._isVoidContentState(new_state);
+      var is_same = this._isSameContentState(new_state, old_state);
 
       if (!is_empty && !is_same) {
         return true;
@@ -956,7 +966,8 @@ JX.install('DiffInline', {
       this.setEditing(false);
       this.setInvisible(false);
 
-      this._applyCancel(this._originalState);
+      var old_state = this._getCommittedContentState();
+      this._applyCancel(old_state.getWireFormat());
 
       this._didUpdate(true);
     },
@@ -1184,6 +1195,9 @@ JX.install('DiffInline', {
     },
 
     _isVoidContentState: function(state) {
+      if (!state.text) {
+        return true;
+      }
       return (!state.text.length && !state.suggestionText.length);
     },
 

From b75517918d86c88d5f388ee2f9d47f17759ed809 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 25 Mar 2021 10:55:56 -0700
Subject: [PATCH 11/14] When creating an inline comment, populate the content
 state with the default suggestion text

Summary:
Ref T13559. Currently, the default text for inline comment side-loads in a bizarre way. Instead, when a user creates an inline comment, load the inline context and set it as part of the initial content state.

This allows the side channel (and the code that puts the text in place at the last second on the client) to be removed.

Test Plan: Created inlines, clicked "Suggest Edit". See followup changes.

Maniphest Tasks: T13559

Differential Revision: https://secure.phabricator.com/D21652
---
 resources/celerity/map.php                      | 14 +++++++-------
 .../diff/PhabricatorInlineCommentController.php | 17 +++++++++++++++++
 .../diff/interface/PhabricatorInlineComment.php | 13 +++++++++++++
 .../diff/view/PHUIDiffInlineCommentEditView.php |  6 ------
 webroot/rsrc/js/application/diff/DiffInline.js  | 13 ++-----------
 5 files changed, 39 insertions(+), 24 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index 67887ef683..47f0dfd9ad 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -13,7 +13,7 @@ return array(
     'core.pkg.js' => '68f29322',
     'dark-console.pkg.js' => '187792c2',
     'differential.pkg.css' => 'ffb69e3d',
-    'differential.pkg.js' => 'bbf6d742',
+    'differential.pkg.js' => 'fbde899f',
     'diffusion.pkg.css' => '42c75c37',
     'diffusion.pkg.js' => '78c9885d',
     'maniphest.pkg.css' => '35995d6d',
@@ -385,7 +385,7 @@ return array(
     'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '0116d3e8',
     'rsrc/js/application/diff/DiffChangeset.js' => 'd7d3ba75',
     'rsrc/js/application/diff/DiffChangesetList.js' => 'cc2c5de5',
-    'rsrc/js/application/diff/DiffInline.js' => 'c794b624',
+    'rsrc/js/application/diff/DiffInline.js' => '62fff8eb',
     'rsrc/js/application/diff/DiffInlineContentState.js' => '68e6339d',
     'rsrc/js/application/diff/DiffPathView.js' => '8207abf9',
     'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b',
@@ -788,7 +788,7 @@ return array(
     'phabricator-dashboard-css' => '5a205b9d',
     'phabricator-diff-changeset' => 'd7d3ba75',
     'phabricator-diff-changeset-list' => 'cc2c5de5',
-    'phabricator-diff-inline' => 'c794b624',
+    'phabricator-diff-inline' => '62fff8eb',
     'phabricator-diff-inline-content-state' => '68e6339d',
     'phabricator-diff-path-view' => '8207abf9',
     'phabricator-diff-tree-view' => '5d83623b',
@@ -1532,6 +1532,10 @@ return array(
       'javelin-request',
       'javelin-uri',
     ),
+    '62fff8eb' => array(
+      'javelin-dom',
+      'phabricator-diff-inline-content-state',
+    ),
     '65bb0011' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -2088,10 +2092,6 @@ return array(
       'javelin-workflow',
       'javelin-json',
     ),
-    'c794b624' => array(
-      'javelin-dom',
-      'phabricator-diff-inline-content-state',
-    ),
     'cc2c5de5' => array(
       'javelin-install',
       'phuix-button-view',
diff --git a/src/infrastructure/diff/PhabricatorInlineCommentController.php b/src/infrastructure/diff/PhabricatorInlineCommentController.php
index 353a8d4430..4930502066 100644
--- a/src/infrastructure/diff/PhabricatorInlineCommentController.php
+++ b/src/infrastructure/diff/PhabricatorInlineCommentController.php
@@ -316,11 +316,28 @@ abstract class PhabricatorInlineCommentController
           $this->updateCommentContentState($inline);
         }
 
+        // NOTE: We're writing the comment as "deleted", then reloading to
+        // pick up context and undeleting it. This is silly -- we just want
+        // to load and attach context -- but just loading context is currently
+        // complicated (for example, context relies on cache keys that expect
+        // the inline to have an ID).
+
+        $inline->setIsDeleted(1);
+
         $this->saveComment($inline);
 
         // Reload the inline to attach context.
         $inline = $this->loadCommentByIDForEdit($inline->getID());
 
+        // Now, we can read the source file and set the initial state.
+        $state = $inline->getContentState();
+        $default_suggestion = $inline->getDefaultSuggestionText();
+        $state->setContentSuggestionText($default_suggestion);
+        $inline->setContentState($state);
+        $inline->setIsDeleted(0);
+
+        $this->saveComment($inline);
+
         $edit_dialog = $this->buildEditDialog($inline);
 
         if ($this->getOperation() == 'reply') {
diff --git a/src/infrastructure/diff/interface/PhabricatorInlineComment.php b/src/infrastructure/diff/interface/PhabricatorInlineComment.php
index 76976b5929..1ab3db30b9 100644
--- a/src/infrastructure/diff/interface/PhabricatorInlineComment.php
+++ b/src/infrastructure/diff/interface/PhabricatorInlineComment.php
@@ -364,6 +364,19 @@ abstract class PhabricatorInlineComment
     return $this->getStorageObject()->getInlineContext();
   }
 
+  public function getDefaultSuggestionText() {
+    $context = $this->getInlineContext();
+
+    if (!$context) {
+      return null;
+    }
+
+    $default = $context->getBodyLines();
+    $default = implode('', $default);
+
+    return $default;
+  }
+
 
 /* -(  PhabricatorMarkupInterface Implementation  )-------------------------- */
 
diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php
index 5c29c1666c..72e5adbe1f 100644
--- a/src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php
+++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php
@@ -114,9 +114,6 @@ final class PHUIDiffInlineCommentEditView
     $main = $state->getContentSuggestionText();
     $main_count = count(phutil_split_lines($main));
 
-    $default = $context->getBodyLines();
-    $default = implode('', $default);
-
     // Browsers ignore one leading newline in text areas. Add one so that
     // any actual leading newlines in the content are preserved.
     $main = "\n".$main;
@@ -127,9 +124,6 @@ final class PHUIDiffInlineCommentEditView
         'class' => 'inline-suggestion-input PhabricatorMonospaced',
         'rows' => max(3, $main_count + 1),
         'sigil' => 'inline-content-suggestion',
-        'meta' => array(
-          'defaultText' => $default,
-        ),
       ),
       $main);
 
diff --git a/webroot/rsrc/js/application/diff/DiffInline.js b/webroot/rsrc/js/application/diff/DiffInline.js
index 399f159864..dde96ea0a3 100644
--- a/webroot/rsrc/js/application/diff/DiffInline.js
+++ b/webroot/rsrc/js/application/diff/DiffInline.js
@@ -772,21 +772,12 @@ JX.install('DiffInline', {
 
       this.setHasSuggestion(!this.getHasSuggestion());
 
-      // The first time the user actually clicks the button and enables
-      // suggestions for a given editor state, fill the input with the
-      // underlying text if there isn't any text yet.
+      // Resize the suggestion input for size of the text.
       if (this.getHasSuggestion()) {
         if (this._editRow) {
           var node = this._getSuggestionNode(this._editRow);
           if (node) {
-            if (!node.value.length) {
-              var data = JX.Stratcom.getData(node);
-              if (!data.hasSetDefault) {
-                data.hasSetDefault = true;
-                node.value = data.defaultText;
-                node.rows = Math.max(3, node.value.split('\n').length);
-              }
-            }
+            node.rows = Math.max(3, node.value.split('\n').length);
           }
         }
       }

From 6fd55d692f14c9bf8d658b1a35ccf7c9216bb651 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 25 Mar 2021 11:24:36 -0700
Subject: [PATCH 12/14] Formally track "initial", "committed", and "active"
 states for inline comments

Summary:
Ref T13559. Various client decisions depend on the "initial" or "committed" states of inline comments. Previously, these were informally constructed from "mostly similar" available values, or glossed over in some cases.

On the server, save the initial state when creating a comment. Save the committed state when applying a "save" operation. Send all three states to the client.

On the client, load and track all three states explicitly.

Test Plan: Created inlines, etc. See followups.

Maniphest Tasks: T13559

Differential Revision: https://secure.phabricator.com/D21653
---
 resources/celerity/map.php                    |  14 +--
 .../PhabricatorInlineCommentController.php    | 101 +++++++++---------
 .../interface/PhabricatorInlineComment.php    |  90 ++++++++++++++--
 .../diff/view/PHUIDiffInlineCommentView.php   |   2 +-
 .../rsrc/js/application/diff/DiffInline.js    |  32 ++++--
 5 files changed, 163 insertions(+), 76 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index 47f0dfd9ad..af877e264a 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -13,7 +13,7 @@ return array(
     'core.pkg.js' => '68f29322',
     'dark-console.pkg.js' => '187792c2',
     'differential.pkg.css' => 'ffb69e3d',
-    'differential.pkg.js' => 'fbde899f',
+    'differential.pkg.js' => '59453886',
     'diffusion.pkg.css' => '42c75c37',
     'diffusion.pkg.js' => '78c9885d',
     'maniphest.pkg.css' => '35995d6d',
@@ -385,7 +385,7 @@ return array(
     'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '0116d3e8',
     'rsrc/js/application/diff/DiffChangeset.js' => 'd7d3ba75',
     'rsrc/js/application/diff/DiffChangesetList.js' => 'cc2c5de5',
-    'rsrc/js/application/diff/DiffInline.js' => '62fff8eb',
+    'rsrc/js/application/diff/DiffInline.js' => '26664c24',
     'rsrc/js/application/diff/DiffInlineContentState.js' => '68e6339d',
     'rsrc/js/application/diff/DiffPathView.js' => '8207abf9',
     'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b',
@@ -788,7 +788,7 @@ return array(
     'phabricator-dashboard-css' => '5a205b9d',
     'phabricator-diff-changeset' => 'd7d3ba75',
     'phabricator-diff-changeset-list' => 'cc2c5de5',
-    'phabricator-diff-inline' => '62fff8eb',
+    'phabricator-diff-inline' => '26664c24',
     'phabricator-diff-inline-content-state' => '68e6339d',
     'phabricator-diff-path-view' => '8207abf9',
     'phabricator-diff-tree-view' => '5d83623b',
@@ -1162,6 +1162,10 @@ return array(
       'javelin-json',
       'phabricator-prefab',
     ),
+    '26664c24' => array(
+      'javelin-dom',
+      'phabricator-diff-inline-content-state',
+    ),
     '289bf236' => array(
       'javelin-install',
       'javelin-util',
@@ -1532,10 +1536,6 @@ return array(
       'javelin-request',
       'javelin-uri',
     ),
-    '62fff8eb' => array(
-      'javelin-dom',
-      'phabricator-diff-inline-content-state',
-    ),
     '65bb0011' => array(
       'javelin-behavior',
       'javelin-dom',
diff --git a/src/infrastructure/diff/PhabricatorInlineCommentController.php b/src/infrastructure/diff/PhabricatorInlineCommentController.php
index 4930502066..0899c2af94 100644
--- a/src/infrastructure/diff/PhabricatorInlineCommentController.php
+++ b/src/infrastructure/diff/PhabricatorInlineCommentController.php
@@ -180,56 +180,60 @@ abstract class PhabricatorInlineCommentController
         $this->saveComment($inline);
 
         return $this->buildEmptyResponse();
-      case 'edit':
       case 'save':
         $inline = $this->loadCommentByIDForEdit($this->getCommentID());
-        if ($op === 'save') {
-          $this->updateCommentContentState($inline);
 
+        $this->updateCommentContentState($inline);
+
+        $inline
+          ->setIsEditing(false)
+          ->setIsDeleted(0);
+
+        // Since we're saving the comment, update the committed state.
+        $active_state = $inline->getContentState();
+        $inline->setCommittedContentState($active_state);
+
+        $this->saveComment($inline);
+
+        return $this->buildRenderedCommentResponse(
+          $inline,
+          $this->getIsOnRight());
+      case 'edit':
+        $inline = $this->loadCommentByIDForEdit($this->getCommentID());
+
+        // NOTE: At time of writing, the "editing" state of inlines is
+        // preserved by simulating a click on "Edit" when the inline loads.
+
+        // In this case, we don't want to "saveComment()", because it
+        // recalculates object drafts and purges versioned drafts.
+
+        // The recalculation is merely unnecessary (state doesn't change)
+        // but purging drafts means that loading a page and then closing it
+        // discards your drafts.
+
+        // To avoid the purge, only invoke "saveComment()" if we actually
+        // have changes to apply.
+
+        $is_dirty = false;
+        if (!$inline->getIsEditing()) {
           $inline
-            ->setIsEditing(false)
-            ->setIsDeleted(0);
+            ->setIsDeleted(0)
+            ->setIsEditing(true);
 
-          $this->saveComment($inline);
+          $is_dirty = true;
+        }
 
-          return $this->buildRenderedCommentResponse(
-            $inline,
-            $this->getIsOnRight());
+        if ($this->hasContentState()) {
+          $this->updateCommentContentState($inline);
+          $is_dirty = true;
         } else {
-          // NOTE: At time of writing, the "editing" state of inlines is
-          // preserved by simulating a click on "Edit" when the inline loads.
+          PhabricatorInlineComment::loadAndAttachVersionedDrafts(
+            $viewer,
+            array($inline));
+        }
 
-          // In this case, we don't want to "saveComment()", because it
-          // recalculates object drafts and purges versioned drafts.
-
-          // The recalculation is merely unnecessary (state doesn't change)
-          // but purging drafts means that loading a page and then closing it
-          // discards your drafts.
-
-          // To avoid the purge, only invoke "saveComment()" if we actually
-          // have changes to apply.
-
-          $is_dirty = false;
-          if (!$inline->getIsEditing()) {
-            $inline
-              ->setIsDeleted(0)
-              ->setIsEditing(true);
-
-            $is_dirty = true;
-          }
-
-          if ($this->hasContentState()) {
-            $this->updateCommentContentState($inline);
-            $is_dirty = true;
-          } else {
-            PhabricatorInlineComment::loadAndAttachVersionedDrafts(
-              $viewer,
-              array($inline));
-          }
-
-          if ($is_dirty) {
-            $this->saveComment($inline);
-          }
+        if ($is_dirty) {
+          $this->saveComment($inline);
         }
 
         $edit_dialog = $this->buildEditDialog($inline)
@@ -333,7 +337,10 @@ abstract class PhabricatorInlineCommentController
         $state = $inline->getContentState();
         $default_suggestion = $inline->getDefaultSuggestionText();
         $state->setContentSuggestionText($default_suggestion);
+
+        $inline->setInitialContentState($state);
         $inline->setContentState($state);
+
         $inline->setIsDeleted(0);
 
         $this->saveComment($inline);
@@ -461,6 +468,7 @@ abstract class PhabricatorInlineCommentController
     PhabricatorInlineComment $inline,
     $view,
     $is_edit) {
+    $viewer = $this->getViewer();
 
     if ($inline->getReplyToCommentPHID()) {
       $can_suggest = false;
@@ -469,18 +477,15 @@ abstract class PhabricatorInlineCommentController
     }
 
     if ($is_edit) {
-      $viewer = $this->getViewer();
-      $content_state = $inline->getContentStateForEdit($viewer);
+      $state = $inline->getContentStateMapForEdit($viewer);
     } else {
-      $content_state = $inline->getContentState();
+      $state = $inline->getContentStateMap();
     }
 
-    $state_map = $content_state->newStorageMap();
-
     $response = array(
       'inline' => array(
         'id' => $inline->getID(),
-        'contentState' => $state_map,
+        'state' => $state,
         'canSuggestEdit' => $can_suggest,
       ),
       'view' => hsprintf('%s', $view),
diff --git a/src/infrastructure/diff/interface/PhabricatorInlineComment.php b/src/infrastructure/diff/interface/PhabricatorInlineComment.php
index 1ab3db30b9..613dfb08aa 100644
--- a/src/infrastructure/diff/interface/PhabricatorInlineComment.php
+++ b/src/infrastructure/diff/interface/PhabricatorInlineComment.php
@@ -336,13 +336,29 @@ abstract class PhabricatorInlineComment
     return $this->newContentState()->readFromRequest($request);
   }
 
-  public function getContentState() {
-    $state = $this->newContentState();
+  public function getInitialContentState() {
+    return $this->getNamedContentState('inline.state.initial');
+  }
 
-    $storage = $this->getStorageObject();
-    $storage_map = $storage->getAttribute('inline.state');
-    if (is_array($storage_map)) {
-      $state->readStorageMap($storage_map);
+  public function setInitialContentState(
+    PhabricatorInlineCommentContentState $state) {
+    return $this->setNamedContentState('inline.state.initial', $state);
+  }
+
+  public function getCommittedContentState() {
+    return $this->getNamedContentState('inline.state.committed');
+  }
+
+  public function setCommittedContentState(
+    PhabricatorInlineCommentContentState $state) {
+    return $this->setNamedContentState('inline.state.committed', $state);
+  }
+
+  public function getContentState() {
+    $state = $this->getNamedContentState('inline.state');
+
+    if (!$state) {
+      $state = $this->newContentState();
     }
 
     $state->setContentText($this->getContent());
@@ -351,11 +367,31 @@ abstract class PhabricatorInlineComment
   }
 
   public function setContentState(PhabricatorInlineCommentContentState $state) {
+    $this->setContent($state->getContentText());
+
+    return $this->setNamedContentState('inline.state', $state);
+  }
+
+  private function getNamedContentState($key) {
+    $storage = $this->getStorageObject();
+
+    $storage_map = $storage->getAttribute($key);
+    if (!is_array($storage_map)) {
+      return null;
+    }
+
+    $state = $this->newContentState();
+    $state->readStorageMap($storage_map);
+    return $state;
+  }
+
+  private function setNamedContentState(
+    $key,
+    PhabricatorInlineCommentContentState $state) {
+
     $storage = $this->getStorageObject();
     $storage_map = $state->newStorageMap();
-    $storage->setAttribute('inline.state', $storage_map);
-
-    $this->setContent($state->getContentText());
+    $storage->setAttribute($key, $storage_map);
 
     return $this;
   }
@@ -364,6 +400,42 @@ abstract class PhabricatorInlineComment
     return $this->getStorageObject()->getInlineContext();
   }
 
+  public function getContentStateMapForEdit(PhabricatorUser $viewer) {
+    return $this->getWireContentStateMap(true, $viewer);
+  }
+
+  public function getContentStateMap() {
+    return $this->getWireContentStateMap(false, null);
+  }
+
+  private function getWireContentStateMap(
+    $is_edit,
+    PhabricatorUser $viewer = null) {
+
+    $initial_state = $this->getInitialContentState();
+    $committed_state = $this->getCommittedContentState();
+
+    if ($is_edit) {
+      $active_state = $this->getContentStateForEdit($viewer);
+    } else {
+      $active_state = $this->getContentState();
+    }
+
+    return array(
+      'initial' => $this->getWireContentState($initial_state),
+      'committed' => $this->getWireContentState($committed_state),
+      'active' => $this->getWireContentState($active_state),
+    );
+  }
+
+  private function getWireContentState($content_state) {
+    if ($content_state === null) {
+      return null;
+    }
+
+    return $content_state->newStorageMap();
+  }
+
   public function getDefaultSuggestionText() {
     $context = $this->getInlineContext();
 
diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php
index 11be8d0ec1..448a7f2f50 100644
--- a/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php
+++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php
@@ -93,7 +93,7 @@ abstract class PHUIDiffInlineCommentView extends AphrontView {
       'startOffset' => $inline->getStartOffset(),
       'endOffset' => $inline->getEndOffset(),
       'on_right' => $this->getIsOnRight(),
-      'contentState' => $inline->getContentState()->newStorageMap(),
+      'state' => $inline->getContentStateMap(),
     );
   }
 
diff --git a/webroot/rsrc/js/application/diff/DiffInline.js b/webroot/rsrc/js/application/diff/DiffInline.js
index dde96ea0a3..6c54217403 100644
--- a/webroot/rsrc/js/application/diff/DiffInline.js
+++ b/webroot/rsrc/js/application/diff/DiffInline.js
@@ -8,8 +8,7 @@
 JX.install('DiffInline', {
 
   construct : function() {
-    this._activeContentState = new JX.DiffInlineContentState();
-    this._committedContentState = new JX.DiffInlineContentState();
+    this._state = {};
   },
 
   members: {
@@ -56,8 +55,7 @@ JX.install('DiffInline', {
     _isSelected: false,
     _canSuggestEdit: false,
 
-    _committedContentState: null,
-    _activeContentState: null,
+    _state: null,
 
     bindToRow: function(row) {
       this._row = row;
@@ -602,15 +600,23 @@ JX.install('DiffInline', {
     _readInlineState: function(state) {
       this._id = state.id;
 
-      // TODO: This is not the correct content state after a reload: it is
-      // the draft state.
-      this._getCommittedContentState().readWireFormat(state.contentState);
-
-      this._getActiveContentState().readWireFormat(state.contentState);
+      this._state = {
+        initial: this._newContentStateFromWireFormat(state.state.initial),
+        committed: this._newContentStateFromWireFormat(state.state.committed),
+        active: this._newContentStateFromWireFormat(state.state.active)
+      };
 
       this._canSuggestEdit = state.canSuggestEdit;
     },
 
+    _newContentStateFromWireFormat: function(map) {
+      if (map === null) {
+        return null;
+      }
+
+      return new JX.DiffInlineContentState().readWireFormat(map);
+    },
+
     _ondeleteresponse: function() {
       // If there's an existing "unedit" undo element, remove it.
       if (this._undoRow) {
@@ -787,7 +793,7 @@ JX.install('DiffInline', {
     },
 
     _getActiveContentState: function() {
-      var state = this._activeContentState;
+      var state = this._state.active;
 
       if (this._editRow) {
         state.readForm(this._editRow);
@@ -797,7 +803,11 @@ JX.install('DiffInline', {
     },
 
     _getCommittedContentState: function() {
-      return this._committedContentState;
+      return this._state.committed;
+    },
+
+    _getInitialContentState: function() {
+      return this._state.initial;
     },
 
     setHasSuggestion: function(has_suggestion) {

From 1308a5555fee7b5d32fb12797e9102534cff40ce Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 25 Mar 2021 13:28:04 -0700
Subject: [PATCH 13/14] Update client logic for inline comment "Save" and
 "Cancel" actions

Summary: Ref T13559. Substantially correct the client logic for "Save" and "Cancel" actions to handle unusual cases.

Test Plan:
Quoting behavior:

  - Quoted a comment.
  - Cancelled the quoted comment without modifying anything.
  - Reloaded page.
    - Before changes: quoted comment still exists.
    - After changes: quoted comment is deleted.
  - Looked at comment count in header, saw consistent behavior (before: weird behavior).

Empty suggestion behavior:

  - Created a new comment on a suggestable file.
  - Clicked "Suggest Edit" to enable suggestions.
  - Without making any text or suggestion changes, clicked "Save".
    - Before changes: comment saves, but is empty.
    - After changes: comment deletes itself without undo.

General behavior:

  - Created and saved an empty comment (deletes itself).
  - Created and saved a nonempty comment (saves as draft).
  - Created and saved an empty comment with an edit suggestion (saves).
  - Created and saved an empty comment with a suggestion to entirely delete lines -- that is, no suggestion text (saves).
  - Edited a comment, saved without changes (save).
  - Edited a comment, save deleting all text (saves -- note that this is intentionally without undo, since this is a lot of steps to do by accident).
  - Cancel editing an unchanged comment (cancels without undo).
  - Cancel editing a changed comment (cancels with undo).
    - Undo'd, got text back.
  - Cancel new comment with no text (deletes without undo).
  - Cancel new comment with text (deletes with undo).
    - Undo'd, got text back.
  - Saved a quoted comment with no changes (saves -- note that this is intentionally not a "delete", since just quoting someone seems fine if you click "Save" -- maybe you want to come back to it later).

Maniphest Tasks: T13559

Differential Revision: https://secure.phabricator.com/D21654
---
 resources/celerity/map.php                    |  24 +--
 .../PhabricatorInlineCommentController.php    |   4 +-
 .../rsrc/js/application/diff/DiffInline.js    | 189 +++++++++---------
 .../diff/DiffInlineContentState.js            |  66 ++++++
 4 files changed, 179 insertions(+), 104 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index af877e264a..309e698272 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -13,7 +13,7 @@ return array(
     'core.pkg.js' => '68f29322',
     'dark-console.pkg.js' => '187792c2',
     'differential.pkg.css' => 'ffb69e3d',
-    'differential.pkg.js' => '59453886',
+    'differential.pkg.js' => '8deec4cd',
     'diffusion.pkg.css' => '42c75c37',
     'diffusion.pkg.js' => '78c9885d',
     'maniphest.pkg.css' => '35995d6d',
@@ -385,8 +385,8 @@ return array(
     'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '0116d3e8',
     'rsrc/js/application/diff/DiffChangeset.js' => 'd7d3ba75',
     'rsrc/js/application/diff/DiffChangesetList.js' => 'cc2c5de5',
-    'rsrc/js/application/diff/DiffInline.js' => '26664c24',
-    'rsrc/js/application/diff/DiffInlineContentState.js' => '68e6339d',
+    'rsrc/js/application/diff/DiffInline.js' => '9c775532',
+    'rsrc/js/application/diff/DiffInlineContentState.js' => 'aa51efb4',
     'rsrc/js/application/diff/DiffPathView.js' => '8207abf9',
     'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b',
     'rsrc/js/application/differential/behavior-diff-radios.js' => '925fe8cd',
@@ -788,8 +788,8 @@ return array(
     'phabricator-dashboard-css' => '5a205b9d',
     'phabricator-diff-changeset' => 'd7d3ba75',
     'phabricator-diff-changeset-list' => 'cc2c5de5',
-    'phabricator-diff-inline' => '26664c24',
-    'phabricator-diff-inline-content-state' => '68e6339d',
+    'phabricator-diff-inline' => '9c775532',
+    'phabricator-diff-inline-content-state' => 'aa51efb4',
     'phabricator-diff-path-view' => '8207abf9',
     'phabricator-diff-tree-view' => '5d83623b',
     'phabricator-drag-and-drop-file-upload' => '4370900d',
@@ -1162,10 +1162,6 @@ return array(
       'javelin-json',
       'phabricator-prefab',
     ),
-    '26664c24' => array(
-      'javelin-dom',
-      'phabricator-diff-inline-content-state',
-    ),
     '289bf236' => array(
       'javelin-install',
       'javelin-util',
@@ -1549,9 +1545,6 @@ return array(
       'javelin-install',
       'javelin-dom',
     ),
-    '68e6339d' => array(
-      'javelin-dom',
-    ),
     '6a1583a8' => array(
       'javelin-behavior',
       'javelin-history',
@@ -1823,6 +1816,10 @@ return array(
       'javelin-dom',
       'javelin-workflow',
     ),
+    '9c775532' => array(
+      'javelin-dom',
+      'phabricator-diff-inline-content-state',
+    ),
     '9cec214e' => array(
       'javelin-behavior',
       'javelin-stratcom',
@@ -1913,6 +1910,9 @@ return array(
       'javelin-typeahead-ondemand-source',
       'javelin-dom',
     ),
+    'aa51efb4' => array(
+      'javelin-dom',
+    ),
     'aa6d2308' => array(
       'javelin-behavior',
       'javelin-dom',
diff --git a/src/infrastructure/diff/PhabricatorInlineCommentController.php b/src/infrastructure/diff/PhabricatorInlineCommentController.php
index 0899c2af94..6f1d3b66c0 100644
--- a/src/infrastructure/diff/PhabricatorInlineCommentController.php
+++ b/src/infrastructure/diff/PhabricatorInlineCommentController.php
@@ -172,7 +172,9 @@ abstract class PhabricatorInlineCommentController
         $inline = $this->loadCommentByIDForEdit($this->getCommentID());
 
         if ($is_delete) {
-          $inline->setIsDeleted(1);
+          $inline
+            ->setIsEditing(false)
+            ->setIsDeleted(1);
         } else {
           $inline->setIsDeleted(0);
         }
diff --git a/webroot/rsrc/js/application/diff/DiffInline.js b/webroot/rsrc/js/application/diff/DiffInline.js
index 6c54217403..4a0db5b520 100644
--- a/webroot/rsrc/js/application/diff/DiffInline.js
+++ b/webroot/rsrc/js/application/diff/DiffInline.js
@@ -44,7 +44,6 @@ JX.install('DiffInline', {
     _undoRow: null,
     _undoType: null,
     _undoState: null,
-    _preventUndo: false,
 
     _draftRequest: null,
     _skipFocus: false,
@@ -161,14 +160,6 @@ JX.install('DiffInline', {
       return this._endOffset;
     },
 
-    _setPreventUndo: function(prevent_undo) {
-      this._preventUndo = prevent_undo;
-    },
-
-    _getPreventUndo: function() {
-      return this._preventUndo;
-    },
-
     setIsSelected: function(is_selected) {
       this._isSelected = is_selected;
 
@@ -452,21 +443,12 @@ JX.install('DiffInline', {
         this._undoText = null;
       }
 
-      var uri = this._getInlineURI();
-      var handler = JX.bind(this, this._oneditresponse);
-
-      var data = this._newRequestData('edit', content_state);
-
-      this.setLoading(true);
-
-      new JX.Request(uri, handler)
-        .setData(data)
-        .send();
+      this._applyEdit(content_state);
     },
 
     delete: function(is_ref) {
       var uri = this._getInlineURI();
-      var handler = JX.bind(this, this._ondeleteresponse);
+      var handler = JX.bind(this, this._ondeleteresponse, false);
 
       // NOTE: This may be a direct delete (the user clicked on the inline
       // itself) or a "refdelete" (the user clicked somewhere else, like the
@@ -586,7 +568,6 @@ JX.install('DiffInline', {
       this._readInlineState(response.inline);
       this._drawEditRows(rows);
 
-      this.setLoading(false);
       this.setInvisible(true);
     },
 
@@ -617,24 +598,24 @@ JX.install('DiffInline', {
       return new JX.DiffInlineContentState().readWireFormat(map);
     },
 
-    _ondeleteresponse: function() {
-      // If there's an existing "unedit" undo element, remove it.
-      if (this._undoRow) {
-        JX.DOM.remove(this._undoRow);
-        this._undoRow = null;
-      }
+    _ondeleteresponse: function(prevent_undo) {
+      if (!prevent_undo) {
+        // If there's an existing "unedit" undo element, remove it.
+        if (this._undoRow) {
+          JX.DOM.remove(this._undoRow);
+          this._undoRow = null;
+        }
 
-      // If there's an existing editor, remove it. This happens when you
-      // delete a comment from the comment preview area. In this case, we
-      // read and preserve the text so "Undo" restores it.
-      var state = null;
-      if (this._editRow) {
-        state = this._getActiveContentState().getWireFormat();
-        JX.DOM.remove(this._editRow);
-        this._editRow = null;
-      }
+        // If there's an existing editor, remove it. This happens when you
+        // delete a comment from the comment preview area. In this case, we
+        // read and preserve the text so "Undo" restores it.
+        var state = null;
+        if (this._editRow) {
+          state = this._getActiveContentState().getWireFormat();
+          JX.DOM.remove(this._editRow);
+          this._editRow = null;
+        }
 
-      if (!this._getPreventUndo()) {
         this._drawUndeleteRows(state);
       }
 
@@ -693,7 +674,6 @@ JX.install('DiffInline', {
       var anchor = cursor || this._row;
       cursor = cursor || this._row.nextSibling;
 
-
       var result_row;
       var next_row;
       while (row) {
@@ -837,8 +817,10 @@ JX.install('DiffInline', {
 
     save: function() {
       if (this._shouldDeleteOnSave()) {
-        this._setPreventUndo(true);
-        this._applyDelete();
+        JX.DOM.remove(this._editRow);
+        this._editRow = null;
+
+        this._applyDelete(true);
         return;
       }
 
@@ -846,35 +828,56 @@ JX.install('DiffInline', {
     },
 
     _shouldDeleteOnSave: function() {
-      var state = this._getActiveContentState();
+      var active = this._getActiveContentState();
+      var initial = this._getInitialContentState();
 
-      // TODO: This is greatly simplified because we don't track all the
-      // state we need yet.
+      // When a user clicks "Save", it counts as a "delete" if the content
+      // of the comment is functionally empty.
 
-      return !state.getText().length;
-    },
+      // This isn't a delete if there's any text. Even if the text is a
+      // quote (so the state is the same as the initial state), we preserve
+      // it when the user clicks "Save".
+      if (!active.isTextEmpty()) {
+        return false;
+      }
 
-    _shouldDeleteOnCancel: function() {
-      var state = this._getActiveContentState();
+      // This isn't a delete if there's a suggestion and that suggestion is
+      // different from the initial state. (This means that an inline which
+      // purely suggests a block of code should be deleted is non-empty.)
+      if (active.getHasSuggestion()) {
+        if (!active.isSuggestionSimilar(initial)) {
+          return false;
+        }
+      }
 
-      // TODO: This is greatly simplified, too.
-
-      return !state.getText().length;
+      // Otherwise, this comment is functionally empty, so we can just treat
+      // a "Save" as a "delete".
+      return true;
     },
 
     _shouldUndoOnCancel: function() {
-      var new_state = this._getActiveContentState().getWireFormat();
-      var old_state = this._getCommittedContentState().getWireFormat();
+      var committed = this._getCommittedContentState();
+      var active = this._getActiveContentState();
+      var initial = this._getInitialContentState();
 
-      // TODO: This is also simplified.
+      // When a user clicks "Cancel", we only offer to let them "Undo" the
+      // action if the undo would be substantive.
 
-      var is_empty = this._isVoidContentState(new_state);
-      var is_same = this._isSameContentState(new_state, old_state);
-
-      if (!is_empty && !is_same) {
+      // The undo is substantive if the text is nonempty, and not similar to
+      // the last state.
+      var versus = committed || initial;
+      if (!active.isTextEmpty() && !active.isTextSimilar(versus)) {
         return true;
       }
 
+      // The undo is substantive if there's a suggestion, and the suggestion
+      // is not similar to the last state.
+      if (active.getHasSuggestion()) {
+        if (!active.isSuggestionSimilar(versus)) {
+          return true;
+        }
+      }
+
       return false;
     },
 
@@ -887,8 +890,8 @@ JX.install('DiffInline', {
       this._applyCall(handler, data);
     },
 
-    _applyDelete: function() {
-      var handler = JX.bind(this, this._ondeleteresponse);
+    _applyDelete: function(prevent_undo) {
+      var handler = JX.bind(this, this._ondeleteresponse, prevent_undo);
 
       var data = this._newRequestData('delete');
 
@@ -903,6 +906,14 @@ JX.install('DiffInline', {
       this._applyCall(handler, data);
     },
 
+    _applyEdit: function(state) {
+      var handler = JX.bind(this, this._oneditresponse);
+
+      var data = this._newRequestData('edit', state);
+
+      this._applyCall(handler, data);
+    },
+
     _applyCall: function(handler, data) {
       var uri = this._getInlineURI();
 
@@ -946,31 +957,41 @@ JX.install('DiffInline', {
     },
 
     cancel: function() {
+      // NOTE: Read the state before we remove the editor. Otherwise, we might
+      // miss text the user has entered into the textarea.
+      var state = this._getActiveContentState().getWireFormat();
+
       JX.DOM.remove(this._editRow);
       this._editRow = null;
 
-      if (this._shouldDeleteOnCancel()) {
-        this._setPreventUndo(true);
-        this._applyDelete();
-        return;
-      }
+      // When a user clicks "Cancel", we delete the comment if it has never
+      // been saved: we don't have a non-empty display state to revert to.
+      var is_delete = (this._getCommittedContentState() === null);
 
-      if (this._shouldUndoOnCancel()) {
-        var state = this._getActiveContentState().getWireFormat();
-        this._drawUneditRows(state);
-      }
+      var is_undo = this._shouldUndoOnCancel();
 
       // If you "undo" to restore text ("AB") and then "Cancel", we put you
       // back in the original text state ("A"). We also send the original
       // text ("A") to the server as the current persistent state.
 
-      this.setEditing(false);
-      this.setInvisible(false);
+      if (is_undo) {
+        this._drawUneditRows(state);
+      }
 
-      var old_state = this._getCommittedContentState();
-      this._applyCancel(old_state.getWireFormat());
+      if (is_delete) {
+        // NOTE: We're always suppressing the undo from "delete". We want to
+        // use the "undo" we just added above instead, which will get us
+        // back to the ephemeral, client-side editor state.
+        this._applyDelete(true);
+      } else {
+        this.setEditing(false);
+        this.setInvisible(false);
 
-      this._didUpdate(true);
+        var old_state = this._getCommittedContentState();
+        this._applyCancel(old_state.getWireFormat());
+
+        this._didUpdate(true);
+      }
     },
 
     _onCancelResponse: function(response) {
@@ -1067,8 +1088,8 @@ JX.install('DiffInline', {
         return null;
       }
 
-      var state = this._getActiveContentState().getWireFormat();
-      if (this._isVoidContentState(state)) {
+      var state = this._getActiveContentState();
+      if (state.isStateEmpty()) {
         return null;
       }
 
@@ -1077,7 +1098,7 @@ JX.install('DiffInline', {
         id: this.getID(),
       };
 
-      JX.copy(draft_data, state);
+      JX.copy(draft_data, state.getWireFormat());
 
       return draft_data;
     },
@@ -1193,22 +1214,8 @@ JX.install('DiffInline', {
         suggestionText: '',
         hasSuggestion: false
       };
-    },
-
-    _isVoidContentState: function(state) {
-      if (!state.text) {
-        return true;
-      }
-      return (!state.text.length && !state.suggestionText.length);
-    },
-
-    _isSameContentState: function(u, v) {
-      return (
-        ((u === null) === (v === null)) &&
-        (u.text === v.text) &&
-        (u.suggestionText === v.suggestionText) &&
-        (u.hasSuggestion === v.hasSuggestion));
     }
+
   }
 
 });
diff --git a/webroot/rsrc/js/application/diff/DiffInlineContentState.js b/webroot/rsrc/js/application/diff/DiffInlineContentState.js
index b1668f962c..0ac8628e74 100644
--- a/webroot/rsrc/js/application/diff/DiffInlineContentState.js
+++ b/webroot/rsrc/js/application/diff/DiffInlineContentState.js
@@ -59,6 +59,72 @@ JX.install('DiffInlineContentState', {
       return text;
     },
 
+    isStateEmpty: function() {
+      return (this.isTextEmpty() && this.isSuggestionEmpty());
+    },
+
+    isTextEmpty: function() {
+      var text = this.getText();
+      if (text === null) {
+        return true;
+      }
+
+      if (this._isStringSimilar(text, '')) {
+        return true;
+      }
+
+      return false;
+    },
+
+    isSuggestionEmpty: function() {
+      if (!this.getHasSuggestion()) {
+        return true;
+      }
+
+      var suggestion = this.getSuggestionText();
+      if (suggestion === null) {
+        return true;
+      }
+
+      if (this._isStringSimilar(suggestion, '')) {
+        return true;
+      }
+
+      return false;
+    },
+
+    isTextSimilar: function(v) {
+      if (!v) {
+        return false;
+      }
+
+      var us = this.getText();
+      var vs = v.getText();
+
+      return this._isStringSimilar(us, vs);
+    },
+
+    isSuggestionSimilar: function(v) {
+      // If we don't have a comparison state, treat them as dissimilar. This
+      // is expected to occur in old inline comments that did not save an
+      // initial state.
+
+      if (!v) {
+        return false;
+      }
+
+      var us = this.getSuggestionText();
+      var vs = v.getSuggestionText();
+
+      return this._isStringSimilar(us, vs);
+    },
+
+    _isStringSimilar: function(u, v) {
+      u = u || '';
+      v = v || '';
+      return (u === v);
+    },
+
     _getSuggestionNode: function(row) {
       try {
         return JX.DOM.find(row, 'textarea', 'inline-content-suggestion');

From 95662ae8f1a76eca66b024a11a21f5c5e8a7e01e Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 7 Apr 2021 14:49:03 -0700
Subject: [PATCH 14/14] Don't attempt to test capabilities on incomplete
 handles

Summary:
As backstory: I accidentally added the subscriber `PHID-USER-abcd` to `T1` on this install by calling `maniphest.edit`. I intended to edit `T1` on my local install.

This edit is permitted for messy technical reasons, described in T13429. It's not valid, but it's hard to prevent.

The state we reach is also possible even if the edit is rejected (i.e., someone can go manually update the database).

Regardless of how we get into this state, the state (a non-user subscriber) breaks the UI on the task page when it attempts to test if the subscriber can see the task.

To prevent this, only claim that a Handle can have capabilities if the handle is complete. If the handle is incomplete (an invalid or restricted object), it either can't be meaningfully tested for capabilities or the viewer isn't allowed to know them.

Test Plan:
Viewed `T1` on this install, saw a fatal. Applied the same edit to `T1` locally, got the same fatal. Applied patch, no more fatal. Now saw "Unknown Object (User)" in subscriber curtain.

Specifically, the fatal is:

> Attempting to test capability "view" for handle of type "USER", but this capability has not been attached.

Differential Revision: https://secure.phabricator.com/D21662
---
 src/applications/phid/PhabricatorObjectHandle.php | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/applications/phid/PhabricatorObjectHandle.php b/src/applications/phid/PhabricatorObjectHandle.php
index 95c40ffa2b..577fd05692 100644
--- a/src/applications/phid/PhabricatorObjectHandle.php
+++ b/src/applications/phid/PhabricatorObjectHandle.php
@@ -402,6 +402,10 @@ final class PhabricatorObjectHandle
   }
 
   public function hasCapabilities() {
+    if (!$this->isComplete()) {
+      return false;
+    }
+
     return ($this->getType() === PhabricatorPeopleUserPHIDType::TYPECONST);
   }