Merge pull request #20 from hypothesis/websocket-defer-initialization

Defer initialization of websocket client until sidebar is interacted with for logged-out users

h/static/scripts/annotation-ui-sync.js

@@ -56,6 +56,9 @@ function AnnotationUISync($rootScope, $window, annotationUI, bridge) {
         bridge.call('setVisibleHighlights', state);
       }
     },
+    sidebarOpened: function () {
+      $rootScope.$broadcast('sidebarOpened');
+    },
   };
 
   for (var channel in channelListeners) {

h/static/scripts/annotation-viewer-controller.js

@@ -71,6 +71,7 @@ function AnnotationViewerController (
       .addClause('/references', 'one_of', topLevelAnnot.id, true)
       .addClause('/id', 'equals', topLevelAnnot.id, true);
     streamer.setConfig('filter', { filter: streamFilter.getFilter() });
+    streamer.connect();
 
     annots.forEach(function (annot) {
       annotationUI.setCollapsed(annot.id, false);

h/static/scripts/annotator/sidebar.coffee

@@ -133,6 +133,8 @@ module.exports = class Sidebar extends Host
         this.hide()
 
   show: ->
+    @crossframe.call('sidebarOpened')
+
     @frame.css 'margin-left': "#{-1 * @frame.width()}px"
     @frame.removeClass 'annotator-collapsed'
 

h/static/scripts/stream-controller.coffee

@@ -44,6 +44,7 @@ module.exports = class StreamController
     terms = searchFilter.generateFacetedFilter $routeParams.q
     queryParser.populateFilter streamFilter, terms
     streamer.setConfig('filter', {filter: streamFilter.getFilter()})
+    streamer.connect()
 
     # Perform the initial search
     fetch(20)

h/static/scripts/streamer.js

@@ -63,6 +63,41 @@ function Streamer($rootScope, annotationMapper, groups, session, settings) {
     session.update(message.model);
   }
 
+  function handleSocketOnError (event) {
+    console.warn('Error connecting to H push notification service:', event);
+
+    // In development, warn if the connection failure might be due to
+    // the app's origin not having been whitelisted in the H service's config.
+    //
+    // Unfortunately the error event does not provide a way to get at the
+    // HTTP status code for HTTP -> WS upgrade requests.
+    var websocketHost = new URL(settings.websocketUrl).hostname;
+    if (['localhost', '127.0.0.1'].indexOf(websocketHost) !== -1) {
+      console.warn('Check that your H service is configured to allow ' +
+                   'WebSocket connections from ' + window.location.origin);
+    }
+  }
+
+  function handleSocketOnMessage (event) {
+    // Wrap message dispatches in $rootScope.$apply() so that
+    // scope watches on app state affected by the received message
+    // are updated
+    $rootScope.$apply(function () {
+      var message = JSON.parse(event.data);
+      if (!message) {
+        return;
+      }
+
+      if (message.type === 'annotation-notification') {
+        handleAnnotationNotification(message);
+      } else if (message.type === 'session-change') {
+        handleSessionChangeNotification(message);
+      } else {
+        console.warn('received unsupported notification', message.type);
+      }
+    });
+  }
+
   function sendClientConfig () {
     Object.keys(configMessages).forEach(function (key) {
       if (configMessages[key]) {
@@ -83,73 +118,48 @@ function Streamer($rootScope, annotationMapper, groups, session, settings) {
     }
   }
 
-  var connect = function () {
-    // If we have no URL configured, don't do anything.
+  var _connect = function () {
     var url = settings.websocketUrl;
+
+    // If we have no URL configured, don't do anything.
     if (!url) {
       return;
     }
 
-    // Open a new socket
-    if (socket) {
-      socket.close();
-    }
-
     socket = new Socket(url);
+
+    socket.on('open', sendClientConfig);
+    socket.on('error', handleSocketOnError);
+    socket.on('message', handleSocketOnMessage);
+
+    // Configure the client ID
     setConfig('client-id', {
       messageType: 'client_id',
       value: clientId,
     });
+  };
 
-    socket.on('open', function () {
-      sendClientConfig();
-    });
-
-    socket.on('error', function (event) {
-      console.warn('Error connecting to H push notification service:', event);
-
-      // In development, warn if the connection failure might be due to
-      // the app's origin not having been whitelisted in the H service's config.
-      //
-      // Unfortunately the error event does not provide a way to get at the
-      // HTTP status code for HTTP -> WS upgrade requests.
-      var websocketHost = new URL(url).hostname;
-      if (['localhost', '127.0.0.1'].indexOf(websocketHost) !== -1) {
-        console.warn('Check that your H service is configured to allow ' +
-                     'WebSocket connections from ' + window.location.origin);
-      }
-    });
+  var connect = function () {
+    if (socket) {
+      return;
+    }
 
-    socket.on('message', function (event) {
-      // Wrap message dispatches in $rootScope.$apply() so that
-      // scope watches on app state affected by the received message
-      // are updated
-      //
-      // Note: The use of $apply() here will no longer be needed once session
-      // state is moved to the Redux store in `annotationUI`.
-      $rootScope.$apply(function () {
-        var message = JSON.parse(event.data);
-        if (!message) {
-          return;
-        }
-
-        if (message.type === 'annotation-notification') {
-          handleAnnotationNotification(message);
-        } else if (message.type === 'session-change') {
-          handleSessionChangeNotification(message);
-        } else {
-          console.warn('received unsupported notification', message.type);
-        }
-      });
-    });
+    _connect();
   };
 
-  connect();
+  var reconnect = function () {
+    if (socket) {
+      socket.close();
+    }
+
+    _connect();
+  };
 
-  this.connect = connect;
   this.clientId = clientId;
+  this.configMessages = configMessages;
+  this.connect = connect;
+  this.reconnect = reconnect;
   this.setConfig = setConfig;
-  this.socket = socket;
 }
 
 module.exports = Streamer;

h/static/scripts/test/annotation-viewer-controller-test.js

@@ -62,7 +62,10 @@ describe('AnnotationViewerController', function () {
         subscribe: sinon.stub(),
       },
       rootThread: {thread: sinon.stub()},
-      streamer: { setConfig: function () {} },
+      streamer: {
+        setConfig: function () {},
+        connect: function () {},
+      },
       store: opts.store,
       streamFilter: {
         setMatchPolicyIncludeAny: function () {

h/static/scripts/test/stream-controller-test.coffee

@@ -69,6 +69,7 @@ describe 'StreamController', ->
       open: sandbox.spy()
       close: sandbox.spy()
       setConfig: sandbox.spy()
+      connect: sandbox.spy()
     }
 
     fakeStreamFilter = {

h/static/scripts/test/streamer-test.js

@@ -30,7 +30,7 @@ function FakeSocket() {
 }
 inherits(FakeSocket, EventEmitter);
 
-describe('streamer', function () {
+describe('Streamer', function () {
   var fakeAnnotationMapper;
   var fakeGroups;
   var fakeRootScope;
@@ -82,28 +82,61 @@ describe('streamer', function () {
 
   it('should not create a websocket connection if websocketUrl is not provided', function () {
     fakeSettings = {};
+    createDefaultStreamer();
+    activeStreamer.connect();
+    assert.isNull(fakeWebSocket);
+  });
+
+  it('should not create a websocket connection', function () {
     createDefaultStreamer();
     assert.isNull(fakeWebSocket);
   });
 
-  it('should send a client ID', function () {
+  it('should have a non-null client ID', function () {
     createDefaultStreamer();
+    assert.ok(activeStreamer.clientId);
+  });
+
+  it('should send the client ID on connection', function () {
+    createDefaultStreamer();
+    activeStreamer.connect();
     assert.equal(fakeWebSocket.messages.length, 1);
     assert.equal(fakeWebSocket.messages[0].messageType, 'client_id');
     assert.equal(fakeWebSocket.messages[0].value, activeStreamer.clientId);
   });
 
-  it('should close any existing socket', function () {
-    createDefaultStreamer();
-    var oldWebSocket = fakeWebSocket;
-    activeStreamer.connect();
-    assert.ok(oldWebSocket.didClose);
-    assert.ok(!fakeWebSocket.didClose);
+  describe('#connect()', function () {
+    it('should create a websocket connection', function () {
+      createDefaultStreamer();
+      activeStreamer.connect();
+      assert.ok(fakeWebSocket);
+    });
+
+    it('should not close any existing socket', function () {
+      createDefaultStreamer();
+      activeStreamer.connect();
+      var oldWebSocket = fakeWebSocket;
+      activeStreamer.connect();
+      assert.ok(!oldWebSocket.didClose);
+      assert.ok(!fakeWebSocket.didClose);
+    });
+  });
+
+  describe('#reconnect()', function () {
+    it('should close the existing socket', function () {
+      createDefaultStreamer();
+      activeStreamer.connect();
+      var oldWebSocket = fakeWebSocket;
+      activeStreamer.reconnect();
+      assert.ok(oldWebSocket.didClose);
+      assert.ok(!fakeWebSocket.didClose);
+    });
   });
 
   describe('annotation notifications', function () {
     it('should load new annotations', function () {
       createDefaultStreamer();
+      activeStreamer.connect();
       fakeWebSocket.notify({
         type: 'annotation-notification',
         options: {
@@ -118,6 +151,7 @@ describe('streamer', function () {
 
     it('should unload deleted annotations', function () {
       createDefaultStreamer();
+      activeStreamer.connect();
       fakeWebSocket.notify({
         type: 'annotation-notification',
         options: {
@@ -134,6 +168,7 @@ describe('streamer', function () {
   describe('session change notifications', function () {
     it('updates the session when a notification is received', function () {
       createDefaultStreamer();
+      activeStreamer.connect();
       var model = {
         groups: [{
           id: 'new-group',
@@ -150,6 +185,7 @@ describe('streamer', function () {
   describe('reconnections', function () {
     it('resends configuration messages when a reconnection occurs', function () {
       createDefaultStreamer();
+      activeStreamer.connect();
       fakeWebSocket.messages = [];
       fakeWebSocket.emit('open');
       assert.equal(fakeWebSocket.messages.length, 1);

h/static/scripts/test/widget-controller-test.js

@@ -99,6 +99,8 @@ describe('WidgetController', function () {
 
     fakeStreamer = {
       setConfig: sandbox.spy(),
+      connect: sandbox.spy(),
+      reconnect: sandbox.spy(),
     };
 
     fakeStreamFilter = {
@@ -136,6 +138,7 @@ describe('WidgetController', function () {
   beforeEach(angular.mock.inject(function ($controller, _annotationUI_, _$rootScope_) {
     $rootScope = _$rootScope_;
     $scope = $rootScope.$new();
+    $scope.auth = {'status': 'unknown'};
     annotationUI = _annotationUI_;
     $controller('WidgetController', {$scope: $scope});
   }));
@@ -441,6 +444,27 @@ describe('WidgetController', function () {
     });
   });
 
+  describe('deferred websocket connection', function () {
+    it('should connect the websocket the first time the sidebar opens', function () {
+      $rootScope.$broadcast('sidebarOpened');
+      assert.called(fakeStreamer.connect);
+    });
+
+    describe('when logged in user changes', function () {
+      it('should not reconnect if the sidebar is closed', function () {
+        $rootScope.$broadcast(events.USER_CHANGED);
+        assert.calledOnce(fakeStreamer.reconnect);
+      });
+
+      it('should reconnect if the sidebar is open', function () {
+        $rootScope.$broadcast('sidebarOpened');
+        fakeStreamer.connect.reset();
+        $rootScope.$broadcast(events.USER_CHANGED);
+        assert.called(fakeStreamer.reconnect);
+      });
+    });
+  });
+
   describe('#forceVisible', function () {
     it('shows the thread', function () {
       var thread = {id: '1'};

h/static/scripts/widget-controller.js

@@ -258,8 +258,10 @@ module.exports = function WidgetController(
    * @param {Array<{uri:string}>} frames - Hypothesis client frames
    *        to load annotations for.
    */
-  function loadAnnotations(frames) {
-    _resetAnnotations();
+  function loadAnnotations(frames, reset) {
+    if (reset || typeof reset === 'undefined') {
+      _resetAnnotations();
+    }
 
     searchClients.forEach(function (client) {
       client.cancel();
@@ -297,6 +299,19 @@ module.exports = function WidgetController(
     }
   }
 
+  $scope.$on('sidebarOpened', function () {
+    streamer.connect();
+  });
+
+  // If the user is logged in, we connect nevertheless
+  if ($scope.auth.status === 'logged-in') {
+    streamer.connect();
+  }
+
+  $scope.$on(events.USER_CHANGED, function () {
+    streamer.reconnect();
+  });
+
   // When a direct-linked annotation is successfully anchored in the page,
   // focus and scroll to it
   $rootScope.$on(events.ANNOTATIONS_SYNCED, function (event, tags) {