Merge pull request #2209 from hypothesis/fix-single-annotation-load

Rectify infinite load on single-annotation, stream views

src/sidebar/components/annotation-viewer-content.js

@@ -12,13 +12,10 @@ import SidebarContentError from './sidebar-content-error';
  * The main content for the single annotation page (aka. https://hypothes.is/a/<annotation ID>)
  */
 function AnnotationViewerContent({
-  api,
+  loadAnnotationsService,
   onLogin,
   rootThread: rootThreadService,
-  streamer,
-  streamFilter,
 }) {
-  const addAnnotations = useStore(store => store.addAnnotations);
   const annotationId = useStore(store => store.routeParams().id);
   const clearAnnotations = useStore(store => store.clearAnnotations);
   const highlightAnnotations = useStore(store => store.highlightAnnotations);
@@ -34,13 +31,12 @@ function AnnotationViewerContent({
     setFetchError(false);
     clearAnnotations();
 
-    fetchThread(api, annotationId)
+    loadAnnotationsService
+      .loadThread(annotationId)
       .then(annots => {
-        addAnnotations(annots);
-
         // Find the top-level annotation in the thread that `annotationId` is
         // part of. This will be different to `annotationId` if `annotationId`
-        // is a reply.
+        // is a reply. A top-level annotation will not have any references.
         const topLevelAnnot = annots.filter(
           ann => (ann.references || []).length === 0
         )[0];
@@ -59,14 +55,6 @@ function AnnotationViewerContent({
           return;
         }
 
-        // Configure the connection to the real-time update service to send us
-        // updates to any of the annotations in the thread.
-        streamFilter
-          .addClause('/references', 'one_of', topLevelAnnot.id, true)
-          .addClause('/id', 'equals', topLevelAnnot.id, true);
-        streamer.setConfig('filter', { filter: streamFilter.getFilter() });
-        streamer.connect();
-
         // Make the full thread of annotations visible. By default replies are
         // not shown until the user expands the thread.
         annots.forEach(annot => setCollapsed(annot.id, false));
@@ -88,13 +76,10 @@ function AnnotationViewerContent({
     userid,
 
     // Static dependencies.
-    addAnnotations,
-    api,
     clearAnnotations,
     highlightAnnotations,
+    loadAnnotationsService,
     setCollapsed,
-    streamFilter,
-    streamer,
   ]);
 
   return (
@@ -114,44 +99,13 @@ AnnotationViewerContent.propTypes = {
   onLogin: propTypes.func.isRequired,
 
   // Injected.
-  api: propTypes.object,
+  loadAnnotationsService: propTypes.object,
   rootThread: propTypes.object,
-  streamer: propTypes.object,
-  streamFilter: propTypes.object,
 };
 
 AnnotationViewerContent.injectedProps = [
-  'api',
+  'loadAnnotationsService',
   'rootThread',
-  'streamer',
-  'streamFilter',
 ];
 
-// NOTE: The function below is intentionally at the bottom of the file.
-//
-// Putting it at the top resulted in an issue where the `createElement` import
-// wasn't correctly referenced in the body of `AnnotationViewerContent` in
-// the compiled JS, causing a runtime error.
-
-/**
- * Fetch all annotations in the same thread as `id`.
- *
- * @param {Object} api - API client
- * @param {string} id - Annotation ID. This may be an annotation or a reply.
- * @return Promise<Annotation[]> - The annotation, followed by any replies.
- */
-async function fetchThread(api, id) {
-  let annot = await api.annotation.get({ id });
-
-  if (annot.references && annot.references.length) {
-    // This is a reply, fetch the top-level annotation
-    annot = await api.annotation.get({ id: annot.references[0] });
-  }
-
-  // Fetch all replies to the top-level annotation.
-  const replySearchResult = await api.search({ references: annot.id });
-
-  return [annot, ...replySearchResult.rows];
-}
-
 export default withServices(AnnotationViewerContent);

src/sidebar/components/stream-content.js

@@ -17,6 +17,12 @@ function StreamContent({
   toastMessenger,
 }) {
   const addAnnotations = useStore(store => store.addAnnotations);
+  const annotationFetchStarted = useStore(
+    store => store.annotationFetchStarted
+  );
+  const annotationFetchFinished = useStore(
+    store => store.annotationFetchFinished
+  );
   const clearAnnotations = useStore(store => store.clearAnnotations);
   const currentQuery = useStore(store => store.routeParams().q);
   const setSortKey = useStore(store => store.setSortKey);
@@ -38,10 +44,21 @@ function StreamContent({
 
         ...searchFilter.toObject(query),
       };
+      try {
+        annotationFetchStarted();
         const results = await api.search(queryParams);
         addAnnotations([...results.rows, ...results.replies]);
+      } finally {
+        annotationFetchFinished();
+      }
     },
-    [addAnnotations, api, searchFilter]
+    [
+      addAnnotations,
+      annotationFetchStarted,
+      annotationFetchFinished,
+      api,
+      searchFilter,
+    ]
   );
 
   // Update the stream when this route is initially displayed and whenever

src/sidebar/components/test/annotation-viewer-content-test.js

@@ -8,38 +8,14 @@ import AnnotationViewerContent, {
   $imports,
 } from '../annotation-viewer-content';
 
-/**
- * Fake implementation of the `api` service.
- */
-class FakeApi {
-  constructor(annots) {
-    this.annotations = annots;
-
-    this.annotation = {
-      get: async query => this.annotations.find(a => a.id === query.id),
-    };
-  }
-
-  async search(query) {
-    let matches = [];
-    if (query.references) {
-      matches = this.annotations.filter(
-        a => a.references && a.references.includes(query.references)
-      );
-    }
-    return { rows: matches };
-  }
-}
-
 describe('AnnotationViewerContent', () => {
   let fakeStore;
+  let fakeOnLogin;
   let fakeRootThread;
-  let fakeStreamer;
-  let fakeStreamFilter;
+  let fakeLoadAnnotationsService;
 
   beforeEach(() => {
     fakeStore = {
-      addAnnotations: sinon.stub(),
       clearAnnotations: sinon.stub(),
       getState: sinon.stub().returns({}),
       highlightAnnotations: sinon.stub(),
@@ -48,21 +24,13 @@ describe('AnnotationViewerContent', () => {
       setCollapsed: sinon.stub(),
     };
 
-    fakeRootThread = { thread: sinon.stub().returns({}) };
-
-    fakeStreamer = {
-      setConfig: () => {},
-      connect: () => {},
+    fakeLoadAnnotationsService = {
+      loadThread: sinon.stub().resolves([]),
     };
 
-    fakeStreamFilter = {
-      addClause: () => {
-        return {
-          addClause: () => {},
-        };
-      },
-      getFilter: () => {},
-    };
+    fakeOnLogin = sinon.stub();
+
+    fakeRootThread = { thread: sinon.stub().returns({}) };
 
     $imports.$mock(mockImportedComponents());
     $imports.$mock({
@@ -74,55 +42,54 @@ describe('AnnotationViewerContent', () => {
     $imports.$restore();
   });
 
-  function createComponent({ api, onLogin = sinon.stub() }) {
+  function createComponent(props = {}) {
     return mount(
       <AnnotationViewerContent
-        api={api}
-        onLogin={onLogin}
+        loadAnnotationsService={fakeLoadAnnotationsService}
+        onLogin={fakeOnLogin}
         rootThread={fakeRootThread}
-        streamer={fakeStreamer}
-        streamFilter={fakeStreamFilter}
+        {...props}
       />
     );
   }
 
-  function waitForAnnotationsToLoad() {
-    return waitFor(() => fakeStore.addAnnotations.called);
-  }
-
   describe('the standalone view for a top-level annotation', () => {
-    it('loads the annotation and all replies', async () => {
-      const fakeApi = new FakeApi([
-        { id: 'test_annotation_id' },
-        { id: 'test_reply_id', references: ['test_annotation_id'] },
-      ]);
-      createComponent({ api: fakeApi });
+    it('loads the annotation thread', () => {
+      createComponent();
 
-      await waitForAnnotationsToLoad();
-
-      assert.calledOnce(fakeStore.addAnnotations);
-      assert.calledWith(
-        fakeStore.addAnnotations,
-        sinon.match(fakeApi.annotations)
-      );
+      assert.calledOnce(fakeLoadAnnotationsService.loadThread);
     });
 
-    it('does not highlight any annotations', async () => {
-      const fakeApi = new FakeApi([
+    context('successfully-loaded annotation thread', () => {
+      beforeEach(() => {
+        fakeLoadAnnotationsService.loadThread.resolves([
           { id: 'test_annotation_id' },
           { id: 'test_reply_id', references: ['test_annotation_id'] },
         ]);
-      createComponent({ api: fakeApi });
+      });
 
-      await waitForAnnotationsToLoad();
+      it('does not highlight any annotations', async () => {
+        createComponent();
+
+        await new Promise(resolve => setTimeout(resolve, 0));
 
         assert.notCalled(fakeStore.highlightAnnotations);
       });
 
+      it('expands the thread', async () => {
+        createComponent();
+
+        await new Promise(resolve => setTimeout(resolve, 0));
+
+        assert.calledWith(fakeStore.setCollapsed, 'test_annotation_id', false);
+        assert.calledWith(fakeStore.setCollapsed, 'test_reply_id', false);
+      });
+    });
+
     it('shows an error if the annotation could not be fetched', async () => {
-      const fakeApi = new FakeApi([]);
+      fakeLoadAnnotationsService.loadThread.rejects();
       const onLogin = sinon.stub();
-      const wrapper = createComponent({ api: fakeApi, onLogin });
+      const wrapper = createComponent({ onLogin });
 
       // Initially the annotation is not available to the user, so an error
       // should be shown.
@@ -138,57 +105,28 @@ describe('AnnotationViewerContent', () => {
       onLoginRequest();
       assert.called(onLogin);
 
-      // After the user logs in, the annotation should be shown.
-      fakeApi.annotations = [{ id: 'test_annotation_id' }];
+      fakeLoadAnnotationsService.loadThread.resetHistory();
+      fakeLoadAnnotationsService.loadThread.resolves([
+        { id: 'test_annotation_id' },
+      ]);
       fakeStore.profile.returns({ userid: 'acct:jimsmith@hypothes.is' });
 
       // Force re-render. `useStore` would do this in the actual app.
       wrapper.setProps({});
 
-      await waitForAnnotationsToLoad();
+      await waitFor(() => fakeLoadAnnotationsService.loadThread.called);
       assert.isFalse(wrapper.exists('SidebarContentError'));
     });
-  });
-
-  describe('the standalone view for a reply', () => {
-    it('loads the top-level annotation and all replies', async () => {
-      const fakeApi = new FakeApi([
-        { id: 'parent_id' },
-        { id: 'test_annotation_id', references: ['parent_id'] },
-      ]);
-      createComponent({ api: fakeApi });
-
-      await waitForAnnotationsToLoad();
-
-      assert.calledWith(
-        fakeStore.addAnnotations,
-        sinon.match(fakeApi.annotations)
-      );
-    });
-
-    it('expands the thread', async () => {
-      const fakeApi = new FakeApi([
-        { id: 'parent_id' },
-        { id: 'test_annotation_id', references: ['parent_id'] },
-      ]);
-
-      createComponent({ api: fakeApi });
-
-      await waitForAnnotationsToLoad();
-
-      assert.calledWith(fakeStore.setCollapsed, 'parent_id', false);
-      assert.calledWith(fakeStore.setCollapsed, 'test_annotation_id', false);
-    });
 
-    it('highlights the reply', async () => {
-      const fakeApi = new FakeApi([
+    it('highlights the annotation if it is a reply', async () => {
+      fakeLoadAnnotationsService.loadThread.resolves([
         { id: 'parent_id' },
         { id: 'test_annotation_id', references: ['parent_id'] },
       ]);
-      createComponent({ api: fakeApi });
 
-      await waitForAnnotationsToLoad();
+      createComponent();
 
+      await new Promise(resolve => setTimeout(resolve, 0));
       assert.calledWith(
         fakeStore.highlightAnnotations,
         sinon.match(['test_annotation_id'])

src/sidebar/components/test/stream-content-test.js

@@ -28,6 +28,8 @@ describe('StreamContent', () => {
 
     fakeStore = {
       addAnnotations: sinon.stub(),
+      annotationFetchStarted: sinon.stub(),
+      annotationFetchFinished: sinon.stub(),
       clearAnnotations: sinon.spy(),
       getState: sinon.stub().returns({}),
       routeParams: sinon.stub().returns({ id: 'test' }),
@@ -69,6 +71,21 @@ describe('StreamContent', () => {
     assert.equal(fakeApi.search.firstCall.args[0]._separate_replies, true);
   });
 
+  it('records the start and finish of the fetch request in the store', async () => {
+    createComponent();
+
+    await waitFor(() => fakeStore.annotationFetchStarted.calledOnce);
+    await waitFor(() => fakeStore.annotationFetchFinished.calledOnce);
+  });
+
+  it('records the finish of the fetch request on error', async () => {
+    fakeApi.search.throws();
+    createComponent();
+
+    await waitFor(() => fakeStore.annotationFetchStarted.calledOnce);
+    await waitFor(() => fakeStore.annotationFetchFinished.calledOnce);
+  });
+
   it('loads the annotations and replies into the store', async () => {
     fakeApi.search.resolves({
       rows: ['annotation_1', 'annotation_2'],

src/sidebar/services/load-annotations.js

@@ -3,6 +3,8 @@
  */
 import SearchClient from '../search-client';
 
+import { isReply } from '../util/annotation-metadata';
+
 // @ngInject
 export default function loadAnnotationsService(
   api,
@@ -61,7 +63,56 @@ export default function loadAnnotationsService(
     searchClient.get({ uri: uris, group: groupId });
   }
 
+  /**
+   * Fetch all annotations in the same thread as `id` and add them to the store.
+   *
+   * @param {string} id - Annotation ID. This may be an annotation or a reply.
+   * @return Promise<Annotation[]> - The annotation, followed by any replies.
+   */
+  async function loadThread(id) {
+    let annotation;
+    let replySearchResult;
+
+    // Clear out any annotations already in the store before fetching new ones
+    store.clearAnnotations();
+
+    try {
+      store.annotationFetchStarted();
+      // 1. Fetch the annotation indicated by `id` — the target annotation
+      annotation = await api.annotation.get({ id });
+
+      // 2. If annotation is not the top-level annotation in its thread,
+      //    fetch the top-level annotation
+      if (isReply(annotation)) {
+        annotation = await api.annotation.get({ id: annotation.references[0] });
+      }
+
+      // 3. Fetch all of the annotations in the thread, based on the
+      //    top-level annotation
+      replySearchResult = await api.search({ references: annotation.id });
+    } finally {
+      store.annotationFetchFinished();
+    }
+    const threadAnnotations = [annotation, ...replySearchResult.rows];
+
+    store.addAnnotations(threadAnnotations);
+
+    // If we've been successful in retrieving a thread, with a top-level annotation,
+    // configure the connection to the real-time update service to send us
+    // updates to any of the annotations in the thread.
+    if (!isReply(annotation)) {
+      streamFilter
+        .addClause('/references', 'one_of', annotation.id, true)
+        .addClause('/id', 'equals', annotation.id, true);
+      streamer.setConfig('filter', { filter: streamFilter.getFilter() });
+      streamer.connect();
+    }
+
+    return threadAnnotations;
+  }
+
   return {
     load,
+    loadThread,
   };
 }

src/sidebar/services/test/load-annotations-test.js

@@ -43,13 +43,17 @@ describe('loadAnnotationsService', () => {
     longRunningSearchClient = false;
 
     fakeApi = {
-      search: sinon.stub(),
+      search: sinon.stub().returns({ rows: [] }),
+      annotation: {
+        get: sinon.stub(),
+      },
     };
 
     fakeStore = {
       addAnnotations: sinon.stub(),
       annotationFetchFinished: sinon.stub(),
       annotationFetchStarted: sinon.stub(),
+      clearAnnotations: sinon.stub(),
       frames: sinon.stub(),
       removeAnnotations: sinon.stub(),
       savedAnnotations: sinon.stub(),
@@ -62,6 +66,9 @@ describe('loadAnnotationsService', () => {
       reconnect: sinon.stub(),
     };
     fakeStreamFilter = {
+      addClause: sinon.stub().returns({
+        addClause: sinon.stub(),
+      }),
       resetFilter: sinon.stub().returns({
         addClause: sinon.stub(),
       }),
@@ -257,4 +264,187 @@ describe('loadAnnotationsService', () => {
       assert.calledWith(console.error, error);
     });
   });
+
+  describe('loadThread', () => {
+    let threadAnnotations = [
+      { id: 'parent_annotation_1' },
+      { id: 'parent_annotation_2', references: ['parent_annotation_1'] },
+      {
+        id: 'target_annotation',
+        references: ['parent_annotation_1', 'parent_annotation_2'],
+      },
+    ];
+    it('clears annotations from the store first', () => {
+      const svc = createService();
+      svc.loadThread('target_annotation');
+      assert.calledOnce(fakeStore.clearAnnotations);
+    });
+
+    describe('fetching the target annotation', () => {
+      beforeEach(() => {
+        fakeApi.annotation.get.onFirstCall().resolves({
+          id: 'target_annotation',
+          references: [],
+        });
+      });
+
+      it('fetches annotation with given `id`', async () => {
+        const svc = createService();
+        await svc.loadThread('target_annotation');
+
+        assert.calledWith(
+          fakeApi.annotation.get,
+          sinon.match({ id: 'target_annotation' })
+        );
+      });
+
+      it('records the start and end of annotation fetch with the store', async () => {
+        const svc = createService();
+        await svc.loadThread('target_annotation');
+
+        assert.calledOnce(fakeStore.annotationFetchStarted);
+        assert.calledOnce(fakeStore.annotationFetchFinished);
+      });
+
+      it('stops the annotation fetch with the store on error', async () => {
+        fakeApi.annotation.get.onFirstCall().throws();
+
+        const svc = createService();
+        try {
+          await svc.loadThread('target_annotation');
+        } catch (e) {
+          assert.calledOnce(fakeStore.annotationFetchStarted);
+          assert.calledOnce(fakeStore.annotationFetchFinished);
+        }
+      });
+    });
+
+    describe('fetching top-level annotation in thread', () => {
+      beforeEach(() => {
+        fakeApi.annotation.get.onFirstCall().resolves({
+          id: 'target_annotation',
+          references: ['parent_annotation_1', 'parent_annotation_2'],
+        });
+        fakeApi.annotation.get.onSecondCall().resolves({
+          id: 'parent_annotation_1',
+          references: [],
+        });
+      });
+
+      it('fetches top-level annotation', async () => {
+        const svc = createService();
+        await svc.loadThread('target_annotation');
+
+        assert.calledWith(
+          fakeApi.annotation.get,
+          sinon.match({ id: 'parent_annotation_1' })
+        );
+      });
+    });
+
+    describe('fetching other annotations in the thread', () => {
+      beforeEach(() => {
+        fakeApi.annotation.get.onFirstCall().resolves({
+          id: 'target_annotation',
+          references: ['parent_annotation_1', 'parent_annotation_2'],
+        });
+        fakeApi.annotation.get.onSecondCall().resolves({
+          id: 'parent_annotation_1',
+          references: [],
+        });
+        fakeApi.search.resolves({
+          rows: [threadAnnotations[1], threadAnnotations[2]],
+        });
+      });
+
+      it('retrieves all annotations in the thread', async () => {
+        const svc = createService();
+        await svc.loadThread('target_annotation');
+
+        assert.calledWith(
+          fakeApi.search,
+          sinon.match({ references: 'parent_annotation_1' })
+        );
+      });
+
+      it('adds all of the annotations in the thread to the store', async () => {
+        const svc = createService();
+        await svc.loadThread('target_annotation');
+
+        assert.calledWith(fakeStore.addAnnotations, sinon.match.array);
+      });
+
+      it('returns thread annotations', async () => {
+        const svc = createService();
+        const annots = await svc.loadThread('target_annotation');
+
+        assert.equal(annots[0].id, 'parent_annotation_1');
+        assert.equal(annots[1].id, 'parent_annotation_2');
+        assert.equal(annots[2].id, 'target_annotation');
+      });
+    });
+
+    describe('connecting to streamer for thread updates', () => {
+      beforeEach(() => {
+        fakeApi.annotation.get.onFirstCall().resolves({
+          id: 'target_annotation',
+          references: ['parent_annotation_1', 'parent_annotation_2'],
+        });
+        fakeApi.annotation.get.onSecondCall().resolves({
+          id: 'parent_annotation_1',
+          references: [],
+        });
+        fakeApi.search.resolves({
+          rows: [threadAnnotations[1], threadAnnotations[2]],
+        });
+      });
+
+      it('does not connect to the streamer if no top-level annotation available', async () => {
+        // Make it so the "top-level" annotation isn't really top level: it has references
+        // and so is a reply
+        fakeApi.annotation.get.onSecondCall().resolves({
+          id: 'parent_annotation_1',
+          references: ['something_else'],
+        });
+
+        const svc = createService();
+        svc.loadThread('target_annotation');
+
+        await new Promise(resolve => setTimeout(resolve, 0));
+
+        assert.notCalled(fakeStreamer.connect);
+      });
+
+      it('configures the stream filter for changes to the thread', async () => {
+        const fakeAddClause = sinon.stub();
+        fakeStreamFilter.addClause.returns({ addClause: fakeAddClause });
+        fakeStreamFilter.getFilter.returns('filter');
+        const svc = createService();
+        svc.loadThread('target_annotation');
+
+        await new Promise(resolve => setTimeout(resolve, 0));
+
+        assert.calledWith(
+          fakeStreamFilter.addClause,
+          '/references',
+          'one_of',
+          'parent_annotation_1',
+          true
+        );
+        assert.calledWith(
+          fakeAddClause,
+          '/id',
+          'equals',
+          'parent_annotation_1',
+          true
+        );
+        assert.calledWith(
+          fakeStreamer.setConfig,
+          'filter',
+          sinon.match({ filter: 'filter' })
+        );
+        assert.calledOnce(fakeStreamer.connect);
+      });
+    });
+  });
 });