Merge pull request #2125 from hypothesis/remove-before-annotation-created-event

Replace the `BEFORE_ANNOTATION_CREATED` event

src/sidebar/components/test/thread-list-test.js

 import { mount } from 'enzyme';
 import { createElement } from 'preact';
-
-import events from '../../events';
 import { act } from 'preact/test-utils';
 
 import ThreadList from '../thread-list';
@@ -14,21 +12,15 @@ describe('ThreadList', () => {
   let fakeDomUtil;
   let fakeMetadata;
   let fakeTopThread;
-  let fakeRootScope;
   let fakeScrollContainer;
   let fakeStore;
   let fakeVisibleThreadsUtil;
   let wrappers;
 
   function createComponent(props) {
-    const wrapper = mount(
-      <ThreadList
-        thread={fakeTopThread}
-        $rootScope={fakeRootScope}
-        {...props}
-      />,
-      { attachTo: fakeScrollContainer }
-    );
+    const wrapper = mount(<ThreadList thread={fakeTopThread} {...props} />, {
+      attachTo: fakeScrollContainer,
+    });
     wrappers.push(wrapper);
     return wrapper;
   }
@@ -40,23 +32,6 @@ describe('ThreadList', () => {
     };
     fakeMetadata = {
       isHighlight: sinon.stub().returns(false),
-      isReply: sinon.stub().returns(false),
-    };
-
-    fakeRootScope = {
-      eventCallbacks: {},
-
-      $apply: function (callback) {
-        callback();
-      },
-
-      $on: function (event, callback) {
-        if (event === events.BEFORE_ANNOTATION_CREATED) {
-          this.eventCallbacks[event] = callback;
-        }
-      },
-
-      $broadcast: sinon.stub(),
     };
 
     fakeScrollContainer = document.createElement('div');
@@ -66,6 +41,7 @@ describe('ThreadList', () => {
 
     fakeStore = {
       clearSelection: sinon.stub(),
+      unsavedAnnotations: sinon.stub().returns([]),
     };
 
     fakeTopThread = {
@@ -118,41 +94,27 @@ describe('ThreadList', () => {
     );
   });
 
-  context('new annotation created in application', () => {
-    it('attaches a listener for the BEFORE_ANNOTATION_CREATED event', () => {
-      fakeRootScope.$on = sinon.stub();
-
-      createComponent();
-
-      assert.calledWith(
-        fakeRootScope.$on,
-        events.BEFORE_ANNOTATION_CREATED,
-        sinon.match.func
-      );
-    });
+  /**
+   * Simulate what happens when a new draft annotation is created in the
+   * application.
+   */
+  const addNewAnnotation = (wrapper, annotation = { $tag: 'foobar' }) => {
+    fakeStore.unsavedAnnotations.returns([annotation]);
+    wrapper.setProps({});
+  };
 
+  context('new annotation created in application', () => {
     it('clears the current selection in the store', () => {
-      createComponent();
-
-      fakeRootScope.eventCallbacks[events.BEFORE_ANNOTATION_CREATED]({}, {});
-
+      const wrapper = createComponent();
+      addNewAnnotation(wrapper);
       assert.calledOnce(fakeStore.clearSelection);
     });
 
     it('does not clear the selection in the store if new annotation is a highlight', () => {
       fakeMetadata.isHighlight.returns(true);
-      createComponent();
-
-      fakeRootScope.eventCallbacks[events.BEFORE_ANNOTATION_CREATED]({}, {});
-
-      assert.notCalled(fakeStore.clearSelection);
-    });
-
-    it('does not clear the selection in the store if new annotation is a reply', () => {
-      fakeMetadata.isReply.returns(true);
-      createComponent();
+      const wrapper = createComponent();
 
-      fakeRootScope.eventCallbacks[events.BEFORE_ANNOTATION_CREATED]({}, {});
+      addNewAnnotation(wrapper);
 
       assert.notCalled(fakeStore.clearSelection);
     });
@@ -181,27 +143,17 @@ describe('ThreadList', () => {
     });
 
     it('should do nothing if the annotation thread to scroll to is not in DOM', () => {
-      createComponent();
+      const wrapper = createComponent();
 
-      act(() => {
-        fakeRootScope.eventCallbacks[events.BEFORE_ANNOTATION_CREATED](
-          {},
-          { $tag: 'nonexistent' }
-        );
-      });
+      addNewAnnotation(wrapper);
 
       assert.notCalled(fakeScrollTop);
     });
 
     it('should set the scroll container `scrollTop` to derived position of thread', () => {
-      createComponent();
+      const wrapper = createComponent();
 
-      act(() => {
-        fakeRootScope.eventCallbacks[events.BEFORE_ANNOTATION_CREATED](
-          {},
-          fakeTopThread.children[3].annotation
-        );
-      });
+      addNewAnnotation(wrapper, fakeTopThread.children[3].annotation);
 
       // The third thread in a collection of threads at default height (200)
       // should be at 600px. This setting of `scrollTop` is the only externally-

src/sidebar/components/thread-list.js

@@ -3,11 +3,9 @@ import { useEffect, useMemo, useState } from 'preact/hooks';
 import propTypes from 'prop-types';
 import debounce from 'lodash.debounce';
 
-import events from '../events';
 import useStore from '../store/use-store';
-import { isHighlight, isReply } from '../util/annotation-metadata';
+import { isHighlight } from '../util/annotation-metadata';
 import { getElementHeightWithMargins } from '../util/dom';
-import { withServices } from '../util/service-context';
 import {
   calculateVisibleThreads,
   THREAD_DIMENSION_DEFAULTS,
@@ -32,7 +30,7 @@ function getScrollContainer() {
  * user-defined content may include rich media such as images, audio clips,
  * embedded YouTube videos, rendered math and more.
  */
-function ThreadList({ thread, $rootScope }) {
+function ThreadList({ thread }) {
   const clearSelection = useStore(store => store.clearSelection);
 
   // Height of the visible area of the scroll container.
@@ -72,24 +70,33 @@ function ThreadList({ thread, $rootScope }) {
     [topLevelThreads, threadHeights, scrollPosition, scrollContainerHeight]
   );
 
-  // Listen for when a new annotation is created in the application, and trigger
-  // a scroll to that annotation's thread card (unless highlight or reply)
+  // Get the `$tag` of the most recently created unsaved annotation.
+  const newAnnotationTag = useStore(store => {
+    // If multiple unsaved annotations exist, assume that the last one in the
+    // list is the most recently created one.
+    const newAnnotations = store
+      .unsavedAnnotations()
+      .filter(ann => !ann.id && !isHighlight(ann));
+    if (!newAnnotations.length) {
+      return null;
+    }
+    return newAnnotations[newAnnotations.length - 1].$tag;
+  });
+
+  // Scroll to newly created annotations and replies.
+  //
+  // nb. If there are multiple unsaved annotations and the newest one is saved
+  // or removed, `newAnnotationTag` will revert to the previous unsaved annotation
+  // and the thread list will scroll to that.
   useEffect(() => {
-    const removeListener = $rootScope.$on(
-      events.BEFORE_ANNOTATION_CREATED,
-      (event, annotation) => {
-        if (!isHighlight(annotation) && !isReply(annotation)) {
-          clearSelection();
-          setScrollToId(annotation.$tag);
-        }
-      }
-    );
-    return removeListener;
-  }, [$rootScope, clearSelection]);
+    if (newAnnotationTag) {
+      clearSelection();
+      setScrollToId(newAnnotationTag);
+    }
+  }, [clearSelection, newAnnotationTag]);
 
   // Effect to scroll a particular thread into view. This is mainly used to
-  // scroll a newly created annotation into view (as triggered by the
-  // listener for `BEFORE_ANNOTATION_CREATED`)
+  // scroll a newly created annotation into view.
   useEffect(() => {
     if (!scrollToId) {
       return;
@@ -97,12 +104,8 @@ function ThreadList({ thread, $rootScope }) {
 
     const threadIndex = topLevelThreads.findIndex(t => t.id === scrollToId);
     if (threadIndex === -1) {
-      // Thread is not currently present.
-      //
-      // When `scrollToId` is set as a result of a `BEFORE_ANNOTATION_CREATED`
-      // event, the annotation is not always present in the _next_ render after
-      // that event is received, but in another render after that. Therefore
-      // we wait until the annotation appears before "consuming" the scroll-to-id.
+      // Thread is not currently present. The `scrollToId` will be consumed
+      // when this thread appears.
       return;
     }
 
@@ -189,11 +192,6 @@ function ThreadList({ thread, $rootScope }) {
 ThreadList.propTypes = {
   /** Should annotations render extra document metadata? */
   thread: propTypes.object.isRequired,
-
-  /** injected */
-  $rootScope: propTypes.object.isRequired,
 };
 
-ThreadList.injectedProps = ['$rootScope'];
-
-export default withServices(ThreadList);
+export default ThreadList;

src/sidebar/events.js

@@ -10,11 +10,4 @@ export default {
    * instance.
    */
   OAUTH_TOKENS_CHANGED: 'oauthTokensChanged',
-
-  // UI state changes
-
-  // Annotation events
-
-  /** A new annotation has been created locally. */
-  BEFORE_ANNOTATION_CREATED: 'beforeAnnotationCreated',
 };

src/sidebar/services/frame-sync.js

 import debounce from 'lodash.debounce';
 
 import bridgeEvents from '../../shared/bridge-events';
-import events from '../events';
 import Discovery from '../../shared/discovery';
 import uiConstants from '../ui-constants';
 import * as metadata from '../util/annotation-metadata';
@@ -41,7 +40,7 @@ export function formatAnnot(ann) {
  * sidebar.
  */
 // @ngInject
-export default function FrameSync($rootScope, $window, store, bridge) {
+export default function FrameSync(annotationsService, bridge, store) {
   // Set of tags of annotations that are currently loaded into the frame
   const inFrame = new Set();
 
@@ -131,7 +130,9 @@ export default function FrameSync($rootScope, $window, store, bridge) {
         return;
       }
       inFrame.add(event.tag);
-      $rootScope.$broadcast(events.BEFORE_ANNOTATION_CREATED, annot);
+
+      // Create the new annotation in the sidebar.
+      annotationsService.create(annot);
     });
 
     bridge.on('destroyFrame', destroyFrame.bind(this));

src/sidebar/services/root-thread.js

 import buildThread from '../build-thread';
-import events from '../events';
 import * as metadata from '../util/annotation-metadata';
 import memoize from '../util/memoize';
 import * as tabs from '../util/tabs';
@@ -38,7 +37,6 @@ const sortFns = {
  */
 // @ngInject
 export default function RootThread(
-  $rootScope,
   annotationsService,
   store,
   searchFilter,
@@ -98,10 +96,6 @@ export default function RootThread(
     });
   }
 
-  $rootScope.$on(events.BEFORE_ANNOTATION_CREATED, function (event, ann) {
-    annotationsService.create(ann);
-  });
-
   /**
    * Build the root conversation thread from the given UI state.
    * @return {Thread}

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

 import EventEmitter from 'tiny-emitter';
 
-import events from '../../events';
 import { Injector } from '../../../shared/injector';
 import * as annotationFixtures from '../../test/annotation-fixtures';
 import createFakeStore from '../../test/fake-redux-store';
@@ -48,10 +47,10 @@ const fixtures = {
 };
 
 describe('sidebar/services/frame-sync', function () {
+  let fakeAnnotationsService;
   let fakeStore;
   let fakeBridge;
   let frameSync;
-  let $rootScope;
 
   beforeEach(function () {
     fakeStore = createFakeStore(
@@ -72,6 +71,8 @@ describe('sidebar/services/frame-sync', function () {
       }
     );
 
+    fakeAnnotationsService = { create: sinon.stub() };
+
     const emitter = new EventEmitter();
     fakeBridge = {
       call: sinon.stub(),
@@ -92,13 +93,8 @@ describe('sidebar/services/frame-sync', function () {
       '../../shared/discovery': FakeDiscovery,
     });
 
-    $rootScope = {
-      $broadcast: sinon.stub(),
-    };
-
     frameSync = new Injector()
-      .register('$rootScope', { value: $rootScope })
-      .register('$window', { value: {} })
+      .register('annotationsService', { value: fakeAnnotationsService })
       .register('bridge', { value: fakeBridge })
       .register('store', { value: fakeStore })
       .register('frameSync', FrameSync)
@@ -214,15 +210,14 @@ describe('sidebar/services/frame-sync', function () {
 
   context('when a new annotation is created in the frame', function () {
     context('when an authenticated user is present', () => {
-      it('emits a BEFORE_ANNOTATION_CREATED event', function () {
+      it('creates the annotation in the sidebar', function () {
         fakeStore.isLoggedIn.returns(true);
         const ann = { target: [] };
 
         fakeBridge.emit('beforeCreateAnnotation', { tag: 't1', msg: ann });
 
         assert.calledWith(
-          $rootScope.$broadcast,
-          events.BEFORE_ANNOTATION_CREATED,
+          fakeAnnotationsService.create,
           sinon.match({
             $tag: 't1',
             target: [],
@@ -236,12 +231,12 @@ describe('sidebar/services/frame-sync', function () {
         fakeStore.isLoggedIn.returns(false);
       });
 
-      it('should not emit BEFORE_ANNOTATION_CREATED event', () => {
+      it('should not create an annotation in the sidebar', () => {
         const ann = { target: [] };
 
         fakeBridge.emit('beforeCreateAnnotation', { tag: 't1', msg: ann });
 
-        assert.notCalled($rootScope.$broadcast);
+        assert.notCalled(fakeAnnotationsService.create);
       });
 
       it('should open the sidebar', () => {

src/sidebar/services/test/root-thread-test.js

-import EventEmitter from 'tiny-emitter';
-
-import events from '../../events';
 import { Injector } from '../../../shared/injector';
 import * as annotationFixtures from '../../test/annotation-fixtures';
 import uiConstants from '../../ui-constants';
@@ -22,14 +19,12 @@ const fixtures = immutable({
 
 describe('rootThread', function () {
   let fakeAnnotationsService;
-  let fakeStore;
   let fakeBuildThread;
   let fakeSearchFilter;
   let fakeSettings;
+  let fakeStore;
   let fakeViewFilter;
 
-  let $rootScope;
-
   let rootThread;
 
   beforeEach(function () {
@@ -88,14 +83,7 @@ describe('rootThread', function () {
       filter: sinon.stub(),
     };
 
-    const emitter = new EventEmitter();
-    $rootScope = {
-      $on: (event, cb) => emitter.on(event, data => cb(null, data)),
-      $broadcast: (event, data) => emitter.emit(event, data),
-    };
-
     rootThread = new Injector()
-      .register('$rootScope', { value: $rootScope })
       .register('annotationsService', { value: fakeAnnotationsService })
       .register('store', { value: fakeStore })
       .register('searchFilter', { value: fakeSearchFilter })
@@ -355,44 +343,4 @@ describe('rootThread', function () {
       );
     });
   });
-
-  context('when annotation events occur', function () {
-    const annot = annotationFixtures.defaultAnnotation();
-
-    it('creates a new annotation when BEFORE_ANNOTATION_CREATED event occurs', function () {
-      $rootScope.$broadcast(events.BEFORE_ANNOTATION_CREATED, annot);
-      assert.notCalled(fakeStore.removeAnnotations);
-      assert.calledWith(fakeAnnotationsService.create, sinon.match(annot));
-    });
-
-    describe('when a new annotation is created', function () {
-      let existingNewAnnot;
-      beforeEach(function () {
-        existingNewAnnot = { $tag: 'a-new-tag' };
-        fakeStore.state.annotations.annotations.push(existingNewAnnot);
-      });
-
-      it('does not remove annotations that have non-empty drafts', function () {
-        fakeStore.getDraftIfNotEmpty.returns(fixtures.nonEmptyDraft);
-        $rootScope.$broadcast(
-          events.BEFORE_ANNOTATION_CREATED,
-          annotationFixtures.newAnnotation()
-        );
-
-        assert.notCalled(fakeStore.removeDraft);
-      });
-
-      it('does not remove saved annotations', function () {
-        const ann = annotationFixtures.defaultAnnotation();
-        fakeStore.state.annotations.annotations = [ann];
-
-        $rootScope.$broadcast(
-          events.BEFORE_ANNOTATION_CREATED,
-          annotationFixtures.newAnnotation()
-        );
-
-        assert.notCalled(fakeStore.removeDraft);
-      });
-    });
-  });
 });

src/sidebar/store/modules/drafts.js

+import { createSelector } from 'reselect';
+
 import * as metadata from '../../util/annotation-metadata';
 import * as util from '../util';
 
@@ -178,11 +180,10 @@ function getDraftIfNotEmpty(state, annotation) {
  *
  * @return {Object[]}
  */
-function unsavedAnnotations(state) {
-  return state.drafts
-    .filter(draft => !draft.annotation.id)
-    .map(draft => draft.annotation);
-}
+const unsavedAnnotations = createSelector(
+  state => state.drafts,
+  drafts => drafts.filter(d => !d.annotation.id).map(d => d.annotation)
+);
 
 export default {
   init,

src/sidebar/test/integration/threading-test.js

@@ -52,13 +52,7 @@ describe('annotation threading', function () {
       flagEnabled: sinon.stub().returns(true),
     };
 
-    const fakeRootScope = {
-      $applyAsync: sinon.stub(),
-      $on: sinon.stub(),
-    };
-
     const container = new Injector()
-      .register('$rootScope', { value: fakeRootScope })
       .register('store', storeFactory)
       .register('rootThread', rootThreadFactory)
       .register('searchFilter', searchFilterFactory)