Refactor reply-toggling logic for top-level threads

Refactor the way headless threads are handled regarding
visibility and reply toggling. Add an `AnnotationMissing`
component; harmonize props between `Annotation` and
`AnnotationMissing`.

Add a new `AnnotationReplyToggle` component.

src/sidebar/components/Annotation.js

@@ -3,7 +3,7 @@ import { createElement } from 'preact';
 import propTypes from 'prop-types';
 
 import { useStoreProxy } from '../store/use-store';
-import { isReply, quote } from '../helpers/annotation-metadata';
+import { quote } from '../helpers/annotation-metadata';
 import { withServices } from '../service-context';
 
 import AnnotationActionBar from './AnnotationActionBar';
@@ -11,7 +11,7 @@ import AnnotationBody from './AnnotationBody';
 import AnnotationEditor from './AnnotationEditor';
 import AnnotationHeader from './AnnotationHeader';
 import AnnotationQuote from './AnnotationQuote';
-import Button from './Button';
+import AnnotationReplyToggle from './AnnotationReplyToggle';
 
 /**
  * @typedef {import("../../types/api").Annotation} Annotation
@@ -21,7 +21,10 @@ import Button from './Button';
 /**
  * @typedef AnnotationProps
  * @prop {Annotation} annotation
- * @prop {number} replyCount - Number of replies to this annotation (thread)
+ * @prop {boolean} hasAppliedFilter - Is any filter applied currently?
+ * @prop {boolean} isReply
+ * @prop {VoidFunction} onToggleReplies - Callback to expand/collapse reply threads
+ * @prop {number} replyCount - Number of replies to this annotation's thread
  * @prop {boolean} showDocumentInfo - Should extended document info be rendered (e.g. in non-sidebar contexts)?
  * @prop {boolean} threadIsCollapsed - Is the thread to which this annotation belongs currently collapsed?
  * @prop {Object} annotationsService - Injected service
@@ -34,10 +37,13 @@ import Button from './Button';
  */
 function Annotation({
   annotation,
-  annotationsService,
+  hasAppliedFilter,
+  isReply,
+  onToggleReplies,
   replyCount,
   showDocumentInfo,
   threadIsCollapsed,
+  annotationsService,
 }) {
   const store = useStoreProxy();
   const isFocused = store.isAnnotationFocused(annotation.$tag);
@@ -47,32 +53,21 @@ function Annotation({
   const userid = store.profile().userid;
   const isSaving = store.isSavingAnnotation(annotation);
 
-  const isCollapsedReply = isReply(annotation) && threadIsCollapsed;
+  const isCollapsedReply = isReply && threadIsCollapsed;
 
   const hasQuote = !!quote(annotation);
 
   const isEditing = !!draft && !isSaving;
 
-  const toggleAction = threadIsCollapsed ? 'Show replies' : 'Hide replies';
-  const toggleText = `${toggleAction} (${replyCount})`;
-
-  const shouldShowActions = !isSaving && !isEditing;
-  const shouldShowReplyToggle = replyCount > 0 && !isReply(annotation);
+  const showActions = !isSaving && !isEditing;
+  const showReplyToggle = !isReply && !hasAppliedFilter && replyCount > 0;
 
   const onReply = () => annotationsService.reply(annotation, userid);
 
-  const onToggleReplies = () =>
-    // nb. We assume the annotation has an ID here because it is not possible
-    // to create replies until the annotation has been saved.
-    store.setExpanded(
-      /** @type {string} */ (annotation.id),
-      !!threadIsCollapsed
-    );
-
   return (
     <article
       className={classnames('Annotation', {
-        'Annotation--reply': isReply(annotation),
+        'Annotation--reply': isReply,
         'is-collapsed': threadIsCollapsed,
         'is-focused': isFocused,
       })}
@@ -98,15 +93,15 @@ function Annotation({
       {!isCollapsedReply && (
         <footer className="Annotation__footer">
           <div className="Annotation__controls u-layout-row">
-            {shouldShowReplyToggle && (
-              <Button
-                className="Annotation__reply-toggle"
-                onClick={onToggleReplies}
-                buttonText={toggleText}
+            {showReplyToggle && (
+              <AnnotationReplyToggle
+                onToggleReplies={onToggleReplies}
+                replyCount={replyCount}
+                threadIsCollapsed={threadIsCollapsed}
               />
             )}
             {isSaving && <div className="Annotation__actions">Saving...</div>}
-            {shouldShowActions && (
+            {showActions && (
               <div className="Annotation__actions">
                 <AnnotationActionBar
                   annotation={annotation}
@@ -123,6 +118,9 @@ function Annotation({
 
 Annotation.propTypes = {
   annotation: propTypes.object.isRequired,
+  hasAppliedFilter: propTypes.bool.isRequired,
+  isReply: propTypes.bool,
+  onToggleReplies: propTypes.func,
   replyCount: propTypes.number.isRequired,
   showDocumentInfo: propTypes.bool.isRequired,
   threadIsCollapsed: propTypes.bool.isRequired,

src/sidebar/components/AnnotationMissing.js

+import classnames from 'classnames';
+import { createElement } from 'preact';
+import propTypes from 'prop-types';
+
+import AnnotationReplyToggle from './AnnotationReplyToggle';
+
+/** @typedef {import('./Annotation').AnnotationProps} AnnotationProps */
+
+/**
+ * @typedef {Omit<AnnotationProps, 'annotation'|'showDocumentInfo'|'annotationsService'>} AnnotationMissingProps
+ */
+
+/**
+ * Renders in place of an annotation if a thread's annotation is missing.
+ *
+ * @param {AnnotationMissingProps} props
+ */
+function AnnotationMissing({
+  hasAppliedFilter,
+  isReply,
+  onToggleReplies,
+  replyCount,
+  threadIsCollapsed,
+}) {
+  const showReplyToggle = !isReply && !hasAppliedFilter && replyCount > 0;
+  const isCollapsedReply = isReply && threadIsCollapsed;
+
+  return (
+    <article
+      className={classnames('Annotation', 'Annotation--missing', {
+        'is-collapsed': threadIsCollapsed,
+      })}
+    >
+      {!isCollapsedReply && (
+        <div>
+          <em>Message not available.</em>
+        </div>
+      )}
+
+      {!isCollapsedReply && (
+        <footer className="Annotation__footer">
+          <div className="Annotation__controls u-layout-row">
+            {showReplyToggle && (
+              <AnnotationReplyToggle
+                onToggleReplies={onToggleReplies}
+                replyCount={replyCount}
+                threadIsCollapsed={threadIsCollapsed}
+              />
+            )}
+          </div>
+        </footer>
+      )}
+    </article>
+  );
+}
+
+AnnotationMissing.propTypes = {
+  hasAppliedFilter: propTypes.bool,
+  isReply: propTypes.bool.isRequired,
+  onToggleReplies: propTypes.func,
+  replyCount: propTypes.number.isRequired,
+  threadIsCollapsed: propTypes.bool,
+};
+
+export default AnnotationMissing;

src/sidebar/components/AnnotationReplyToggle.js

+import { createElement } from 'preact';
+import propTypes from 'prop-types';
+
+import Button from './Button';
+
+/**
+ * @typedef AnnotationReplyToggleProps
+ * @prop {() => any} onToggleReplies
+ * @prop {number} replyCount
+ * @prop {boolean} threadIsCollapsed
+ */
+
+/**
+ * Render a thread-card control to toggle (expand or collapse) all of this
+ * thread's children.
+ *
+ * @param {AnnotationReplyToggleProps} props
+ */
+function AnnotationReplyToggle({
+  onToggleReplies,
+  replyCount,
+  threadIsCollapsed,
+}) {
+  const toggleAction = threadIsCollapsed ? 'Show replies' : 'Hide replies';
+  const toggleText = `${toggleAction} (${replyCount})`;
+
+  return (
+    <Button
+      className="Annotation__reply-toggle"
+      onClick={onToggleReplies}
+      buttonText={toggleText}
+    />
+  );
+}
+
+AnnotationReplyToggle.propTypes = {
+  onToggleReplies: propTypes.func,
+  replyCount: propTypes.number,
+  threadIsCollapsed: propTypes.bool.isRequired,
+};
+
+export default AnnotationReplyToggle;

src/sidebar/components/NotebookResultCount.js

@@ -28,7 +28,7 @@ function NotebookResultCount() {
 
   const hasResults = rootThread.children.length > 0;
 
-  const hasForcedVisible = forcedVisibleCount > 0;
+  const hasForcedVisible = hasAppliedFilter && forcedVisibleCount > 0;
   const matchCount = visibleCount - forcedVisibleCount;
   const threadCount = rootThread.children.length;
 

src/sidebar/components/Thread.js

 import classnames from 'classnames';
 import { createElement, Fragment } from 'preact';
-import { useMemo } from 'preact/hooks';
+import { useCallback, useMemo } from 'preact/hooks';
 
 import propTypes from 'prop-types';
 import { useStoreProxy } from '../store/use-store';
@@ -8,6 +8,7 @@ import { withServices } from '../service-context';
 import { countHidden, countVisible } from '../helpers/thread';
 
 import Annotation from './Annotation';
+import AnnotationMissing from './AnnotationMissing';
 import Button from './Button';
 import ModerationBanner from './ModerationBanner';
 
@@ -30,6 +31,7 @@ import ModerationBanner from './ModerationBanner';
 function Thread({ showDocumentInfo = false, thread, threadsService }) {
   // Only render this thread's annotation if it exists and the thread is `visible`
   const showAnnotation = thread.annotation && thread.visible;
+  const showMissingAnnotation = thread.visible && !thread.annotation;
 
   // Render this thread's replies only if the thread is expanded
   const showChildren = !thread.collapsed;
@@ -51,32 +53,54 @@ function Thread({ showDocumentInfo = false, thread, threadsService }) {
   );
 
   const store = useStoreProxy();
-  const onToggleReplies = () =>
-    store.setExpanded(thread.id, !!thread.collapsed);
+  const hasAppliedFilter = store.hasAppliedFilter();
+  const onToggleReplies = useCallback(
+    () => store.setExpanded(thread.id, !!thread.collapsed),
+    [store, thread.id, thread.collapsed]
+  );
 
   // Memoize annotation content to avoid re-rendering an annotation when content
   // in other annotations/threads change.
-  const annotationContent = useMemo(
-    () =>
-      showAnnotation && (
+  const annotationContent = useMemo(() => {
+    if (showAnnotation) {
+      return (
         <Fragment>
           <ModerationBanner annotation={thread.annotation} />
           <Annotation
             annotation={thread.annotation}
+            hasAppliedFilter={hasAppliedFilter}
+            isReply={!!thread.parent}
+            onToggleReplies={onToggleReplies}
             replyCount={thread.replyCount}
             showDocumentInfo={showDocumentInfo}
             threadIsCollapsed={thread.collapsed}
           />
         </Fragment>
-      ),
-    [
-      showAnnotation,
-      thread.annotation,
-      thread.replyCount,
-      showDocumentInfo,
-      thread.collapsed,
-    ]
-  );
+      );
+    } else if (showMissingAnnotation) {
+      return (
+        <AnnotationMissing
+          hasAppliedFilter={hasAppliedFilter}
+          isReply={!!thread.parent}
+          onToggleReplies={onToggleReplies}
+          replyCount={thread.replyCount}
+          threadIsCollapsed={thread.collapsed}
+        />
+      );
+    } else {
+      return null;
+    }
+  }, [
+    hasAppliedFilter,
+    onToggleReplies,
+    showAnnotation,
+    showMissingAnnotation,
+    showDocumentInfo,
+    thread.annotation,
+    thread.parent,
+    thread.replyCount,
+    thread.collapsed,
+  ]);
 
   return (
     <section
@@ -99,12 +123,6 @@ function Thread({ showDocumentInfo = false, thread, threadsService }) {
       <div className="Thread__content">
         {annotationContent}
 
-        {!thread.annotation && (
-          <div className="Thread__unavailable-message">
-            <em>Message not available.</em>
-          </div>
-        )}
-
         {showHiddenToggle && (
           <Button
             buttonText={`Show ${countHidden(thread)} more in conversation`}

src/sidebar/components/test/Annotation-test.js

 import { mount } from 'enzyme';
 import { createElement } from 'preact';
-import { act } from 'preact/test-utils';
 
 import * as fixtures from '../../test/annotation-fixtures';
 
@@ -11,6 +10,8 @@ import Annotation from '../Annotation';
 import { $imports } from '../Annotation';
 
 describe('Annotation', () => {
+  let fakeOnToggleReplies;
+
   // Dependency Mocks
   let fakeMetadata;
 
@@ -32,6 +33,9 @@ describe('Annotation', () => {
       <Annotation
         annotation={fixtures.defaultAnnotation()}
         annotationsService={fakeAnnotationsService}
+        hasAppliedFilter={false}
+        isReply={false}
+        onToggleReplies={fakeOnToggleReplies}
         replyCount={0}
         showDocumentInfo={false}
         threadIsCollapsed={true}
@@ -41,13 +45,14 @@ describe('Annotation', () => {
   };
 
   beforeEach(() => {
+    fakeOnToggleReplies = sinon.stub();
+
     fakeAnnotationsService = {
       reply: sinon.stub(),
       save: sinon.stub().resolves(),
     };
 
     fakeMetadata = {
-      isReply: sinon.stub(),
       quote: sinon.stub(),
     };
 
@@ -72,9 +77,10 @@ describe('Annotation', () => {
 
   describe('annotation classnames', () => {
     it('should assign a reply class if the annotation is a reply', () => {
-      fakeMetadata.isReply.returns(true);
-
-      const wrapper = createComponent({ threadIsCollapsed: false });
+      const wrapper = createComponent({
+        isReply: true,
+        threadIsCollapsed: false,
+      });
       const annot = wrapper.find('.Annotation');
 
       assert.isTrue(annot.hasClass('Annotation--reply'));
@@ -132,61 +138,50 @@ describe('Annotation', () => {
     assert.include(wrapper.find('.Annotation__actions').text(), 'Saving...');
   });
 
-  describe('reply thread toggle button', () => {
-    const findRepliesButton = wrapper =>
-      wrapper.find('Button').filter('.Annotation__reply-toggle');
-
+  describe('reply thread toggle', () => {
     it('should render a toggle button if the annotation has replies', () => {
-      fakeMetadata.isReply.returns(false);
       const wrapper = createComponent({
         replyCount: 5,
         threadIsCollapsed: true,
       });
 
-      assert.isTrue(findRepliesButton(wrapper).exists());
-      assert.equal(
-        findRepliesButton(wrapper).props().buttonText,
-        'Show replies (5)'
-      );
+      const toggle = wrapper.find('AnnotationReplyToggle');
+
+      assert.isTrue(toggle.exists());
+      assert.equal(toggle.props().onToggleReplies, fakeOnToggleReplies);
+      assert.equal(toggle.props().replyCount, 5);
+      assert.equal(toggle.props().threadIsCollapsed, true);
     });
 
-    it('should not render a toggle button if the annotation has no replies', () => {
-      fakeMetadata.isReply.returns(false);
+    it('should not render a reply toggle if the annotation has no replies', () => {
       const wrapper = createComponent({
+        isReply: false,
         replyCount: 0,
         threadIsCollapsed: true,
       });
 
-      assert.isFalse(findRepliesButton(wrapper).exists());
+      assert.isFalse(wrapper.find('AnnotationReplyToggle').exists());
     });
 
-    it('should not render a toggle button if the annotation itself is a reply', () => {
-      fakeMetadata.isReply.returns(true);
+    it('should not render a reply toggle if there are applied filters', () => {
       const wrapper = createComponent({
+        hasAppliedFilter: true,
+        isReply: false,
         replyCount: 5,
         threadIsCollapsed: true,
       });
 
-      assert.isFalse(findRepliesButton(wrapper).exists());
+      assert.isFalse(wrapper.find('AnnotationReplyToggle').exists());
     });
 
-    it('should toggle the collapsed state of the thread on click', () => {
-      fakeMetadata.isReply.returns(false);
+    it('should not render a reply toggle if the annotation itself is a reply', () => {
       const wrapper = createComponent({
+        isReply: true,
         replyCount: 5,
         threadIsCollapsed: true,
       });
 
-      act(() => {
-        findRepliesButton(wrapper).props().onClick();
-      });
-      wrapper.setProps({ threadIsCollapsed: false });
-
-      assert.calledOnce(fakeStore.setExpanded);
-      assert.equal(
-        findRepliesButton(wrapper).props().buttonText,
-        'Hide replies (5)'
-      );
+      assert.isFalse(wrapper.find('AnnotationReplyToggle').exists());
     });
   });
 
@@ -224,19 +219,21 @@ describe('Annotation', () => {
 
   context('annotation thread is collapsed', () => {
     context('collapsed reply', () => {
-      beforeEach(() => {
-        fakeMetadata.isReply.returns(true);
-      });
-
       it('should not render body or footer', () => {
-        const wrapper = createComponent({ threadIsCollapsed: true });
+        const wrapper = createComponent({
+          isReply: true,
+          threadIsCollapsed: true,
+        });
 
         assert.isFalse(wrapper.find('AnnotationBody').exists());
         assert.isFalse(wrapper.find('footer').exists());
       });
 
       it('should not show actions', () => {
-        const wrapper = createComponent({ threadIsCollapsed: true });
+        const wrapper = createComponent({
+          isReply: true,
+          threadIsCollapsed: true,
+        });
 
         assert.isFalse(wrapper.find('AnnotationActionBar').exists());
       });
@@ -244,8 +241,10 @@ describe('Annotation', () => {
 
     context('collapsed top-level annotation', () => {
       it('should render body and footer', () => {
-        fakeMetadata.isReply.returns(false);
-        const wrapper = createComponent({ threadIsCollapsed: true });
+        const wrapper = createComponent({
+          isReply: false,
+          threadIsCollapsed: true,
+        });
 
         assert.isTrue(wrapper.find('AnnotationBody').exists());
         assert.isTrue(wrapper.find('footer').exists());
@@ -269,15 +268,13 @@ describe('Annotation', () => {
       {
         name: 'when a collapsed top-level thread',
         content: () => {
-          fakeMetadata.isReply.returns(false);
-          return createComponent({ threadIsCollapsed: true });
+          return createComponent({ isReply: false, threadIsCollapsed: true });
         },
       },
       {
         name: 'when a collapsed reply',
         content: () => {
-          fakeMetadata.isReply.returns(true);
-          return createComponent({ threadIsCollapsed: true });
+          return createComponent({ isReply: true, threadIsCollapsed: true });
         },
       },
     ])

src/sidebar/components/test/AnnotationMissing-test.js

+import { mount } from 'enzyme';
+import { createElement } from 'preact';
+
+import AnnotationMissing, { $imports } from '../AnnotationMissing';
+
+import { checkAccessibility } from '../../../test-util/accessibility';
+import mockImportedComponents from '../../../test-util/mock-imported-components';
+
+describe('AnnotationMissing', () => {
+  let fakeOnToggleReplies;
+
+  function createComponent(props = {}) {
+    return mount(
+      <AnnotationMissing
+        hasAppliedFilter={false}
+        isReply={false}
+        onToggleReplies={fakeOnToggleReplies}
+        replyCount={5}
+        threadIsCollapsed={true}
+        {...props}
+      />
+    );
+  }
+
+  beforeEach(() => {
+    fakeOnToggleReplies = sinon.stub();
+    $imports.$mock(mockImportedComponents());
+  });
+
+  afterEach(() => {
+    $imports.$restore();
+  });
+
+  context('collapsed reply', () => {
+    it('does not show message-unavailable text', () => {
+      const wrapper = createComponent({
+        isReply: true,
+        threadIsCollapsed: true,
+      });
+      assert.equal(wrapper.text(), '');
+    });
+
+    it('does not render a reply toggle', () => {
+      const wrapper = createComponent({
+        isReply: true,
+        threadIsCollapsed: true,
+      });
+
+      assert.isFalse(wrapper.find('AnnotationReplyToggle').exists());
+    });
+  });
+
+  context('collapsed thread, not a reply', () => {
+    it('shows message-unavailable text', () => {
+      const wrapper = createComponent({
+        isReply: false,
+        threadIsCollapsed: true,
+      });
+
+      assert.match(wrapper.text(), /Message not available/);
+    });
+
+    it('renders a reply toggle control', () => {
+      const wrapper = createComponent({
+        isReply: false,
+        threadIsCollapsed: true,
+      });
+
+      const toggle = wrapper.find('AnnotationReplyToggle');
+      assert.equal(toggle.props().onToggleReplies, fakeOnToggleReplies);
+    });
+  });
+
+  context('expanded thread, not a reply', () => {
+    it('shows message-unavailable text', () => {
+      const wrapper = createComponent({
+        isReply: false,
+        threadIsCollapsed: false,
+      });
+
+      assert.match(wrapper.text(), /Message not available/);
+    });
+
+    it('renders a reply toggle control', () => {
+      const wrapper = createComponent({
+        isReply: false,
+        threadIsCollapsed: false,
+      });
+
+      const toggle = wrapper.find('AnnotationReplyToggle');
+      assert.equal(toggle.props().onToggleReplies, fakeOnToggleReplies);
+    });
+  });
+
+  it(
+    'should pass a11y checks',
+    checkAccessibility({
+      content: () => createComponent(),
+    })
+  );
+});

src/sidebar/components/test/AnnotationReplyToggle-test.js

+import { mount } from 'enzyme';
+import { createElement } from 'preact';
+import { act } from 'preact/test-utils';
+
+import AnnotationReplyToggle from '../AnnotationReplyToggle';
+
+import { checkAccessibility } from '../../../test-util/accessibility';
+
+describe('AnnotationReplyToggle', () => {
+  let fakeOnToggleReplies;
+
+  function createComponent(props = {}) {
+    return mount(
+      <AnnotationReplyToggle
+        onToggleReplies={fakeOnToggleReplies}
+        replyCount={5}
+        threadIsCollapsed={true}
+        {...props}
+      />
+    );
+  }
+
+  beforeEach(() => {
+    fakeOnToggleReplies = sinon.stub();
+    // Note that this component does not mock imported components
+    // because it entirely consists of a `Button`
+  });
+
+  it('renders expand wording if thread is collapsed', () => {
+    const wrapper = createComponent();
+
+    assert.match(wrapper.text(), /^Show replies/);
+  });
+
+  it('renders collapse wording if thread is expanded', () => {
+    const wrapper = createComponent({ threadIsCollapsed: false });
+
+    assert.match(wrapper.text(), /^Hide replies/);
+  });
+
+  it('shows the reply count', () => {
+    const wrapper = createComponent({ replyCount: 7 });
+    assert.equal(wrapper.text(), 'Show replies (7)');
+  });
+
+  it('invokes the toggle callback when clicked', () => {
+    const wrapper = createComponent();
+    const button = wrapper.find('Button');
+
+    act(() => {
+      button.props().onClick();
+    });
+
+    assert.calledOnce(fakeOnToggleReplies);
+  });
+
+  it(
+    'should pass a11y checks',
+    checkAccessibility({
+      content: () => createComponent(),
+    })
+  );
+});

src/sidebar/components/test/Thread-test.js

@@ -81,6 +81,7 @@ describe('Thread', () => {
 
   beforeEach(() => {
     fakeStore = {
+      hasAppliedFilter: sinon.stub().returns(false),
       setExpanded: sinon.stub(),
     };
 
@@ -189,7 +190,7 @@ describe('Thread', () => {
     });
   });
 
-  context('thread annotation has been deleted', () => {
+  context('visible thread whose annotation has been deleted', () => {
     let noAnnotationThread;
 
     beforeEach(() => {
@@ -204,10 +205,29 @@ describe('Thread', () => {
       assert.isFalse(wrapper.find('ModerationBanner').exists());
     });
 
-    it('renders an unavailable message', () => {
+    it('renders a missing annotation component', () => {
       const wrapper = createComponent({ thread: noAnnotationThread });
 
-      assert.isTrue(wrapper.find('.Thread__unavailable-message').exists());
+      const annotationMissing = wrapper.find('AnnotationMissing');
+
+      assert.isTrue(annotationMissing.exists());
+    });
+  });
+
+  context('non-visible thread whose annotation has been deleted', () => {
+    let noAnnotationThread;
+
+    beforeEach(() => {
+      noAnnotationThread = createThread();
+      noAnnotationThread.annotation = undefined;
+      noAnnotationThread.visible = false;
+    });
+
+    it('does not render any kind of annotation component', () => {
+      const wrapper = createComponent({ thread: noAnnotationThread });
+
+      assert.isFalse(wrapper.find('Annotation').exists());
+      assert.isFalse(wrapper.find('AnnotationMissing').exists());
     });
   });