Merge pull request #559 from hypothesis/coalesce-anchor-status-updates

Coalesce anchoring status updates

src/sidebar/annotation-ui.js

@@ -114,6 +114,7 @@ module.exports = function ($rootScope, settings) {
     hasSelectedAnnotations: selectionReducer.hasSelectedAnnotations,
 
     annotationExists: annotationsReducer.annotationExists,
+    findAnnotationByID: annotationsReducer.findAnnotationByID,
     findIDsForTags: annotationsReducer.findIDsForTags,
     savedAnnotations: annotationsReducer.savedAnnotations,
 

src/sidebar/frame-sync.js

 'use strict';
 
+var debounce = require('lodash.debounce');
+
 var events = require('./events');
 var bridgeEvents = require('../shared/bridge-events');
 var metadata = require('./annotation-metadata');
@@ -126,12 +128,24 @@ function FrameSync($rootScope, $window, Discovery, annotationUI, bridge) {
 
     bridge.on('destroyFrame', destroyFrame.bind(this));
 
+    // Map of annotation tag to anchoring status
+    // ('anchored'|'orphan'|'timeout').
+    //
+    // Updates are coalesced to reduce the overhead from processing
+    // triggered by each `UPDATE_ANCHOR_STATUS` action that is dispatched.
+    var anchoringStatusUpdates = {};
+    var scheduleAnchoringStatusUpdate = debounce(() => {
+      annotationUI.updateAnchorStatus(anchoringStatusUpdates);
+      $rootScope.$broadcast(events.ANNOTATIONS_SYNCED, Object.keys(anchoringStatusUpdates));
+      anchoringStatusUpdates = {};
+    }, 10);
+
     // Anchoring an annotation in the frame completed
     bridge.on('sync', function (events_) {
       events_.forEach(function (event) {
         inFrame.add(event.tag);
-        annotationUI.updateAnchorStatus(null, event.tag, event.msg.$orphan);
-        $rootScope.$broadcast(events.ANNOTATIONS_SYNCED, [event.tag]);
+        anchoringStatusUpdates[event.tag] = event.msg.$orphan ? 'orphan' : 'anchored';
+        scheduleAnchoringStatusUpdate();
       });
     });
 

src/sidebar/reducers/annotations.js

@@ -173,17 +173,16 @@ var update = {
 
   UPDATE_ANCHOR_STATUS: function (state, action) {
     var annotations = state.annotations.map(function (annot) {
-      var match = (annot.id && annot.id === action.id) ||
-                  (annot.$tag && annot.$tag === action.tag);
-      if (match) {
-        return Object.assign({}, annot, {
-          $anchorTimeout: action.anchorTimeout || annot.$anchorTimeout,
-          $orphan: action.isOrphan,
-          $tag: action.tag,
-        });
-      } else {
+      if (!action.statusUpdates.hasOwnProperty(annot.$tag)) {
         return annot;
       }
+
+      var state = action.statusUpdates[annot.$tag];
+      if (state === 'timeout') {
+        return Object.assign({}, annot, { $anchorTimeout: true });
+      } else {
+        return Object.assign({}, annot, { $orphan: state === 'orphan' });
+      }
     });
     return {annotations: annotations};
   },
@@ -262,22 +261,21 @@ function addAnnotations(annotations, now) {
     // successfully anchor then the status will be updated.
     var ANCHORING_TIMEOUT = 500;
 
-    var anchoringAnnots = added.filter(metadata.isWaitingToAnchor);
-    if (anchoringAnnots.length) {
-      setTimeout(function () {
-        arrayUtil
-          .filterMap(anchoringAnnots, function (annot) {
-            return findByID(getState().annotations, annot.id);
-          })
-          .filter(metadata.isWaitingToAnchor)
-          .forEach(function (orphan) {
-            dispatch({
-              type: actions.UPDATE_ANCHOR_STATUS,
-              anchorTimeout: true,
-              id: orphan.id,
-              tag: orphan.$tag,
-            });
-          });
+    var anchoringIDs = added.filter(metadata.isWaitingToAnchor)
+                            .map(ann => ann.id);
+    if (anchoringIDs.length > 0) {
+      setTimeout(() => {
+        // Find annotations which haven't yet been anchored in the document.
+        var anns = getState().annotations;
+        var annsStillAnchoring = anchoringIDs.map(id => findByID(anns, id))
+                                             .filter(ann => ann && metadata.isWaitingToAnchor(ann));
+
+        // Mark anchoring as timed-out for these annotations.
+        var anchorStatusUpdates = annsStillAnchoring.reduce((updates, ann) => {
+          updates[ann.$tag] = 'timeout';
+          return updates;
+        }, {});
+        dispatch(updateAnchorStatus(anchorStatusUpdates));
       }, ANCHORING_TIMEOUT);
     }
   };
@@ -297,19 +295,14 @@ function clearAnnotations() {
 }
 
 /**
- * Updating the local tag and anchoring status of an annotation.
+ * Update the anchoring status of an annotation.
  *
- * @param {string|null} id - Annotation ID
- * @param {string} tag - The local tag assigned to this annotation to link
- *        the object in the page and the annotation in the sidebar
- * @param {boolean} isOrphan - True if the annotation failed to anchor
+ * @param {{ [tag: string]: 'anchored'|'orphan'|'timeout'} } statusUpdates - A map of annotation tag to orphan status
  */
-function updateAnchorStatus(id, tag, isOrphan) {
+function updateAnchorStatus(statusUpdates) {
   return {
     type: actions.UPDATE_ANCHOR_STATUS,
-    id: id,
-    tag: tag,
-    isOrphan: isOrphan,
+    statusUpdates,
   };
 }
 

src/sidebar/test/annotation-ui-test.js

@@ -26,6 +26,14 @@ describe('annotationUI', function () {
   var annotationUI;
   var fakeRootScope;
 
+  function tagForID(id) {
+    var storeAnn = annotationUI.findAnnotationByID(id);
+    if (!storeAnn) {
+      throw new Error(`No annotation with ID ${id}`);
+    }
+    return storeAnn.$tag;
+  }
+
   beforeEach(function () {
     fakeRootScope = {$applyAsync: sinon.stub()};
     annotationUI = annotationUIFactory(fakeRootScope, {});
@@ -137,7 +145,7 @@ describe('annotationUI', function () {
     it('preserves anchoring status of updated annotations', function () {
       var annot = defaultAnnotation();
       annotationUI.addAnnotations([annot]);
-      annotationUI.updateAnchorStatus(annot.id, null, false /* not an orphan */);
+      annotationUI.updateAnchorStatus({ [tagForID(annot.id)]: 'anchored' });
 
       var update = Object.assign({}, defaultAnnotation(), {text: 'update'});
       annotationUI.addAnnotations([update]);
@@ -158,7 +166,7 @@ describe('annotationUI', function () {
     it('does not set the timeout flag on annotations that do anchor within a time limit', function () {
       var annot = defaultAnnotation();
       annotationUI.addAnnotations([annot]);
-      annotationUI.updateAnchorStatus(annot.id, 'atag', false);
+      annotationUI.updateAnchorStatus({ [tagForID(annot.id)]: 'anchored' });
 
       clock.tick(ANCHOR_TIME_LIMIT);
 
@@ -168,7 +176,7 @@ describe('annotationUI', function () {
     it('does not attempt to modify orphan status if annotations are removed before anchoring timeout expires', function () {
       var annot = defaultAnnotation();
       annotationUI.addAnnotations([annot]);
-      annotationUI.updateAnchorStatus(annot.id, 'atag', false);
+      annotationUI.updateAnchorStatus({ [tagForID(annot.id)]: 'anchored' });
       annotationUI.removeAnnotations([annot]);
 
       assert.doesNotThrow(function () {
@@ -483,28 +491,12 @@ describe('annotationUI', function () {
   });
 
   describe('#updatingAnchorStatus', function () {
-    it("updates the annotation's tag", function () {
-      var annot = defaultAnnotation();
-      annotationUI.addAnnotations([annot]);
-      annotationUI.updateAnchorStatus(annot.id, 'atag', true);
-      assert.equal(annotationUI.getState().annotations[0].$tag, 'atag');
-    });
-
     it("updates the annotation's orphan flag", function () {
       var annot = defaultAnnotation();
       annotationUI.addAnnotations([annot]);
-      annotationUI.updateAnchorStatus(annot.id, 'atag', true);
+      annotationUI.updateAnchorStatus({ [tagForID(annot.id)]: 'orphan' });
       assert.equal(annotationUI.getState().annotations[0].$orphan, true);
     });
-
-    it('updates annotations by tag', function () {
-      annotationUI.addAnnotations(fixtures.newPair);
-      annotationUI.updateAnchorStatus(null, 't2', true);
-
-      var annots = annotationUI.getState().annotations;
-      assert.isFalse(annots[0].$orphan);
-      assert.isTrue(annots[1].$orphan);
-    });
   });
 
   describe('selector functions', function () {

src/sidebar/test/frame-sync-test.js

@@ -50,7 +50,7 @@ var fixtures = {
   },
 };
 
-describe('FrameSync', function () {
+describe('sidebar.frame-sync', function () {
   var fakeAnnotationUI;
   var fakeBridge;
   var frameSync;
@@ -189,9 +189,40 @@ describe('FrameSync', function () {
   });
 
   context('when anchoring completes', function () {
+    var clock = sinon.stub();
+
+    beforeEach(() => {
+      clock = sinon.useFakeTimers();
+    });
+
+    afterEach(() => {
+      clock.restore();
+    });
+
+    function expireDebounceTimeout() {
+      // "Wait" for debouncing timeout to expire and pending anchoring status
+      // updates to be applied.
+      clock.tick(20);
+    }
+
     it('updates the anchoring status for the annotation', function () {
       fakeBridge.emit('sync', [{tag: 't1', msg: {$orphan: false}}]);
-      assert.calledWith(fakeAnnotationUI.updateAnchorStatus, null, 't1', false);
+
+      expireDebounceTimeout();
+
+      assert.calledWith(fakeAnnotationUI.updateAnchorStatus, { t1: 'anchored' });
+    });
+
+    it('coalesces multiple "sync" messages', () => {
+      fakeBridge.emit('sync', [{tag: 't1', msg: {$orphan: false}}]);
+      fakeBridge.emit('sync', [{tag: 't2', msg: {$orphan: true}}]);
+
+      expireDebounceTimeout();
+
+      assert.calledWith(fakeAnnotationUI.updateAnchorStatus, {
+        t1: 'anchored',
+        t2: 'orphan',
+      });
     });
 
     it('emits an ANNOTATIONS_SYNCED event', function () {
@@ -199,6 +230,7 @@ describe('FrameSync', function () {
       $rootScope.$on(events.ANNOTATIONS_SYNCED, onSync);
 
       fakeBridge.emit('sync', [{tag: 't1', msg: {$orphan: false}}]);
+      expireDebounceTimeout();
 
       assert.calledWithMatch(onSync, sinon.match.any, sinon.match(['t1']));
     });