Merge pull request #2103 from hypothesis/migrate-sidebar-content

Migrate `SidebarContent` to preact

src/sidebar/events.js

@@ -19,7 +19,4 @@ export default {
 
   /** A new annotation has been created locally. */
   BEFORE_ANNOTATION_CREATED: 'beforeAnnotationCreated',
-
-  /** Annotations were anchored in a connected document. */
-  ANNOTATIONS_SYNCED: 'sync',
 };

src/sidebar/index.js

@@ -108,14 +108,10 @@ registerIcons(iconSet);
 // Preact UI components that are wrapped for use within Angular templates.
 
 import AnnotationViewerContent from './components/annotation-viewer-content';
-import FocusedModeHeader from './components/focused-mode-header';
 import HelpPanel from './components/help-panel';
-import LoggedOutMessage from './components/logged-out-message';
 import LoginPromptPanel from './components/login-prompt-panel';
-import SearchStatusBar from './components/search-status-bar';
-import SelectionTabs from './components/selection-tabs';
 import ShareAnnotationsPanel from './components/share-annotations-panel';
-import SidebarContentError from './components/sidebar-content-error';
+import SidebarContent from './components/sidebar-content';
 import StreamContent from './components/stream-content';
 import ThreadList from './components/thread-list';
 import ToastMessages from './components/toast-messages';
@@ -124,7 +120,6 @@ import TopBar from './components/top-bar';
 // Remaining UI components that are still built with Angular.
 
 import hypothesisApp from './components/hypothesis-app';
-import sidebarContent from './components/sidebar-content';
 
 // Services.
 
@@ -243,12 +238,7 @@ function startAngularApp(config) {
     )
     .component('helpPanel', wrapComponent(HelpPanel))
     .component('loginPromptPanel', wrapComponent(LoginPromptPanel))
-    .component('loggedOutMessage', wrapComponent(LoggedOutMessage))
-    .component('searchStatusBar', wrapComponent(SearchStatusBar))
-    .component('focusedModeHeader', wrapComponent(FocusedModeHeader))
-    .component('selectionTabs', wrapComponent(SelectionTabs))
-    .component('sidebarContent', sidebarContent)
-    .component('sidebarContentError', wrapComponent(SidebarContentError))
+    .component('sidebarContent', wrapComponent(SidebarContent))
     .component('shareAnnotationsPanel', wrapComponent(ShareAnnotationsPanel))
     .component('streamContent', wrapComponent(StreamContent))
     .component('threadList', wrapComponent(ThreadList))

src/sidebar/services/frame-sync.js

@@ -144,10 +144,6 @@ export default function FrameSync($rootScope, $window, store, bridge) {
     let anchoringStatusUpdates = {};
     const scheduleAnchoringStatusUpdate = debounce(() => {
       store.updateAnchorStatus(anchoringStatusUpdates);
-      $rootScope.$broadcast(
-        events.ANNOTATIONS_SYNCED,
-        Object.keys(anchoringStatusUpdates)
-      );
       anchoringStatusUpdates = {};
     }, 10);
 
@@ -176,7 +172,7 @@ export default function FrameSync($rootScope, $window, store, bridge) {
     });
 
     bridge.on('sidebarOpened', function () {
-      $rootScope.$broadcast('sidebarOpened');
+      store.setSidebarOpened(true);
     });
 
     // These invoke the matching methods by name on the Guests

src/sidebar/services/streamer.js

@@ -3,6 +3,7 @@ import * as queryString from 'query-string';
 import warnOnce from '../../shared/warn-once';
 import { generateHexString } from '../util/random';
 import Socket from '../websocket';
+import { watch } from '../util/watch';
 
 /**
  * Open a new WebSocket connection to the Hypothesis push notification service.
@@ -160,6 +161,26 @@ export default function Streamer(store, auth, groups, session, settings) {
       });
   };
 
+  let reconnectSetUp = false;
+  /**
+   * Set up automatic reconnecting when user changes.
+   */
+  function setUpAutoReconnect() {
+    if (reconnectSetUp) {
+      return;
+    }
+    reconnectSetUp = true;
+
+    // Reconnect when user changes, as auth token will have changed
+    watch(
+      store.subscribe,
+      () => store.profile().userid,
+      () => {
+        reconnect();
+      }
+    );
+  }
+
   /**
    * Connect to the Hypothesis real time update service.
    *
@@ -169,10 +190,10 @@ export default function Streamer(store, auth, groups, session, settings) {
    *                   process has started.
    */
   function connect() {
+    setUpAutoReconnect();
     if (socket) {
       return Promise.resolve();
     }
-
     return _connect();
   }
 

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

