Replace the `BEFORE_ANNOTATION_CREATED` event

The `BEFORE_ANNOTATION_CREATED` event was used for two purposes: 1. Invoke `annotationsService.create(...)` to create the annotation in the sidebar. 2. Scroll the newly created annotation into view Resolve (1) by calling `annotationsService.create` directly from the `frameSync` service and resolve (2) by watching for changes to `unsavedAnnotations()` in the store and scrolling to the new annotation when it appears. There is one functional change which is that replies will be scrolled into view, as this seems like a useful behavior if the user clicks the "Reply" button for a thread and the input field appears below the bottom of the screen.

Replace the `BEFORE_ANNOTATION_CREATED` event
The `BEFORE_ANNOTATION_CREATED` event was used for two purposes: 1. Invoke `annotationsService.create(...)` to create the annotation in the sidebar. 2. Scroll the newly created annotation into view Resolve (1) by calling `annotationsService.create` directly from the `frameSync` service and resolve (2) by watching for changes to `unsavedAnnotations()` in the store and scrolling to the new annotation when it appears. There is one functional change which is that replies will be scrolled into view, as this seems like a useful behavior if the user clicks the "Reply" button for a thread and the input field appears below the bottom of the screen.
75a0e8aa · Robert Knight · 3ba68201 · 75a0e8aa · 75a0e8aa · 75a0e8aa
Commit 75a0e8aa authored May 05, 2020 by Robert Knight
9 changed files
--- a/src/sidebar/components/test/thread-list-test.js
+++ b/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(
+    const wrapper = mount(<ThreadList thread={fakeTopThread} {...props} />, {
-      <ThreadList
+      attachTo: fakeScrollContainer,
-        thread={fakeTopThread}
+    });
-        $rootScope={fakeRootScope}
-        {...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', () => {
+   * Simulate what happens when a new draft annotation is created in the
-      fakeRootScope.$on = sinon.stub();
+   * application.
+   */
-      createComponent();
+  const addNewAnnotation = (wrapper, annotation = { $tag: 'foobar' }) => {
+    fakeStore.unsavedAnnotations.returns([annotation]);
-      assert.calledWith(
+    wrapper.setProps({});
-        fakeRootScope.$on,
+  };
-        events.BEFORE_ANNOTATION_CREATED,
-        sinon.match.func
-      );
-    });
+  context('new annotation created in application', () => {
    it('clears the current selection in the store', () => {
-      createComponent();
+      const wrapper = createComponent();
+      addNewAnnotation(wrapper);
-      fakeRootScope.eventCallbacks[events.BEFORE_ANNOTATION_CREATED]({}, {});
      assert.calledOnce(fakeStore.clearSelection);
    });
    it('does not clear the selection in the store if new annotation is a highlight', () => {
      fakeMetadata.isHighlight.returns(true);
-      createComponent();
+      const wrapper = 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();
-      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(() => {
+      addNewAnnotation(wrapper);
-        fakeRootScope.eventCallbacks[events.BEFORE_ANNOTATION_CREATED](
-          {},
-          { $tag: 'nonexistent' }
-        );
-      });
      assert.notCalled(fakeScrollTop);
    });
    it('should set the scroll container `scrollTop` to derived position of thread', () => {
-      createComponent();
+      const wrapper = createComponent();
-      act(() => {
+      addNewAnnotation(wrapper, fakeTopThread.children[3].annotation);
-        fakeRootScope.eventCallbacks[events.BEFORE_ANNOTATION_CREATED](
-          {},
-          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-

--- a/src/sidebar/components/thread-list.js
+++ b/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,23 @@ function ThreadList({ thread, $rootScope }) {
    [topLevelThreads, threadHeights, scrollPosition, scrollContainerHeight]
  );
-  // Listen for when a new annotation is created in the application, and trigger
+  // Scroll to newly created annotations and replies.
-  // a scroll to that annotation's thread card (unless highlight or reply)
+  const newAnnotationTag = useStore(store => {
+    const ann = store
+      .unsavedAnnotations()
+      .find(ann => !ann.id && !isHighlight(ann));
+    return ann ? ann.$tag : null;
+  });
  useEffect(() => {
-    const removeListener = $rootScope.$on(
+    if (newAnnotationTag) {
-      events.BEFORE_ANNOTATION_CREATED,
-      (event, annotation) => {
-        if (!isHighlight(annotation) && !isReply(annotation)) {
      clearSelection();
-          setScrollToId(annotation.$tag);
+      setScrollToId(newAnnotationTag);
-        }
    }
-    );
+  }, [clearSelection, newAnnotationTag]);
-    return removeListener;
-  }, [$rootScope, clearSelection]);
  // Effect to scroll a particular thread into view. This is mainly used to
-  // scroll a newly created annotation into view (as triggered by the
+  // scroll a newly created annotation into view.
-  // listener for `BEFORE_ANNOTATION_CREATED`)
  useEffect(() => {
    if (!scrollToId) {
      return;
@@ -97,12 +94,8 @@ function ThreadList({ thread, $rootScope }) {
    const threadIndex = topLevelThreads.findIndex(t => t.id === scrollToId);
    if (threadIndex === -1) {
-      // Thread is not currently present.
+      // Thread is not currently present. The `scrollToId` will be consumed
-      //
+      // when this thread appears.
-      // 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.
      return;
    }
@@ -189,11 +182,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 ThreadList;
-export default withServices(ThreadList);
--- a/src/sidebar/events.js
+++ b/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',
 };
--- a/src/sidebar/services/frame-sync.js
+++ b/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));

--- a/src/sidebar/services/root-thread.js
+++ b/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}

--- a/src/sidebar/services/test/frame-sync-test.js
+++ b/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('annotationsService', { value: fakeAnnotationsService })
-      .register('$window', { value: {} })
      .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,
+          fakeAnnotationsService.create,
-          events.BEFORE_ANNOTATION_CREATED,
          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', () => {

--- a/src/sidebar/services/test/root-thread-test.js
+++ b/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);
-      });
-    });
-  });
 });
--- a/src/sidebar/store/modules/drafts.js
+++ b/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) {
+const unsavedAnnotations = createSelector(
-  return state.drafts
+  state => state.drafts,
-    .filter(draft => !draft.annotation.id)
+  drafts => drafts.filter(d => !d.annotation.id).map(d => d.annotation)
-    .map(draft => draft.annotation);
+);
-}
 export default {
  init,

--- a/src/sidebar/test/integration/threading-test.js
+++ b/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)