Accommodate sorting of threads with no annotation at root level

Refactor sorting functions to operate on threads instead of annotations.
Establish logic for retrieving and comparing "root" annotations, which
aren't always at the top level of a thread for "headless threads".

src/sidebar/util/build-thread.js

@@ -170,44 +170,16 @@ function mapThread(thread, mapFn) {
   });
 }
 
-/**
- * Return a sorted copy of an array of threads.
- *
- * @param {Thread[]} threads - The list of threads to sort
- * @param {(a: Annotation, b: Annotation) => boolean} compareFn
- * @return {Thread[]} Sorted list of threads
- */
-function sort(threads, compareFn) {
-  return threads.slice().sort((a, b) => {
-    // Threads with no annotation always sort to the top
-    if (!a.annotation || !b.annotation) {
-      if (!a.annotation && !b.annotation) {
-        return 0;
-      } else {
-        return !a.annotation ? -1 : 1;
-      }
-    }
-
-    if (compareFn(a.annotation, b.annotation)) {
-      return -1;
-    } else if (compareFn(b.annotation, a.annotation)) {
-      return 1;
-    } else {
-      return 0;
-    }
-  });
-}
-
 /**
  * Return a new `Thread` object with all (recursive) `children` arrays sorted.
  * Sort the children of top-level threads using `compareFn` and all other
  * children using `replyCompareFn`.
  *
  * @param {Thread} thread
- * @param {(a: Annotation, b: Annotation) => boolean} compareFn - Less-than
- *         comparison function for sorting top-level annotations
- * @param {(a: Annotation, b: Annotation) => boolean} replyCompareFn - Less-than
- *       comparison function for sorting replies
+ * @param {(a: Thread, b: Thread) => number} compareFn - comparison function
+ *   for sorting top-level annotations
+ * @param {(a: Thread, b: Thread) => number} replyCompareFn - comparison
+ *   function for sorting replies
  * @return {Thread}
  */
 function sortThread(thread, compareFn, replyCompareFn) {
@@ -215,7 +187,9 @@ function sortThread(thread, compareFn, replyCompareFn) {
     sortThread(child, replyCompareFn, replyCompareFn)
   );
 
-  return { ...thread, children: sort(children, compareFn) };
+  const sortedChildren = children.slice().sort(compareFn);
+
+  return { ...thread, children: sortedChildren };
 }
 
 /**
@@ -253,38 +227,38 @@ function hasVisibleChildren(thread) {
 }
 
 /**
- * @typedef Options
- * @prop {string[]} selected - List of currently-selected annotation ids, from
- *       the data store
+ * @typedef BuildThreadOptions
+ * @prop {Object.<string, boolean>} expanded - Map of thread id => expansion state
  * @prop {string[]} forcedVisible - List of $tags of annotations that have
  *       been explicitly expanded by the user, even if they don't
  *       match current filters
+ * @prop {string[]} selected - List of currently-selected annotation ids, from
+ *       the data store
+ * @prop {(a: Thread, b: Thread) => number} sortCompareFn - comparison
+ *       function for sorting top-level annotations
  * @prop {(a: Annotation) => boolean} [filterFn] - Predicate function that
  *       returns `true` if annotation should be visible
  * @prop {(t: Thread) => boolean} [threadFilterFn] - Predicate function that
  *       returns `true` if the annotation should be included in the thread tree
- * @prop {Object.<string, boolean>} expanded - Map of thread id => expansion state
- * @prop {(a: Annotation, b: Annotation) => boolean} sortCompareFn - Less-than
- *       comparison function for sorting top-level annotations
- * @prop {(a: Annotation, b: Annotation) => boolean} replySortCompareFn - Less-than
- *       comparison function for sorting replies
  */
 
 /**
- * Default options for buildThread()
+ * Sort by reply (Annotation) `created` date
  *
- * @type {Options}
+ * @param {Thread} a
+ * @param {Thread} b
+ * @return {number}
  */