@@ -66,6 +66,7 @@ describe('sidebar/services/frame-sync', function () {
         openSidebarPanel: sinon.stub(),
         selectAnnotations: sinon.stub(),
         selectTab: sinon.stub(),
+        setSidebarOpened: sinon.stub(),
         toggleSelectedAnnotations: sinon.stub(),
         updateAnchorStatus: sinon.stub(),
       }
@@ -305,14 +306,6 @@ describe('sidebar/services/frame-sync', function () {
         t2: 'orphan',
       });
     });
-
-    it('emits an ANNOTATIONS_SYNCED event', function () {
-      fakeBridge.emit('sync', [{ tag: 't1', msg: { $orphan: false } }]);
-      expireDebounceTimeout();
-      assert.calledWith($rootScope.$broadcast, events.ANNOTATIONS_SYNCED, [
-        't1',
-      ]);
-    });
   });
 
   context('when a new frame connects', function () {
@@ -376,9 +369,10 @@ describe('sidebar/services/frame-sync', function () {
   });
 
   describe('on "sidebarOpened" message', function () {
-    it('broadcasts a sidebarOpened event', function () {
+    it('sets the sidebar open in the store', function () {
       fakeBridge.emit('sidebarOpened');
-      assert.calledWith($rootScope.$broadcast, 'sidebarOpened');
+
+      assert.calledWith(fakeStore.setSidebarOpened, true);
     });
   });
 

src/sidebar/services/test/streamer-test.js

 import EventEmitter from 'tiny-emitter';
 
+import fakeReduxStore from '../../test/fake-redux-store';
 import Streamer from '../streamer';
 import { $imports } from '../streamer';
 
@@ -43,12 +44,14 @@ const fixtures = {
 
 // the most recently created FakeSocket instance
 let fakeWebSocket = null;
+let fakeWebSockets = [];
 
 class FakeSocket extends EventEmitter {
   constructor(url) {
     super();
 
     fakeWebSocket = this; // eslint-disable-line consistent-this
+    fakeWebSockets.push(this);
 
     this.url = url;
     this.messages = [];
@@ -95,19 +98,22 @@ describe('Streamer', function () {
       },
     };
 
-    fakeStore = {
-      addAnnotations: sinon.stub(),
-      annotationExists: sinon.stub().returns(false),
-      clearPendingUpdates: sinon.stub(),
-      pendingUpdates: sinon.stub().returns({}),
-      pendingDeletions: sinon.stub().returns({}),
-      profile: sinon.stub().returns({
-        userid: 'jim@hypothes.is',
-      }),
-      receiveRealTimeUpdates: sinon.stub(),
-      removeAnnotations: sinon.stub(),
-      route: sinon.stub().returns('sidebar'),
-    };
+    fakeStore = fakeReduxStore(
+      {},
+      {
+        addAnnotations: sinon.stub(),
+        annotationExists: sinon.stub().returns(false),
+        clearPendingUpdates: sinon.stub(),
+        pendingUpdates: sinon.stub().returns({}),
+        pendingDeletions: sinon.stub().returns({}),
+        profile: sinon.stub().returns({
+          userid: 'jim@hypothes.is',
+        }),
+        receiveRealTimeUpdates: sinon.stub(),
+        removeAnnotations: sinon.stub(),
+        route: sinon.stub().returns('sidebar'),
+      }
+    );
 
     fakeGroups = {
       focused: sinon.stub().returns({ id: 'public' }),
@@ -130,6 +136,7 @@ describe('Streamer', function () {
   afterEach(function () {
     $imports.$restore();
     activeStreamer = null;
+    fakeWebSockets = [];
   });
 
   it('should not create a websocket connection if websocketUrl is not provided', function () {
@@ -246,6 +253,47 @@ describe('Streamer', function () {
     });
   });
 
+  describe('Automatic reconnection', function () {
+    function delay(ms) {
+      return new Promise(resolve => setTimeout(resolve, ms));
+    }
+
+    it('should reconnect when user changes', function () {
+      let oldWebSocket;
+      createDefaultStreamer();
+
+      return activeStreamer
+        .connect()
+        .then(function () {
+          oldWebSocket = fakeWebSocket;
+          fakeStore.profile.returns({ userid: 'somebody' });
+          return fakeStore.setState({});
+        })
+        .then(function () {
+          assert.ok(oldWebSocket.didClose);
+          assert.ok(!fakeWebSocket.didClose);
+        });
+    });
+
+    it('should only set up auto-reconnect once', async () => {
+      createDefaultStreamer();
+      // This should register auto-reconnect
+      await activeStreamer.connect();
+      // Call connect again: this should not "re-register" auto-reconnect
+      await activeStreamer.connect();
+
+      // This should trigger auto-reconnect, but only once, proving that
+      // only one registration happened
+      fakeStore.profile.returns({ userid: 'somebody' });
+      fakeStore.setState({});
+
+      await delay(1);
+      // Total number of web sockets blown through in this test should be 2
+      // 3+ would indicate `reconnect` fired more than once
+      assert.lengthOf(fakeWebSockets, 2);
+    });
+  });
+
   describe('annotation notifications', function () {
     beforeEach(function () {
       createDefaultStreamer();

src/sidebar/store/modules/activity.js

@@ -19,6 +19,10 @@ function init() {
      * The number of annotation fetches that have started and not yet completed.
      */
     activeAnnotationFetches: 0,
+    /**
+     * Have annotations ever been fetched?
+     */
+    hasFetchedAnnotations: false,
   };
 }
 
@@ -86,6 +90,7 @@ const update = {
 
     return {
       ...state,
+      hasFetchedAnnotations: true,
       activeAnnotationFetches: state.activeAnnotationFetches - 1,
     };
   },
@@ -127,6 +132,10 @@ function apiRequestFinished() {
 
 /** Selectors */
 
+function hasFetchedAnnotations(state) {
+  return state.activity.hasFetchedAnnotations;
+}
+
 /**
  * Return true when annotations are actively being fetched.
  */
@@ -173,6 +182,7 @@ export default {
   },
 
   selectors: {
+    hasFetchedAnnotations,
     isLoading,
     isFetchingAnnotations,
     isSavingAnnotation,

src/sidebar/store/modules/direct-linked.js

@@ -136,6 +136,10 @@ function directLinkedGroupId(state) {
   return state.directLinked.directLinkedGroupId;
 }
 
+function directLinkedGroupFetchFailed(state) {
+  return state.directLinked.directLinkedGroupFetchFailed;
+}
+
 export default {
   init,
   namespace: 'directLinked',
@@ -149,6 +153,7 @@ export default {
   },
   selectors: {
     directLinkedAnnotationId,
+    directLinkedGroupFetchFailed,
     directLinkedGroupId,
   },
 };

src/sidebar/store/modules/frames.js

-import { createSelector } from 'reselect';
+import {
+  createSelector,
+  createSelectorCreator,
+  defaultMemoize,
+} from 'reselect';
+import shallowEqual from 'shallowequal';
 
 import * as util from '../util';
 
@@ -103,15 +108,23 @@ function searchUrisForFrame(frame) {
   return uris;
 }
 
-/**
- * Return the set of URIs that should be used to search for annotations on the
- * current page.
- */
-function searchUris(state) {
-  return state.frames.reduce(function (uris, frame) {
-    return uris.concat(searchUrisForFrame(frame));
-  }, []);
-}
+// "selector creator" that uses `shallowEqual` instead of `===` for memoization
+const createShallowEqualSelector = createSelectorCreator(
+  defaultMemoize,
+  shallowEqual
+);
+
+// Memoized selector will return the same array (of URIs) reference unless the
+// values of the array change (are not shallow-equal).
+const searchUris = createShallowEqualSelector(
+  state => {
+    return state.frames.reduce(
+      (uris, frame) => uris.concat(searchUrisForFrame(frame)),
+      []
+    );
+  },
+  uris => uris
+);
 
 export default {
   init: init,

src/sidebar/store/modules/selection.js

@@ -519,6 +519,21 @@ function getSelectedAnnotationMap(state) {
   return state.selection.selectedAnnotationMap;
 }
 
+/**
+ * Is any sort of filtering currently applied to the list of annotations? This
+ * includes a search query, but also if annotations are selected or a user
+ * is focused.
+ *
+ * @return {boolean}
+ */
+const hasAppliedFilter = createSelector(
+  filterQuery,
+  focusModeFocused,
+  hasSelectedAnnotations,
+  (filterQuery, focusModeFocused, hasSelectedAnnotations) =>
+    !!filterQuery || focusModeFocused || hasSelectedAnnotations
+);
+
 export default {
   init: init,
   namespace: 'selection',
@@ -541,7 +556,6 @@ export default {
   },
 
   selectors: {
-    hasSelectedAnnotations,
     expandedThreads,
     filterQuery,
     focusModeFocused,
@@ -553,5 +567,7 @@ export default {
     isAnnotationSelected,
     getFirstSelectedAnnotationId,
     getSelectedAnnotationMap,
+    hasAppliedFilter,
+    hasSelectedAnnotations,
   },
 };

src/sidebar/store/modules/test/activity-test.js

@@ -8,6 +8,23 @@ describe('sidebar/store/modules/activity', () => {
     store = createStore([activity]);
   });
 
+  describe('hasFetchedAnnotations', () => {
+    it('returns false if no fetches have completed yet', () => {
+      assert.isFalse(store.hasFetchedAnnotations());
+    });
+
+    it('returns false after fetch(es) started', () => {
+      store.annotationFetchStarted();
+      assert.isFalse(store.hasFetchedAnnotations());
+    });
+
+    it('returns true once a fetch has finished', () => {
+      store.annotationFetchStarted();
+      store.annotationFetchFinished();
+      assert.isTrue(store.hasFetchedAnnotations());
+    });
+  });
+
   describe('#isLoading', () => {
     it('returns false with the initial state', () => {
       assert.equal(store.isLoading(), false);

src/sidebar/store/modules/test/direct-linked-test.js

@@ -88,6 +88,13 @@ describe('sidebar/store/modules/direct-linked', () => {
       });
     });
 
+    describe('#directLinkedGroupFetchFailed', () => {
+      it('should return the group fetch failed status', () => {
+        store.setDirectLinkedGroupFetchFailed(true);
+        assert.isTrue(store.directLinkedGroupFetchFailed());
+      });
+    });
+
     describe('#directLinkedGroupId', () => {
       it('should return the current direct-linked group ID', () => {
         store.setDirectLinkedGroupId('group-id');

src/sidebar/store/modules/test/frames-test.js

@@ -157,7 +157,12 @@ describe('sidebar/store/modules/frames', function () {
         testCase.frames.forEach(frame => {
           store.connectFrame(frame);
         });
-        assert.deepEqual(store.searchUris(), testCase.searchUris);
+        const firstResults = store.searchUris();
+        const secondResults = store.searchUris();
+        assert.deepEqual(firstResults, testCase.searchUris);
+        // The selector is memoized and should return the same Array reference
+        // assuming the list of search URIs hasn't changed
+        assert.equal(firstResults, secondResults);
       });
     });
   });

src/sidebar/store/modules/test/selection-test.js

@@ -62,6 +62,34 @@ describe('sidebar/store/modules/selection', () => {
     });
   });
 
+  describe('hasAppliedFilter', () => {
+    it('returns true if there is a search query set', () => {
+      store.setFilterQuery('foobar');
+
+      assert.isTrue(store.hasAppliedFilter());
+    });
+
+    it('returns true if in user-focused mode', () => {
+      store = createStore([selection], [{ focus: { user: {} } }]);
+      store.setFocusModeFocused(true);
+
+      assert.isTrue(store.hasAppliedFilter());
+    });
+
+    it('returns true if there are selected annotations', () => {
+      store.selectAnnotations([1]);
+
+      assert.isTrue(store.hasAppliedFilter());
+    });
+
+    it('returns false after selection is cleared', () => {
+      store.setFilterQuery('foobar');
+      store.clearSelection();
+
+      assert.isFalse(store.hasAppliedFilter());
+    });
+  });
+
   describe('hasSelectedAnnotations', function () {
     it('returns true if there are any selected annotations', function () {
       store.selectAnnotations([1]);

src/sidebar/store/modules/test/viewer-test.js

@@ -19,4 +19,23 @@ describe('store/modules/viewer', function () {
       assert.isFalse(store.getState().viewer.visibleHighlights);
     });
   });
+
+  describe('hasSidebarOpened', () => {
+    it('is `false` if sidebar has never been opened', () => {
+      assert.isFalse(store.hasSidebarOpened());
+      store.setSidebarOpened(false);
+      assert.isFalse(store.hasSidebarOpened());
+    });
+
+    it('is `true` if sidebar has been opened', () => {
+      store.setSidebarOpened(true);
+      assert.isTrue(store.hasSidebarOpened());
+    });
+
+    it('is `true` if sidebar is closed after being opened', () => {
+      store.setSidebarOpened(true);
+      store.setSidebarOpened(false);
+      assert.isTrue(store.hasSidebarOpened());
+    });
+  });
 });

src/sidebar/store/modules/viewer.js

@@ -7,6 +7,9 @@ import * as util from '../util';
 
 function init() {
   return {
+    // Has the sidebar ever been opened? NB: This is not necessarily the
+    // current state of the sidebar, but tracks whether it has ever been open
+    sidebarHasOpened: false,
     visibleHighlights: false,
   };
 }
@@ -15,10 +18,20 @@ const update = {
   SET_HIGHLIGHTS_VISIBLE: function (state, action) {
     return { visibleHighlights: action.visible };
   },
+  SET_SIDEBAR_OPENED: (state, action) => {
+    if (action.opened === true) {
+      // If the sidebar is open, track that it has ever been opened
+      return { sidebarHasOpened: true };
+    }
+    // Otherwise, nothing to do here
+    return {};
+  },
 };
 
 const actions = util.actionTypes(update);
 
+// Action creators
+
 /**
  * Sets whether annotation highlights in connected documents are shown
  * or not.
@@ -27,12 +40,28 @@ function setShowHighlights(show) {
   return { type: actions.SET_HIGHLIGHTS_VISIBLE, visible: show };
 }
 
+/**
+ * @param {boolean} sidebarState - If the sidebar is open
+ */
+function setSidebarOpened(opened) {
+  return { type: actions.SET_SIDEBAR_OPENED, opened };
+}
+
+// Selectors
+
+function hasSidebarOpened(state) {
+  return state.viewer.sidebarHasOpened;
+}
+
 export default {
   init: init,
   namespace: 'viewer',
   update: update,
   actions: {
-    setShowHighlights: setShowHighlights,
+    setShowHighlights,
+    setSidebarOpened,
+  },
+  selectors: {
+    hasSidebarOpened,
   },
-  selectors: {},
 };

src/sidebar/templates/hypothesis-app.html

@@ -15,11 +15,7 @@
     <main ng-if="vm.route()">
       <annotation-viewer-content ng-if="vm.route() == 'annotation'"></annotation-viewer-content>
       <stream-content ng-if="vm.route() == 'stream'"></stream-content>
-      <sidebar-content
-        ng-if="vm.route() == 'sidebar'"
-        auth="vm.auth"
-        on-login="vm.login()"
-        on-sign-up="vm.signUp()"></sidebar-content>
+      <sidebar-content ng-if="vm.route() == 'sidebar'" on-login="vm.login()" on-signUp="vm.signUp()"></sidebar-content>
     </main>
   </div>
 </div>

src/sidebar/templates/sidebar-content.html

-<focused-mode-header
-  ng-if="vm.showFocusedHeader()">
-</focused-mode-header>
-
-<login-prompt-panel on-login="vm.onLogin()" on-sign-up="vm.onSignUp()"></login-prompt-panel>
-
-
-<!-- Display error message if direct-linked annotation fetch failed. -->
-<sidebar-content-error
-  error-type="'annotation'"
-  on-login-request="vm.onLogin()"
-  ng-if="vm.selectedAnnotationUnavailable()"
->
-</sidebar-content-error>
-
-<!-- Display error message if direct-linked group fetch failed. -->
-<sidebar-content-error
-  error-type="'group'"
-  on-login-request="vm.onLogin()"
-  ng-if="vm.selectedGroupUnavailable()"
->
-</sidebar-content-error>
-
-
-<selection-tabs
-  ng-if="vm.showSelectedTabs()"
-  is-loading="vm.isLoading()">
-</selection-tabs>
-
-
-
-<search-status-bar
-  ng-if="!vm.isLoading() && !(vm.selectedAnnotationUnavailable() || vm.selectedGroupUnavailable())">
-</search-status-bar>
-
-<thread-list thread="vm.rootThread()"></thread-list>
-
-<logged-out-message ng-if="vm.shouldShowLoggedOutMessage()" on-login="vm.onLogin()">
-</logged-out-message>