From 87c6c270b43599988fe7e2bb3563475d89f4b516 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 18 Mar 2021 11:29:59 -0700 Subject: [PATCH] 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;