From ab2aa74d6e29b46981d3021350bd73a49c23734d Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 18 Apr 2017 11:14:37 -0700 Subject: [PATCH] Fix several duplication/replay behaviors in Aphlict Summary: Ref T12566. Ref T12563. This fixes three bugs with Aphlict replay stuff: First, Conphernece would try to repaint the UI even if no thread was open. Only repaint when a thread is open. Second, although we deduplicate JX.Leader messages, we didn't deduplicate actual notification messages. If you browsed the leader window, then it re-elected itelf as a leader and replayed history, it could rebroadcast notifications and other windows could show doubles. Deduplicate notifications to prevent this. Third, we always replayed the last 60 seconds of history. When you browsed the leader window, whichever window became the new leader (possibly the one you just browsed) could replay messages from before it had opened, leading to duplicate messages. Particularly, after receiving a message and then browsing you could see that message again. Instead, only replay history as far back as when the window first opened. Test Plan: - Clicked "Repaint" with a thread open, saw a repaint. Clicked "Repaint" with Conpherence open but no thread, no repaint and no 404 request to `/update/null/`. - In browser A, opened three windows. In browser B, sent a notification. In browser A, browsed the leader window away twice in a row. Observed that the window which never became a leader doesn't duplicate notifications. - In browser A, opened three windows. In browser B, sent a notification. In browser A, browsed the leader window away over and over again. Observed that replay requests issued with appropriate history windows. Reviewers: chad Reviewed By: chad Maniphest Tasks: T12566, T12563 Differential Revision: https://secure.phabricator.com/D17722 --- resources/celerity/map.php | 78 +++++++++---------- ...icatorNotificationIndividualController.php | 1 + .../rsrc/js/application/aphlict/Aphlict.js | 18 ++++- .../aphlict/behavior-aphlict-listen.js | 10 ++- .../conpherence/ConpherenceThreadManager.js | 4 + 5 files changed, 67 insertions(+), 44 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index c4885db350..02db832ff4 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -10,7 +10,7 @@ return array( 'conpherence.pkg.css' => 'a34d59bd', 'conpherence.pkg.js' => '5f86c17d', 'core.pkg.css' => '959330a2', - 'core.pkg.js' => '5363ae35', + 'core.pkg.js' => '47a69358', 'darkconsole.pkg.js' => '1f9a31bc', 'differential.pkg.css' => '90b30783', 'differential.pkg.js' => 'ddfeb49b', @@ -362,16 +362,16 @@ return array( 'rsrc/image/texture/table_header.png' => '5c433037', 'rsrc/image/texture/table_header_hover.png' => '038ec3b9', 'rsrc/image/texture/table_header_tall.png' => 'd56b434f', - 'rsrc/js/application/aphlict/Aphlict.js' => '674e335f', + 'rsrc/js/application/aphlict/Aphlict.js' => 'e1d4b11a', 'rsrc/js/application/aphlict/behavior-aphlict-dropdown.js' => 'caade6f2', - 'rsrc/js/application/aphlict/behavior-aphlict-listen.js' => '06e7f30e', + 'rsrc/js/application/aphlict/behavior-aphlict-listen.js' => '3c547a81', 'rsrc/js/application/aphlict/behavior-aphlict-status.js' => '5e2634b9', 'rsrc/js/application/aphlict/behavior-desktop-notifications-control.js' => 'd5a2d665', 'rsrc/js/application/calendar/behavior-day-view.js' => '4b3c4443', 'rsrc/js/application/calendar/behavior-event-all-day.js' => 'b41537c9', 'rsrc/js/application/calendar/behavior-month-view.js' => 'fe33e256', 'rsrc/js/application/config/behavior-reorder-fields.js' => 'b6993408', - 'rsrc/js/application/conpherence/ConpherenceThreadManager.js' => '311eae46', + 'rsrc/js/application/conpherence/ConpherenceThreadManager.js' => '4d863052', 'rsrc/js/application/conpherence/behavior-conpherence-search.js' => '9bbf3762', 'rsrc/js/application/conpherence/behavior-durable-column.js' => 'aa3bd034', 'rsrc/js/application/conpherence/behavior-menu.js' => '80dda04a', @@ -561,7 +561,7 @@ return array( 'conpherence-message-pane-css' => '14199428', 'conpherence-notification-css' => 'cef0a3fc', 'conpherence-participant-pane-css' => '26a3ce56', - 'conpherence-thread-manager' => '311eae46', + 'conpherence-thread-manager' => '4d863052', 'conpherence-transaction-css' => '85129c68', 'd3' => 'a11a5ff2', 'differential-changeset-view-css' => '41af6d25', @@ -584,10 +584,10 @@ return array( 'herald-rule-editor' => 'd6a7e717', 'herald-test-css' => 'a52e323e', 'inline-comment-summary-css' => '51efda3a', - 'javelin-aphlict' => '674e335f', + 'javelin-aphlict' => 'e1d4b11a', 'javelin-behavior' => '61cbc29a', 'javelin-behavior-aphlict-dropdown' => 'caade6f2', - 'javelin-behavior-aphlict-listen' => '06e7f30e', + 'javelin-behavior-aphlict-listen' => '3c547a81', 'javelin-behavior-aphlict-status' => '5e2634b9', 'javelin-behavior-aphront-basic-tokenizer' => 'b3a4b884', 'javelin-behavior-aphront-drag-and-drop-textarea' => '484a6e22', @@ -948,20 +948,6 @@ return array( 'javelin-stratcom', 'javelin-workflow', ), - '06e7f30e' => array( - 'javelin-behavior', - 'javelin-aphlict', - 'javelin-stratcom', - 'javelin-request', - 'javelin-uri', - 'javelin-dom', - 'javelin-json', - 'javelin-router', - 'javelin-util', - 'javelin-leader', - 'javelin-sound', - 'phabricator-notification', - ), '0825c27a' => array( 'javelin-behavior', 'javelin-dom', @@ -1133,17 +1119,6 @@ return array( '2ee659ce' => array( 'javelin-install', ), - '311eae46' => array( - 'javelin-dom', - 'javelin-util', - 'javelin-stratcom', - 'javelin-install', - 'javelin-aphlict', - 'javelin-workflow', - 'javelin-router', - 'javelin-behavior-device', - 'javelin-vector', - ), '31420f77' => array( 'javelin-behavior', ), @@ -1166,6 +1141,20 @@ return array( 'javelin-dom', 'javelin-magical-init', ), + '3c547a81' => array( + 'javelin-behavior', + 'javelin-aphlict', + 'javelin-stratcom', + 'javelin-request', + 'javelin-uri', + 'javelin-dom', + 'javelin-json', + 'javelin-router', + 'javelin-util', + 'javelin-leader', + 'javelin-sound', + 'phabricator-notification', + ), '3cb0b2fc' => array( 'javelin-behavior', 'javelin-dom', @@ -1284,6 +1273,17 @@ return array( 'javelin-uri', 'phabricator-notification', ), + '4d863052' => array( + 'javelin-dom', + 'javelin-util', + 'javelin-stratcom', + 'javelin-install', + 'javelin-aphlict', + 'javelin-workflow', + 'javelin-router', + 'javelin-behavior-device', + 'javelin-vector', + ), '4e3e79a6' => array( 'javelin-behavior', 'javelin-stratcom', @@ -1409,13 +1409,6 @@ return array( 'javelin-workflow', 'javelin-dom', ), - '674e335f' => array( - 'javelin-install', - 'javelin-util', - 'javelin-websocket', - 'javelin-leader', - 'javelin-json', - ), '680ea2c8' => array( 'javelin-install', 'javelin-dom', @@ -2127,6 +2120,13 @@ return array( 'javelin-dom', 'phabricator-draggable-list', ), + 'e1d4b11a' => array( + 'javelin-install', + 'javelin-util', + 'javelin-websocket', + 'javelin-leader', + 'javelin-json', + ), 'e1ff79b1' => array( 'javelin-behavior', 'javelin-stratcom', diff --git a/src/applications/notification/controller/PhabricatorNotificationIndividualController.php b/src/applications/notification/controller/PhabricatorNotificationIndividualController.php index 41dade2747..af3e8bcff0 100644 --- a/src/applications/notification/controller/PhabricatorNotificationIndividualController.php +++ b/src/applications/notification/controller/PhabricatorNotificationIndividualController.php @@ -47,6 +47,7 @@ final class PhabricatorNotificationIndividualController 'title' => $data['title'], 'body' => $data['body'], 'content' => hsprintf('%s', $content), + 'uniqueID' => 'story/'.$story->getChronologicalKey(), ); return id(new AphrontAjaxResponse())->setContent($response); diff --git a/webroot/rsrc/js/application/aphlict/Aphlict.js b/webroot/rsrc/js/application/aphlict/Aphlict.js index b2d7ec3215..e27ff1b4eb 100644 --- a/webroot/rsrc/js/application/aphlict/Aphlict.js +++ b/webroot/rsrc/js/application/aphlict/Aphlict.js @@ -29,6 +29,7 @@ JX.install('Aphlict', { this._uri = uri; this._subscriptions = subscriptions; this._setStatus('setup'); + this._startTime = new Date().getTime(); JX.Aphlict._instance = this; }, @@ -42,6 +43,7 @@ JX.install('Aphlict', { _status: null, _isReconnect: false, _keepaliveInterval: false, + _startTime: null, start: function() { JX.Leader.listen('onBecomeLeader', JX.bind(this, this._lead)); @@ -121,8 +123,22 @@ JX.install('Aphlict', { }, replay: function() { + var age = 60000; + + // If the page was loaded a few moments ago, only query for recent + // history. This keeps us from replaying events over and over again as + // a user browses normally. + + // Allow a small margin of error for the actual page load time. It's + // also fine to replay a notification which the user saw for a brief + // moment on the previous page. + var extra_time = 500; + var now = new Date().getTime(); + + age = Math.min(extra_time + (now - this._startTime), age); + var replay = { - age: 60000 + age: age }; JX.Leader.broadcast(null, {type: 'aphlict.replay', data: replay}); diff --git a/webroot/rsrc/js/application/aphlict/behavior-aphlict-listen.js b/webroot/rsrc/js/application/aphlict/behavior-aphlict-listen.js index 398b2af6df..333e8daac1 100644 --- a/webroot/rsrc/js/application/aphlict/behavior-aphlict-listen.js +++ b/webroot/rsrc/js/application/aphlict/behavior-aphlict-listen.js @@ -66,10 +66,12 @@ JX.behavior('aphlict-listen', function(config) { return; } - JX.Leader.broadcast(null, { - type: 'notification.individual', - data: response - }); + JX.Leader.broadcast( + response.uniqueID, + { + type: 'notification.individual', + data: response + }); } JX.Stratcom.listen('aphlict-notification-message', null, function(e) { diff --git a/webroot/rsrc/js/application/conpherence/ConpherenceThreadManager.js b/webroot/rsrc/js/application/conpherence/ConpherenceThreadManager.js index 7c3c749e0f..81e10bf35b 100644 --- a/webroot/rsrc/js/application/conpherence/ConpherenceThreadManager.js +++ b/webroot/rsrc/js/application/conpherence/ConpherenceThreadManager.js @@ -245,6 +245,10 @@ JX.install('ConpherenceThreadManager', { 'aphlict-reconnect', null, JX.bind(this, function() { + if (!this._loadedThreadPHID) { + return; + } + this._updateThread(); }));