-const defaultOpts = {
-  selected: [],
-  expanded: {},
-  forcedVisible: [],
-  sortCompareFn: (a, b) => {
-    return a.$tag < b.$tag;
-  },
-  replySortCompareFn: (a, b) => {
-    return a.created < b.created;
-  },
+const replySortCompareFn = (a, b) => {
+  if (!a.annotation || !b.annotation) {
+    return 0;
+  }
+  if (a.annotation.created < b.annotation.created) {
+    return -1;
+  } else if (a.annotation.created > b.annotation.created) {
+    return 1;
+  }
+  return 0;
 };
 
 /**
@@ -306,15 +280,13 @@ const defaultOpts = {
  * a user).
  *
  * @param {Annotation[]} annotations - A list of annotations and replies
- * @param {Partial<Options>} options
+ * @param {BuildThreadOptions} options
  * @return {Thread} - The root thread, whose children are the top-level
  *                    annotations to display.
  */
 export default function buildThread(annotations, options) {
-  const opts = { ...defaultOpts, ...options };
-
-  const hasSelection = opts.selected.length > 0;
-  const hasForcedVisible = opts.forcedVisible.length > 0;
+  const hasSelection = options.selected.length > 0;
+  const hasForcedVisible = options.forcedVisible.length > 0;
 
   let thread = threadAnnotations(annotations);
 
@@ -322,18 +294,18 @@ export default function buildThread(annotations, options) {
     // Remove threads (annotations) that are not selected or
     // are not forced-visible
     thread.children = thread.children.filter(child => {
-      const isSelected = opts.selected.includes(child.id);
+      const isSelected = options.selected.includes(child.id);
       const isForcedVisible =
         hasForcedVisible &&
         child.annotation &&
-        opts.forcedVisible.includes(child.annotation.$tag);
+        options.forcedVisible.includes(child.annotation.$tag);
       return isSelected || isForcedVisible;
     });
   }
 
-  if (opts.threadFilterFn) {
+  if (options.threadFilterFn) {
     // Remove threads not matching thread-level filters
-    thread.children = thread.children.filter(opts.threadFilterFn);
+    thread.children = thread.children.filter(options.threadFilterFn);
   }
 
   // Set visibility for threads
@@ -342,17 +314,17 @@ export default function buildThread(annotations, options) {
 
     if (!thread.annotation) {
       threadIsVisible = false; // Nothing to show
-    } else if (opts.filterFn) {
+    } else if (options.filterFn) {
       if (
         hasForcedVisible &&
-        opts.forcedVisible.includes(thread.annotation.$tag)
+        options.forcedVisible.includes(thread.annotation.$tag)
       ) {
         // This annotation may or may not match the filter, but we should
         // make sure it is visible because it has been forced visible by user
         threadIsVisible = true;
       } else {
         // Otherwise, visibility depends on whether it matches the filter
-        threadIsVisible = !!opts.filterFn(thread.annotation);
+        threadIsVisible = !!options.filterFn(thread.annotation);
       }
     }
     return { ...thread, visible: threadIsVisible };
@@ -369,20 +341,22 @@ export default function buildThread(annotations, options) {
       collapsed: thread.collapsed,
     };
 
-    if (opts.expanded.hasOwnProperty(thread.id)) {
+    if (options.expanded.hasOwnProperty(thread.id)) {
       // This thread has been explicitly expanded/collapsed by user
-      threadStates.collapsed = !opts.expanded[thread.id];
+      threadStates.collapsed = !options.expanded[thread.id];
     } else {
       // If annotations are filtered, and at least one child matches
       // those filters, make sure thread is not collapsed
-      const hasUnfilteredChildren = opts.filterFn && hasVisibleChildren(thread);
+      const hasUnfilteredChildren =
+        options.filterFn && hasVisibleChildren(thread);
       threadStates.collapsed = thread.collapsed && !hasUnfilteredChildren;
     }
     return { ...thread, ...threadStates };
   });
 
   // Sort the root thread according to the current search criteria
-  thread = sortThread(thread, opts.sortCompareFn, opts.replySortCompareFn);
+  //const compareFn = options.sortCompareFn ?? defaultSortCompareFn;
+  thread = sortThread(thread, options.sortCompareFn, replySortCompareFn);
 
   // Update `replyCount` and `depth` properties
   thread = countRepliesAndDepth(thread, -1);

src/sidebar/util/test/thread-annotations-test.js

 import * as annotationFixtures from '../../test/annotation-fixtures';
 import uiConstants from '../../ui-constants';
 import threadAnnotations from '../thread-annotations';
+import { sorters } from '../thread-sorters';
 import { $imports } from '../thread-annotations';
 import immutable from '../immutable';
 
@@ -100,71 +101,15 @@ describe('sidebar/utils/thread-annotations', () => {
     });
 
     describe('when sort order changes', () => {
-      function sortBy(annotations, sortCompareFn) {
-        return annotations.slice().sort((a, b) => {
-          if (sortCompareFn(a, b)) {
-            return -1;
-          }
-          return sortCompareFn(b, a) ? 1 : 0;
-        });
-      }
-
-      // Format TextPositionSelector for the given position `pos`
-      function targetWithPos(pos) {
-        return [
-          {
-            selector: [{ type: 'TextPositionSelector', start: pos }],
-          },
-        ];
-      }
-
-      const annotations = [
-        {
-          target: targetWithPos(1),
-          updated: 20,
-        },
-        {
-          target: targetWithPos(100),
-          updated: 100,
-        },
-        {
-          target: targetWithPos(50),
-          updated: 50,
-        },
-        {
-          target: targetWithPos(20),
-          updated: 10,
-        },
-      ];
-
-      [
-        {
-          order: 'Location',
-          expectedOrder: [0, 3, 2, 1],
-        },
-        {
-          order: 'Oldest',
-          expectedOrder: [3, 0, 2, 1],
-        },
-        {
-          order: 'Newest',
-          expectedOrder: [1, 2, 0, 3],
-        },
-      ].forEach(testCase => {
-        it(`sorts correctly when sorting by ${testCase.order}`, () => {
-          fakeThreadState.selection.sortKey = testCase.order;
+      ['Location', 'Oldest', 'Newest'].forEach(testCase => {
+        it(`uses the appropriate sorting function when sorting by ${testCase}`, () => {
+          fakeThreadState.selection.sortKey = testCase;
 
           threadAnnotations(fakeThreadState);
 
           // The sort compare fn passed to `buildThread`
           const sortCompareFn = fakeBuildThread.args[0][1].sortCompareFn;
-
-          // Sort the test annotations by the sort compare fn that would be
-          // used by `build-thread` and make sure it's as expected
-          const actualOrder = sortBy(annotations, sortCompareFn).map(annot =>
-            annotations.indexOf(annot)
-          );
-          assert.deepEqual(actualOrder, testCase.expectedOrder);
+          assert.equal(sortCompareFn, sorters[testCase]);
         });
       });
     });

src/sidebar/util/test/thread-sorters-test.js

+import { sorters, $imports } from '../thread-sorters';
+
+describe('sidebar/util/thread-sorters', () => {
+  let fakeRootAnnotations;
+  let fakeLocation;
+
+  beforeEach(() => {
+    // The thread argument passed to `Newest` or `Oldest` sorting functions
+    // gets wrapped with an additional Array by `*RootAnnotationDate` before
+    // being passed on to `rootAnnotations`. This unwraps that extra array
+    // and returns the original first argument to `*RootAnnotationDate`
+    fakeRootAnnotations = sinon.stub().callsFake(threads => threads[0]);
+    fakeLocation = sinon.stub().callsFake(annotation => annotation.location);
+
+    $imports.$mock({
+      './annotation-metadata': { location: fakeLocation },
+      './thread': { rootAnnotations: fakeRootAnnotations },
+    });
+  });
+
+  afterEach(() => {
+    $imports.$restore();
+  });
+
+  describe('sorting by newest annotation thread first', () => {
+    [
+      {
+        a: [{ updated: 40 }, { updated: 5 }],
+        b: [{ updated: 20 }, { updated: 3 }],
+        expected: -1,
+      },
+      {
+        a: [{ updated: 20 }, { updated: 3 }],
+        b: [{ updated: 20 }, { updated: 3 }],
+        expected: 0,
+      },
+      {
+        a: [{ updated: 20 }, { updated: 3 }],
+        b: [{ updated: 40 }, { updated: 5 }],
+        expected: 1,
+      },
+    ].forEach(testCase => {
+      it('sorts by newest updated root annotation', () => {
+        // Disable eslint: `sorters` properties start with capital letters
+        // to match their displayed sort option values
+        /* eslint-disable-next-line new-cap */
+        assert.equal(sorters.Newest(testCase.a, testCase.b), testCase.expected);
+      });
+    });
+  });
+
+  describe('sorting by oldest annotation thread first', () => {
+    [
+      {
+        a: [{ updated: 20 }, { updated: 5 }],
+        b: [{ updated: 40 }, { updated: 3 }],
+        expected: 1,
+      },
+      {
+        a: [{ updated: 20 }, { updated: 3 }],
+        b: [{ updated: 20 }, { updated: 3 }],
+        expected: 0,
+      },
+      {
+        a: [{ updated: 40 }, { updated: 3 }],
+        b: [{ updated: 20 }, { updated: 5 }],
+        expected: -1,
+      },
+    ].forEach(testCase => {
+      it('sorts by oldest updated root annotation', () => {
+        // Disable eslint: `sorters` properties start with capital letters
+        // to match their displayed sort option values
+        /* eslint-disable-next-line new-cap */
+        assert.equal(sorters.Oldest(testCase.a, testCase.b), testCase.expected);
+      });
+    });
+  });
+
+  describe('sorting by document location', () => {
+    [
+      {
+        a: { annotation: { location: 5 } },
+        b: { annotation: { location: 10 } },
+        expected: -1,
+      },
+      {
+        a: { annotation: { location: 10 } },
+        b: { annotation: { location: 10 } },
+        expected: 0,
+      },
+      {
+        a: { annotation: { location: 10 } },
+        b: { annotation: { location: 5 } },
+        expected: 1,
+      },
+      {
+        a: {},
+        b: { annotation: { location: 5 } },
+        expected: -1,
+      },
+      {
+        a: {},
+        b: {},
+        expected: 0,
+      },
+      {
+        a: { annotation: { location: 10 } },
+        b: {},
+        expected: 1,
+      },
+    ].forEach(testCase => {
+      it('sorts by annotation location', () => {
+        assert.equal(
+          // Disable eslint: `sorters` properties start with capital letters
+          // to match their displayed sort option values
+          /* eslint-disable-next-line new-cap */
+          sorters.Location(testCase.a, testCase.b),
+          testCase.expected
+        );
+      });
+    });
+  });
+});

src/sidebar/util/test/thread-test.js

@@ -51,4 +51,45 @@ describe('sidebar/util/thread', () => {
       assert.equal(threadUtil.countHidden(thread), 3);
     });
   });
+
+  describe('rootAnnotations', () => {
+    it("returns all of the annotations in the thread's child threads if there is at least one annotation present", () => {
+      const fixture = {
+        children: [
+          { annotation: 1, children: [] },
+          { children: [] },
+          { annotation: 2, children: [] },
+        ],
+      };
+      assert.deepEqual(threadUtil.rootAnnotations(fixture.children), [1, 2]);
+    });
+
+    it('returns all of the annotations at the first depth that has any annotations', () => {
+      const fixture = {
+        children: [
+          {
+            children: [
+              { annotation: 1, children: [] },
+              { children: [] },
+              { annotation: 2, children: [] },
+            ],
+          },
+          { children: [{ children: [{ annotation: 3, children: [] }] }] },
+          { children: [{ annotation: 4, children: [] }] },
+        ],
+      };
+
+      assert.deepEqual(threadUtil.rootAnnotations(fixture.children), [1, 2, 4]);
+    });
+
+    it('throws an exception if fed a thread hierarchy with no annotations', () => {
+      const fixture = {
+        children: [{ children: [{ children: [] }] }],
+      };
+
+      assert.throws(() => {
+        threadUtil.rootAnnotations(fixture.children);
+      }, /Thread contains no annotations/);
+    });
+  });
 });

src/sidebar/util/thread-annotations.js

 import buildThread from './build-thread';
 import memoize from './memoize';
-import * as metadata from './annotation-metadata';
 import { generateFacetedFilter } from './search-filter';
 import filterAnnotations from './view-filter';
 import { shouldShowInTab } from './tabs';
+import { sorters } from './thread-sorters';
 
 /** @typedef {import('../../types/api').Annotation} Annotation */
 /** @typedef {import('./build-thread').Thread} Thread */
-/** @typedef {import('./build-thread').Options} BuildThreadOptions */
+/** @typedef {import('./build-thread').BuildThreadOptions} BuildThreadOptions */
 
 /**
  * @typedef ThreadState
@@ -23,19 +23,6 @@ import { shouldShowInTab } from './tabs';
  * @prop {string|null} route
  */
 
-// Sort functions keyed on sort option
-const sortFns = {
-  Newest: function (a, b) {
-    return a.updated > b.updated;
-  },
-  Oldest: function (a, b) {
-    return a.updated < b.updated;
-  },
-  Location: function (a, b) {
-    return metadata.location(a) < metadata.location(b);
-  },
-};
-
 /**
  * Cobble together the right set of options and filters based on current
  * `threadState` to build the root thread.
@@ -46,12 +33,11 @@ const sortFns = {
 function buildRootThread(threadState) {
   const selection = threadState.selection;
 
-  /** @type {Partial<BuildThreadOptions>} */
   const options = {
     expanded: selection.expanded,
     forcedVisible: selection.forcedVisible,
     selected: selection.selected,
-    sortCompareFn: sortFns[selection.sortKey],
+    sortCompareFn: sorters[selection.sortKey],
   };
 
   // Is there a filter query present, or an applied user (focus) filter?

src/sidebar/util/thread-sorters.js

+import { location } from './annotation-metadata';
+import { rootAnnotations } from './thread';
+
+/** @typedef {import('./build-thread').Thread} Thread */
+
+/**
+ * Sort comparison function when one or both threads being compared is lacking
+ * an annotation.
+ * Sort such that a thread without an annotation sorts to the top
+ *
+ * @param {Thread} a
+ * @param {Thread} b
+ * @return {number}
+ */
+function compareHeadlessThreads(a, b) {
+  if (!a.annotation && !b.annotation) {
+    return 0;
+  } else {
+    return !a.annotation ? -1 : 1;
+  }
+}
+
+/**
+ * Find the most recent updated date amongst a thread's root annotation set
+ *
+ * @param {Thread} thread
+ * @return {string}
+ */
+function newestRootAnnotationDate(thread) {
+  const annotations = rootAnnotations([thread]);
+  return annotations.reduce(
+    (newestDate, annotation) =>
+      annotation.updated > newestDate ? annotation.updated : newestDate,
+    ''
+  );
+}
+
+/**
+ * Find the oldest updated date amongst a thread's root annotation set
+ *
+ * @param {Thread} thread
+ * @return {string}
+ */
+function oldestRootAnnotationDate(thread) {
+  const annotations = rootAnnotations([thread]);
+  return annotations.reduce((oldestDate, annotation) => {
+    if (!oldestDate) {
+      oldestDate = annotation.updated;
+    }
+    return annotation.updated < oldestDate ? annotation.updated : oldestDate;
+  }, '');
+}
+
+/**
+ * Sorting comparison functions for the three defined application options for
+ * sorting annotation (threads)
+ */
+export const sorters = {
+  Newest: function (a, b) {
+    const dateA = newestRootAnnotationDate(a);
+    const dateB = newestRootAnnotationDate(b);
+    if (dateA > dateB) {
+      return -1;
+    } else if (dateA < dateB) {
+      return 1;
+    }
+    return 0;
+  },
+
+  Oldest: function (a, b) {
+    const dateA = oldestRootAnnotationDate(a);
+    const dateB = oldestRootAnnotationDate(b);
+    if (dateA < dateB) {
+      return -1;
+    } else if (dateA > dateB) {
+      return 1;
+    }
+    return 0;
+  },
+
+  Location: function (a, b) {
+    if (!a.annotation || !b.annotation) {
+      return compareHeadlessThreads(a, b);
+    }
+    const aLocation = location(a.annotation);
+    const bLocation = location(b.annotation);
+    if (aLocation < bLocation) {
+      return -1;
+    } else if (aLocation > bLocation) {
+      return 1;
+    }
+    return 0;
+  },
+};

src/sidebar/util/thread.js

+import { notNull } from './typing';
+
+/** @typedef {import('../../types/api').Annotation} Annotation */
 /** @typedef {import('../util/build-thread').Thread} Thread */
 
 /**
@@ -31,3 +34,58 @@ export function countHidden(thread) {
 export function countVisible(thread) {
   return countByVisibility(thread, true);
 }
+
+/**
+ * Find the topmost annotations in a thread.
+ *
+ * For the (vast) majority of threads, this is the single annotation at the
+ * top level of the thread hierarchy.
+ *
+ * However, when the top-level thread lacks
+ * an annotation, as is the case if that annotation has been deleted but still
+ * has replies, find the first level of descendants that has at least one
+ * annotation (reply) and return the set of annotations (replies) at that level.
+ *
+ * For example, given the (overly-complex) thread-annotation structure of:
+ *
+ * [missing]
+ *   - [missing]
+ *       - reply 1
+ *            - reply 2
+ *            - reply 3
+ *       - reply 4
+ *   - [missing]
+ *       - reply 5
+ *   - [missing]
+ *      - [missing]
+ *          - reply 6
+ *
+ * Return [reply 1, reply 4, reply 5]
+ *
+ * @param {Thread[]} threads
+ * @return {Annotation[]}
+ */
+export function rootAnnotations(threads) {
+  // If there are any threads at this level with extant annotations, return
+  // those annotations
+  const threadAnnotations = threads
+    .filter(thread => !!thread.annotation)
+    .map(thread => notNull(thread.annotation));
+
+  if (threadAnnotations.length) {
+    return threadAnnotations;
+  }
+
+  // Else, search across all children at once (an entire hierarchical level)
+  const allChildren = [];
+  threads.forEach(thread => {
+    if (thread.children) {
+      allChildren.push(...thread.children);
+    }
+  });
+  if (allChildren.length) {
+    return rootAnnotations(allChildren);
+  }
+
+  throw new Error('Thread contains no annotations');
+}