Merge pull request #2183 from hypothesis/single-annotation-handle-404

Improve handling of annotation fetch error in single annotation view

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

-import { createElement } from 'preact';
-import { useEffect } from 'preact/hooks';
+import { Fragment, createElement } from 'preact';
+import { useEffect, useState } from 'preact/hooks';
 import propTypes from 'prop-types';
 
 import useStore from '../store/use-store';
 import { withServices } from '../util/service-context';
 
 import ThreadList from './thread-list';
+import SidebarContentError from './sidebar-content-error';
 
 /**
  * The main content for the single annotation page (aka. https://hypothes.is/a/<annotation ID>)
  */
 function AnnotationViewerContent({
   api,
+  onLogin,
   rootThread: rootThreadService,
   streamer,
   streamFilter,
@@ -24,56 +26,67 @@ function AnnotationViewerContent({
     rootThreadService.thread(store.getState())
   );
   const setCollapsed = useStore(store => store.setCollapsed);
+  const userid = useStore(store => store.profile().userid);
+
+  const [fetchError, setFetchError] = useState(false);
 
   useEffect(() => {
+    setFetchError(false);
     clearAnnotations();
 
-    // TODO - Handle exceptions during the `fetchThread` call.
-    fetchThread(api, 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.
-      const topLevelAnnot = annots.filter(
-        ann => (ann.references || []).length === 0
-      )[0];
-
-      if (!topLevelAnnot) {
-        // We were able to fetch annotations in the thread that `annotationId`
-        // is part of (note that `annotationId` may refer to a reply) but
-        // couldn't find a top-level (non-reply) annotation in that thread.
-        //
-        // This might happen if the top-level annotation was deleted or
-        // moderated or had its permissions changed.
-        //
-        // We need to decide what what be the most useful behavior in this case
-        // and implement it.
-        /* istanbul ignore next */
-        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));
-
-      // FIXME - This should show a visual indication of which reply the
-      // annotation ID in the URL refers to. That isn't currently working.
-      if (topLevelAnnot.id !== annotationId) {
-        highlightAnnotations([annotationId]);
-      }
-    });
+    fetchThread(api, 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.
+        const topLevelAnnot = annots.filter(
+          ann => (ann.references || []).length === 0
+        )[0];
+
+        if (!topLevelAnnot) {
+          // We were able to fetch annotations in the thread that `annotationId`
+          // is part of (note that `annotationId` may refer to a reply) but
+          // couldn't find a top-level (non-reply) annotation in that thread.
+          //
+          // This might happen if the top-level annotation was deleted or
+          // moderated or had its permissions changed.
+          //
+          // We need to decide what what be the most useful behavior in this case
+          // and implement it.
+          /* istanbul ignore next */
+          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));
+
+        // FIXME - This should show a visual indication of which reply the
+        // annotation ID in the URL refers to. That isn't currently working.
+        if (topLevelAnnot.id !== annotationId) {
+          highlightAnnotations([annotationId]);
+        }
+      })
+      .catch(() => {
+        setFetchError(true);
+      });
   }, [
     annotationId,
 
+    // This is not used by the effect but ensures that the annotation is
+    // fetched after the user logs in/out, in case the annotation is private.
+    userid,
+
     // Static dependencies.
     addAnnotations,
     api,
@@ -84,10 +97,22 @@ function AnnotationViewerContent({
     streamer,
   ]);
 
-  return <ThreadList thread={rootThread} />;
+  return (
+    <Fragment>
+      {fetchError && (
+        // This is the same error shown if a direct-linked annotation cannot
+        // be fetched in the sidebar. Fortunately the error message makes sense
+        // for this scenario as well.
+        <SidebarContentError errorType="annotation" onLoginRequest={onLogin} />
+      )}
+      <ThreadList thread={rootThread} />
+    </Fragment>
+  );
 }
 
 AnnotationViewerContent.propTypes = {
+  onLogin: propTypes.func.isRequired,
+
   // Injected.
   api: propTypes.object,
   rootThread: propTypes.object,

src/sidebar/components/hypothesis-app.js

@@ -168,7 +168,9 @@ function HypothesisApp({
 
         {route && (
           <main>
-            {route === 'annotation' && <AnnotationViewerContent />}
+            {route === 'annotation' && (
+              <AnnotationViewerContent onLogin={login} />
+            )}
             {route === 'stream' && <StreamContent />}
             {route === 'sidebar' && (
               <SidebarContent onLogin={login} onSignUp={signUp} />

src/sidebar/components/sidebar-content-error.js

@@ -7,9 +7,14 @@ import Button from './button';
 import SvgIcon from '../../shared/components/svg-icon';
 
 /**
- * An error message to display in the sidebar.
+ * Show an error indicating that an annotation or group referenced in the URL
+ * could not be fetched.
  */
-export default function SidebarContentError({ errorType, onLoginRequest }) {
+export default function SidebarContentError({
+  errorType,
+  onLoginRequest,
+  showClearSelection = false,
+}) {
   const clearSelection = useStore(store => store.clearSelection);
   const isLoggedIn = useStore(store => store.isLoggedIn());
 
@@ -42,12 +47,14 @@ export default function SidebarContentError({ errorType, onLoginRequest }) {
       <div className="sidebar-content-error__content">
         <p>{errorMessage}</p>
         <div className="sidebar-content-error__actions">
-          <Button
-            buttonText="Show all annotations"
-            className="sidebar-content-error__button"
-            onClick={clearSelection}
-            usePrimaryStyle={isLoggedIn}
-          />
+          {showClearSelection && (
+            <Button
+              buttonText="Show all annotations"
+              className="sidebar-content-error__button"
+              onClick={clearSelection}
+              usePrimaryStyle={isLoggedIn}
+            />
+          )}
           {!isLoggedIn && (
             <Button
               buttonText="Log in"
@@ -64,6 +71,12 @@ export default function SidebarContentError({ errorType, onLoginRequest }) {
 
 SidebarContentError.propTypes = {
   errorType: propTypes.oneOf(['annotation', 'group']),
+
+  /**
+   * Whether to show a "Clear selection" button.
+   */
+  showClearSelection: propTypes.bool,
+
   /* A function that will launch the login flow for the user. */
   onLoginRequest: propTypes.func.isRequired,
 };

src/sidebar/components/sidebar-content.js

@@ -131,7 +131,11 @@ function SidebarContent({
       {isFocusedMode && <FocusedModeHeader />}
       <LoginPromptPanel onLogin={onLogin} onSignUp={onSignUp} />
       {hasDirectLinkedAnnotationError && (
-        <SidebarContentError errorType="annotation" onLoginRequest={onLogin} />
+        <SidebarContentError
+          errorType="annotation"
+          onLoginRequest={onLogin}
+          showClearSelection={true}
+        />
       )}
       {hasDirectLinkedGroupError && (
         <SidebarContentError errorType="group" onLoginRequest={onLogin} />

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

@@ -44,6 +44,7 @@ describe('AnnotationViewerContent', () => {
       getState: sinon.stub().returns({}),
       highlightAnnotations: sinon.stub(),
       routeParams: sinon.stub().returns({ id: 'test_annotation_id' }),
+      profile: sinon.stub().returns({ userid: null }),
       setCollapsed: sinon.stub(),
     };
 
@@ -73,10 +74,11 @@ describe('AnnotationViewerContent', () => {
     $imports.$restore();
   });
 
-  function createComponent({ api }) {
+  function createComponent({ api, onLogin = sinon.stub() }) {
     return mount(
       <AnnotationViewerContent
         api={api}
+        onLogin={onLogin}
         rootThread={fakeRootThread}
         streamer={fakeStreamer}
         streamFilter={fakeStreamFilter}
@@ -116,6 +118,36 @@ describe('AnnotationViewerContent', () => {
 
       assert.notCalled(fakeStore.highlightAnnotations);
     });
+
+    it('shows an error if the annotation could not be fetched', async () => {
+      const fakeApi = new FakeApi([]);
+      const onLogin = sinon.stub();
+      const wrapper = createComponent({ api: fakeApi, onLogin });
+
+      // Initially the annotation is not available to the user, so an error
+      // should be shown.
+      await waitFor(() => {
+        wrapper.update();
+        return wrapper.exists('SidebarContentError');
+      });
+
+      // Simulate clicking the "Login" button in the error.
+      const onLoginRequest = wrapper
+        .find('SidebarContentError')
+        .prop('onLoginRequest');
+      onLoginRequest();
+      assert.called(onLogin);
+
+      // After the user logs in, the annotation should be shown.
+      fakeApi.annotations = [{ 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();
+      assert.isFalse(wrapper.exists('SidebarContentError'));
+    });
   });
 
   describe('the standalone view for a reply', () => {

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

@@ -47,7 +47,10 @@ describe('SidebarContentError', () => {
 
   it('should provide a button to clear the selection (show all annotations)', () => {
     const fakeOnLogin = sinon.stub();
-    const wrapper = createComponent({ onLoginRequest: fakeOnLogin });
+    const wrapper = createComponent({
+      onLoginRequest: fakeOnLogin,
+      showClearSelection: true,
+    });
 
     const clearButton = findButtonByText(wrapper, 'Show all annotations');
     assert.isTrue(clearButton.exists());