Refactor `build-thread` (#2321)

- Add additional type-checking and clarify comments
- Use ES6 conventions
- Structure code for readability

src/sidebar/build-thread.js

@@ -2,107 +2,110 @@
  * @typedef {import('../types/api').Annotation} Annotation
  *
  * @typedef Thread
- * @prop {string} id
- * @prop {Annotation|undefined} annotation
- * @prop {Thread|undefined} parent
- * @prop {boolean} visible
- * @prop {boolean} collapsed
+ * @prop {string} id - The thread's id, which equivalent to the id of its
+ *       annotation. For unsaved annotations, the id is derived from the
+ *       annotation's local `$tag` property.
+ * @prop {Annotation} [annotation] - This thread's annotation. Undefined in cases
+ *       when an annotation _should_ exist—it's implied by a reference from
+ *       another annotation—but is not present in our collection of annotations.
+ *       This can happen when a reply has been deleted, but still has children
+ *       that exist.
+ * @prop {string} [parent] - The id of this thread's parent. Top-level threads
+ *       do not have parents
+ * @prop {boolean} visible - Whether this thread should be visible when rendered.
+ *       true when the thread's annotation matches current annotation filters.
+ * @prop {boolean} collapsed - Whether the replies in this thread should be
+ *       rendered as collapsed (when true) or expanded (when false)
  * @prop {Thread[]} children
- * @prop {number} totalChildren
- * @prop {'dim'|'highlight'|undefined} highlightState
+ * @prop {number} totalChildren - Computed count of this thread's immediate
+ *       children. This count includes visually-hidden threads.
+ * @prop {number} replyCount - Computed count of all replies to a thread
+ * @prop {number} [depth] - The thread's depth in the hierarchy
+ * @prop {'dim'|'highlight'} [highlightState] - In cases where there are one
+ *       or more threads currently designated as "highlighted", we track
+ *       the highlight state for each thread. undefined when there are no
+ *       highlighted threads.
  */
 
 /**
- * Default state for new threads, before applying filters etc.
- *
- * @type {Thread}
+ * Default state for new threads
  */
 const DEFAULT_THREAD_STATE = {
-  /**
-   * The ID of this thread. This will be the same as the annotation ID for
-   * created annotations or the `$tag` property for new annotations.
-   */
   id: '__default__',
-  /**
-   * The Annotation which is displayed by this thread.
-   *
-   * This may be null if the existence of an annotation is implied by the
-   * `references` field in an annotation but the referenced parent annotation
-   * does not exist.
-   */
-  annotation: undefined,
-  /** The parent thread ID */
-  parent: undefined,
-  /** True if this thread is collapsed, hiding replies to this annotation. */
   collapsed: false,
-  /** True if this annotation matches the current filters. */
   visible: true,
-  /** Replies to this annotation. */
-  children: [],
-  /**
-   * The total number of children of this annotation,
-   * including any which have been hidden by filters.
-   */
+  replyCount: 0,
   totalChildren: 0,
-  /**
-   * The highlight state of this annotation:
-   *  undefined - Do not (de-)emphasize this annotation
-   *  'dim' - De-emphasize this annotation
-   *  'highlight' - Emphasize this annotation
-   */
-  highlightState: undefined,
 };
 
 /**
  * Returns a persistent identifier for an Annotation.
  * If the Annotation has been created on the server, it will have
- * an ID assigned, otherwise we fall back to the local-only '$tag'
+ * an id assigned, otherwise we fall back to the local-only '$tag'
  * property.
+ *
+ * @param {Annotation} annotation
+ * @return {string}
  */
-function id(annotation) {
+function annotationId(annotation) {
   return annotation.id || annotation.$tag;
 }
 
 /**
- * Link the annotation with ID `id` to its parent thread.
+ * Is there a valid path from the thread indicated by `id` to the root thread,
+ * with no circular references?
  *
+ * @param {string} id - The id of the thread to be verified
+ * @param {string} ancestorId - The ancestor of the thread indicated by id that
+ *        is to be verified: is it extant and not a circular reference?
+ * @return {boolean}
+ */
+function hasPathToRoot(threads, id, ancestorId) {
+  if (!threads[ancestorId] || threads[ancestorId].parent === id) {
+    // Thread for ancestor not found, or points at itself: circular reference
+    return false;
+  } else if (!threads[ancestorId].parent) {
+    // Top of the tree: we've made it
+    return true;
+  }
+  return hasPathToRoot(threads, id, threads[ancestorId].parent);
+}
+
+/**
+ * Link the thread's annotation to its parent
+ * @param {Object.<string,Thread>} threads
  * @param {string} id
- * @param {Array<string>} parents - IDs of parent annotations, from the
- *        annotation's `references` field.
+ * @param {string[]} [parents] - ids of parent annotations, from the
+ *        annotation's `references` field. Immediate parent is last entry.
  */
-function setParentID(threads, id, parents) {
+function setParent(threads, id, parents = []) {
   if (threads[id].parent || !parents.length) {
     // Parent already assigned, do not try to change it.
     return;
   }
-  const parentID = parents[parents.length - 1];
-  if (!threads[parentID]) {
+  const parentId = parents[parents.length - 1];
+
+  if (!threads[parentId]) {
     // Parent does not exist. This may be a reply to an annotation which has
     // been deleted. Create a placeholder Thread with no annotation to
     // represent the missing annotation.
-    threads[parentID] = Object.assign({}, DEFAULT_THREAD_STATE, {
-      id: parentID,
+    threads[parentId] = {
+      ...DEFAULT_THREAD_STATE,
       children: [],
-    });
-    setParentID(threads, parentID, parents.slice(0, -1));
+    };
+    // Link up this new thread to _its_ parent, which should be the original
+    // thread's grandparent
+    setParent(threads, parentId, parents.slice(0, -1));
   }
 
-  let grandParentID = threads[parentID].parent;
-  while (grandParentID) {
-    if (grandParentID === id) {
-      // There is a loop in the `references` field, abort.
-      return;
-    } else {
-      grandParentID = threads[grandParentID].parent;
-    }
+  if (hasPathToRoot(threads, id, parentId)) {
+    threads[id].parent = parentId;
+    threads[parentId].children.push(threads[id]);
   }
-
-  threads[id].parent = parentID;
-  threads[parentID].children.push(threads[id]);
 }
 
 /**
- * Creates a thread of annotations from a list of annotations.
+ * Creates a thread tree of annotations from a list of annotations.
  *
  * Given a flat list of annotations and replies, this generates a hierarchical
  * thread, using the `references` field of an annotation to link together
@@ -110,53 +113,52 @@ function setParentID(threads, id, parents) {
  * incomplete ordered list of the parents of an annotation, from furthest to
  * nearest ancestor.
  *
- * @param {Array<Annotation>} annotations - The input annotations to thread.
+ * @param {Annotation[]} annotations - The input annotations to thread.
  * @return {Thread} - The input annotations threaded into a tree structure.
  */
 function threadAnnotations(annotations) {
-  // Map of annotation ID -> container
+  /** @type {Object.<string,Thread>} */
   const threads = {};
 
-  // Build mapping of annotation ID -> thread
-  annotations.forEach(function (annotation) {
-    threads[id(annotation)] = Object.assign({}, DEFAULT_THREAD_STATE, {
-      id: id(annotation),
-      annotation: annotation,
+  // Create a `Thread` for each annotation
+  annotations.forEach(annotation => {
+    const id = annotationId(annotation);
+    threads[id] = {
+      ...DEFAULT_THREAD_STATE,
       children: [],
-    });
+      annotation,
+      id,
+    };
   });
 
-  // Set each thread's parent based on the references field
-  annotations.forEach(function (annotation) {
-    if (!annotation.references) {
-      return;
-    }
-    setParentID(threads, id(annotation), annotation.references);
+  // Establish ancestral relationships between annotations
+  annotations.forEach(annotation => {
+    // Remove references to self from `references` to avoid circular references
+    const parents = (annotation.references || []).filter(
+      id => id !== annotation.id
+    );
+    return setParent(threads, annotationId(annotation), parents);
   });
 
   // Collect the set of threads which have no parent as
   // children of the thread root
-  const roots = [];
-  Object.keys(threads).forEach(function (id) {
-    if (!threads[id].parent) {
+  const rootThreads = [];
+  for (const rootThreadId in threads) {
+    if (!threads[rootThreadId].parent) {
       // Top-level threads are collapsed by default
-      threads[id].collapsed = true;
-      roots.push(threads[id]);
+      threads[rootThreadId].collapsed = true;
+      rootThreads.push(threads[rootThreadId]);
     }
-  });
+  }
 
-  const root = {
+  const rootThread = {
+    ...DEFAULT_THREAD_STATE,
     id: 'root',
-    annotation: undefined,
-    children: roots,
-    visible: true,
-    collapsed: false,
-    totalChildren: roots.length,
-    parent: undefined,
-    highlightState: undefined,
+    children: rootThreads,
+    totalChildren: rootThreads.length,
   };
 
-  return root;
+  return rootThread;
 }
 
 /**
@@ -164,11 +166,12 @@ function threadAnnotations(annotations) {
  * and each of its children transformed by mapFn(thread).
  *
  * @param {Thread} thread
- * @param {(Thread) => Thread} mapFn
+ * @param {(t: Thread) => Thread} mapFn
+ * @return {Thread}
  */
 function mapThread(thread, mapFn) {
   return Object.assign({}, mapFn(thread), {
-    children: thread.children.map(function (child) {
+    children: thread.children.map(child => {
       return mapThread(child, mapFn);
     }),
   });
@@ -177,12 +180,12 @@ function mapThread(thread, mapFn) {
 /**
  * Return a sorted copy of an array of threads.
  *
- * @param {Array<Thread>} threads - The list of threads to sort
+ * @param {Thread[]} threads - The list of threads to sort
  * @param {(a: Annotation, b: Annotation) => boolean} compareFn
- * @return {Array<Thread>} Sorted list of threads
+ * @return {Thread[]} Sorted list of threads
  */
 function sort(threads, compareFn) {
-  return threads.slice().sort(function (a, b) {
+  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) {
@@ -203,96 +206,91 @@ function sort(threads, compareFn) {
 }
 
 /**
- * Return a copy of `thread` with siblings of the top-level thread sorted according
- * to `compareFn` and replies sorted by `replyCompareFn`.
+ * 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
+ * @return {Thread}
  */
 function sortThread(thread, compareFn, replyCompareFn) {
-  const children = thread.children.map(function (child) {
-    return sortThread(child, replyCompareFn, replyCompareFn);
-  });
+  const children = thread.children.map(child =>
+    sortThread(child, replyCompareFn, replyCompareFn)
+  );
 
-  return Object.assign({}, thread, {
-    children: sort(children, compareFn),
-  });
+  return { ...thread, children: sort(children, compareFn) };
 }
 
 /**
- * Return a copy of @p thread with the `replyCount` and `depth` properties
+ * Return a copy of `thread` with the `replyCount` and `depth` properties
  * updated.
+ *
+ * @param {Thread} thread
+ * @param {number} depth
+ * @return {Thread}
  */
 function countRepliesAndDepth(thread, depth) {
-  const children = thread.children.map(function (c) {
-    return countRepliesAndDepth(c, depth + 1);
-  });
-  return Object.assign({}, thread, {
-    children: children,
-    depth: depth,
-    replyCount: children.reduce(function (total, child) {
-      return total + 1 + child.replyCount;
-    }, 0),
-  });
+  const children = thread.children.map(c => countRepliesAndDepth(c, depth + 1));
+  const replyCount = children.reduce(
+    (total, child) => total + 1 + child.replyCount,
+    0
+  );
+  return {
+    ...thread,
+    children,
+    depth,
+    replyCount,
+  };
 }
 
-/** Return true if a thread has any visible children. */
+/**
+ * Does this thread have any visible children?
+ *
+ * @param {Thread} thread
+ * @return {boolean}
+ */
 function hasVisibleChildren(thread) {
-  return thread.children.some(function (child) {
+  return thread.children.some(child => {
     return child.visible || hasVisibleChildren(child);
   });
 }
 
 /**
  * @typedef Options
- * @prop {string[]} [selected]
- * @prop {string[]} [forceVisible]
- * @prop {(a: Annotation) => boolean} [filterFn]
- * @prop {(t: Thread) => boolean} [threadFilterFn]
- * @prop {Object} [expanded]
- * @prop {Object} [highlighted]
- * @prop {(a: Annotation, b: Annotation) => boolean} [sortCompareFn]
- * @prop {(a: Annotation, b: Annotation) => boolean} [replySortCompareFn]
+ * @prop {string[]} [selected] - List of currently-selected annotation ids, from
+ *       the data store
+ * @prop {string[]} [forceVisible] - List of ids of annotations that have
+ *       been explicitly expanded by the user, even if they don't
+ *       match current filters
+ * @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 {string[]} [highlighted] - List of ids of annotations that are highlighted
+ * @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()
  *
- * @type {Partial<Options>}
+ * @type {Options}
  */
 const defaultOpts = {
-  /** List of currently selected annotation IDs */
   selected: [],
-  /**
-   * List of IDs of annotations that should be shown even if they
-   * do not match the current filter.
-   */
-  forceVisible: undefined,
-  /**
-   * Predicate function that returns true if an annotation should be
-   * displayed.
-   */
-  filterFn: undefined,
-  /**
-   * A filter function which should return true if a given annotation and
-   * its replies should be displayed.
-   */
-  threadFilterFn: undefined,
-  /**
-   * Mapping of annotation IDs to expansion states.
-   */
   expanded: {},
-  /** List of highlighted annotation IDs */
   highlighted: [],
-  /**
-   * Less-than comparison function used to compare annotations in order to sort
-   * the top-level thread.
-   */
-  sortCompareFn: function (a, b) {
+  sortCompareFn: (a, b) => {
     return a.id < b.id;
   },
-  /**
-   * Less-than comparison function used to compare annotations in order to sort
-   * replies.
-   */
-  replySortCompareFn: function (a, b) {
+  replySortCompareFn: (a, b) => {
     return a.created < b.created;
   },
 };
@@ -305,86 +303,99 @@ const defaultOpts = {
  * the current visibility filters and sort function and returns
  * the thread structure that should be rendered.
  *
- * @param {Array<Annotation>} annotations - A list of annotations and replies
- * @param {Options} opts
+ * An Annotation present in `annotations` will not be present in the returned threads if:
+ * - The annotation does not match thread-level filters (options.threadFilterFn), OR
+ * - The annotation is not in the current selection (options.selected), OR
+ * - The annotation's thread is hidden and has no visible children
+ *
+ * Annotations that do not match the currently-applied annotation filters
+ * (options.filterFn) will have their thread's `visible` property set to `hidden`
+ * (an exception is made if that annotation's thead has been forced visible by
+ * a user).
+ *
+ * @param {Annotation[]} annotations - A list of annotations and replies
+ * @param {Partial<Options>} options
  * @return {Thread} - The root thread, whose children are the top-level
  *                    annotations to display.
  */
-export default function buildThread(annotations, opts) {
-  opts = Object.assign({}, defaultOpts, opts);
+export default function buildThread(annotations, options) {
+  const opts = { ...defaultOpts, ...options };
+
+  const annotationsFiltered = !!opts.filterFn;
+  const threadsFiltered = !!opts.threadFilterFn;
+
+  const hasHighlights = opts.highlighted.length > 0;
+  const hasSelection = opts.selected.length > 0;
+  const hasForcedVisible = opts.forceVisible && opts.forceVisible.length;
 
   let thread = threadAnnotations(annotations);
 
-  // Mark annotations as visible or hidden depending on whether
-  // they are being edited and whether they match the current filter
-  // criteria
-  const shouldShowThread = function (annotation) {
-    if (opts.forceVisible && opts.forceVisible.indexOf(id(annotation)) !== -1) {
-      return true;
-    }
-    if (opts.filterFn && !opts.filterFn(annotation)) {
-      return false;
-    }
-    return true;
-  };
+  if (hasSelection) {
+    // Remove threads (annotations) that are not selected
+    thread.children = thread.children.filter(
+      child => opts.selected.indexOf(child.id) !== -1
+    );
+  }
 
-  // When there is a selection, only include top-level threads (annotations)
-  // that are selected
-  if (opts.selected.length > 0) {
-    thread = Object.assign({}, thread, {
-      children: thread.children.filter(function (child) {
-        return opts.selected.indexOf(child.id) !== -1;
-      }),
-    });
+  if (threadsFiltered) {
+    // Remove threads not matching thread-level filters
+    thread.children = thread.children.filter(opts.threadFilterFn);
   }
 
-  // Set the visibility and highlight states of threads
-  thread = mapThread(thread, function (thread) {
-    let highlightState;
-    if (opts.highlighted.length > 0) {
-      const isHighlighted =
-        thread.annotation && opts.highlighted.indexOf(thread.id) !== -1;
-      highlightState = isHighlighted ? 'highlight' : 'dim';
+  // Set visibility for threads
+  thread = mapThread(thread, thread => {
+    let threadIsVisible = thread.visible;
+
+    if (!thread.annotation) {
+      threadIsVisible = false; // Nothing to show
+    } else if (annotationsFiltered) {
+      if (
+        hasForcedVisible &&
+        opts.forceVisible.indexOf(annotationId(thread.annotation)) !== -1
+      ) {
+        // 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);
+      }
     }
-
-    return Object.assign({}, thread, {
-      highlightState: highlightState,
-      visible:
-        thread.visible &&
-        thread.annotation &&
-        shouldShowThread(thread.annotation),
-    });
+    return { ...thread, visible: threadIsVisible };
   });
 
-  // Expand any threads which:
-  // 1) Have been explicitly expanded OR
-  // 2) Have children matching the filter
-  thread = mapThread(thread, function (thread) {
-    const id = thread.id;
-
-    // If the thread was explicitly expanded or collapsed, respect that option
-    if (opts.expanded.hasOwnProperty(id)) {
-      return Object.assign({}, thread, { collapsed: !opts.expanded[id] });
+  // Remove top-level threads which contain no visible annotations
+  thread.children = thread.children.filter(
+    child => child.visible || hasVisibleChildren(child)
+  );
+
+  // Determine other UI states for threads: highlight and collapsed
+  thread = mapThread(thread, thread => {
+    const threadStates = {
+      collapsed: thread.collapsed,
+    };
+
+    if (hasHighlights) {
+      if (thread.annotation && opts.highlighted.indexOf(thread.id) !== -1) {
+        threadStates.highlightState = 'highlight';
+      } else {
+        threadStates.highlightState = 'dim';
+      }
     }
 
-    const hasUnfilteredChildren = opts.filterFn && hasVisibleChildren(thread);
-
-    return Object.assign({}, thread, {
-      collapsed: thread.collapsed && !hasUnfilteredChildren,
-    });
-  });
-
-  // Remove top-level threads which contain no visible annotations
-  thread.children = thread.children.filter(function (child) {
-    return child.visible || hasVisibleChildren(child);
+    if (opts.expanded.hasOwnProperty(thread.id)) {
+      // This thread has been explicitly expanded/collapsed by user
+      threadStates.collapsed = !opts.expanded[thread.id];
+    } else {
+      // If annotations are filtered, and at least one child matches
+      // those filters, make sure thread is not collapsed
+      const hasUnfilteredChildren =
+        annotationsFiltered && hasVisibleChildren(thread);
+      threadStates.collapsed = thread.collapsed && !hasUnfilteredChildren;
+    }
+    return { ...thread, ...threadStates };
   });
 
-  // Get annotations which are of type notes or annotations depending
-  // on the filter.
-  if (opts.threadFilterFn) {
-    thread.children = thread.children.filter(opts.threadFilterFn);
-  }
-
   // Sort the root thread according to the current search criteria
   thread = sortThread(thread, opts.sortCompareFn, opts.replySortCompareFn);
 

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

@@ -141,6 +141,39 @@ describe('build-thread', function () {
       ]);
     });
 
+    it('handles annotations that have their own ID in their `references` list', () => {
+      const fixture = [
+        { id: '1', references: ['1'] },
+        { id: '2', references: ['1'] },
+        { id: '3', references: ['1', '3'] },
+      ];
+      const thread = createThread(fixture);
+      assert.deepEqual(thread, [
+        {
+          annotation: {
+            id: '1',
+            references: ['1'],
+          },
+          children: [
+            {
+              annotation: {
+                id: '2',
+                references: ['1'],
+              },
+              children: [],
+            },
+            {
+              annotation: {
+                id: '3',
+                references: ['1', '3'],
+              },
+              children: [],
+            },
+          ],
+        },
+      ]);
+    });
+
     it('handles missing parent annotations', function () {
       const fixture = [
         {

src/types/api.js

@@ -38,6 +38,8 @@
  *
  * @typedef Annotation
  * @prop {string} [id]
+ * @prop {string} [$tag] - A locally-generated unique identifier for annotations
+ *       that have not been saved to the service yet (and thus do not have an id)
  * @prop {string[]} [references]
  * @prop {string} created
  * @prop {string} group