Remove `threadState`, add supporting selectors

Eliminate the enormous `threadState` `rootSelector`
and re-implement as multiple selectors. Refactor
`useRootThread` and threading integration tests
to account for these changes.

src/sidebar/components/hooks/test/use-root-thread-test.js

@@ -7,7 +7,11 @@ describe('sidebar/components/hooks/use-root-thread', () => {
 
   beforeEach(() => {
     fakeStore = {
-      threadState: sinon.stub(),
+      allAnnotations: sinon.stub().returns(['1', '2']),
+      filterQuery: sinon.stub().returns('itchy'),
+      route: sinon.stub().returns('66'),
+      selectionState: sinon.stub().returns({ hi: 'there' }),
+      userFilter: sinon.stub().returns('hotspur'),
     };
     fakeThreadAnnotations = sinon.stub();
 
@@ -23,11 +27,14 @@ describe('sidebar/components/hooks/use-root-thread', () => {
 
   it('should return results of `threadAnnotations` with current thread state', () => {
     fakeThreadAnnotations.returns('a tisket, a tasket');
-    fakeStore.threadState.returns('current-thread-state');
 
     const results = useRootThread();
 
-    assert.calledWith(fakeThreadAnnotations, 'current-thread-state');
+    const threadState = fakeThreadAnnotations.getCall(0).args[0];
+    assert.deepEqual(threadState.annotations, ['1', '2']);
+    assert.equal(threadState.selection.filterQuery, 'itchy');
+    assert.equal(threadState.route, '66');
+    assert.equal(threadState.selection.filters.user, 'hotspur');
     assert.equal(results, 'a tisket, a tasket');
   });
 });

src/sidebar/components/hooks/use-root-thread.js

@@ -10,5 +10,23 @@ import threadAnnotations from '../../util/thread-annotations';
  * @return {Thread}
  */
 export default function useRootThread() {
-  return useStore(store => threadAnnotations(store.threadState()));
+  const annotations = useStore(store => store.allAnnotations());
+  const query = useStore(store => store.filterQuery());
+  const route = useStore(store => store.route());
+  const selectionState = useStore(store => store.selectionState());
+  const filters = useStore(store => {
+    /** @type {Object.<string,string>} */
+    const filters = {};
+    const userFilter = store.userFilter();
+    if (userFilter) {
+      filters.user = userFilter;
+    }
+    return filters;
+  });
+
+  return threadAnnotations({
+    annotations,
+    route,
+    selection: { ...selectionState, filterQuery: query, filters },
+  });
 }

src/sidebar/store/modules/annotations.js

@@ -412,6 +412,15 @@ const annotationCount = createSelector(
   annotations => countIf(annotations, metadata.isAnnotation)
 );
 
+/**
+ * Retrieve all annotations currently in the store
+ *
+ * @type {(state: any) => Annotation[]}
+ */
+function allAnnotations(state) {
+  return state.annotations;
+}
+
 /**
  * Does the annotation indicated by `id` exist in the collection?
  *
@@ -559,6 +568,7 @@ function savedAnnotations(state) {
 
  *
  * // Selectors
+ * @prop {() => Annotation[]} allAnnotations
  * @prop {() => number} annotationCount
  * @prop {(id: string) => boolean} annotationExists
  * @prop {(id: string) => Annotation} findAnnotationByID
@@ -591,6 +601,7 @@ export default {
   },
 
   selectors: {
+    allAnnotations,
     annotationCount,
     annotationExists,
     findAnnotationByID,

src/sidebar/store/modules/filters.js

@@ -182,6 +182,13 @@ const focusState = createSelector(
   }
 );
 
+/**
+ * Retrieve any applied user filter
+ */
+function userFilter(state) {
+  return state.focus.active ? state.focus.user.filter : null;
+}
+
 /**
  * @typedef FiltersStore
  *
@@ -193,6 +200,7 @@ const focusState = createSelector(
  * // Selectors
  * @prop {() => string|null} filterQuery
  * @prop {() => FocusState} focusState
+ * @prop {() => string|null} userFilter
  *
  */
 
@@ -208,5 +216,6 @@ export default {
   selectors: {
     filterQuery,
     focusState,
+    userFilter,
   },
 };

src/sidebar/store/modules/selection.js

@@ -8,17 +8,12 @@
  */
 
 /**
- * @typedef ThreadState
- * @prop {Annotation[]} annotations
- * @prop {Object} selection
- *   @prop {Object<string,boolean>} selection.expanded
- *   @prop {string|null} selection.filterQuery
- *   @prop {Object<string,string>} selection.filters
- *   @prop {string[]} selection.forcedVisible
- *   @prop {string[]} selection.selected
- *   @prop {string} selection.sortKey
- *   @prop {'annotation'|'note'|'orphan'} selection.selectedTab
- * @prop {string} route
+ * @typedef SelectionState
+ *   @prop {Object<string,boolean>} expanded
+ *   @prop {string[]} forcedVisible
+ *   @prop {string[]} selected
+ *   @prop {string} sortKey
+ *   @prop {'annotation'|'note'|'orphan'} selectedTab
  */
 
 import { createSelector } from 'reselect';
@@ -357,6 +352,17 @@ function selectedTab(state) {
   return state.selectedTab;
 }
 
+function selectionState(state) {
+  const selectionState = {
+    expanded: expandedMap(state),
+    forcedVisible: forcedVisibleAnnotations(state),
+    selected: selectedAnnotations(state),
+    sortKey: sortKey(state),
+    selectedTab: selectedTab(state),
+  };
+  return selectionState;
+}
+
 /**
  * Retrieve the current sort option key
  *
@@ -382,44 +388,6 @@ function sortKeys(state) {
 
 /* Selectors that take root state */
 
-/**
- * Retrieve state needed to calculate the root thread
- *
- * TODO: Refactor
- *  - Remove route entirely and make that logic the responsibility of a caller
- *  - Remove all filter-related properties. Those should come from `filterState`
- *  - Likely remove `annotations` as well
- *  - Likely rename this to `selectionState`, and it may not need to be a
- *    `rootSelector`
- *
- * @type {(rootState: any) => ThreadState}
- */
-const threadState = createSelector(
-  rootState => rootState.annotations.annotations,
-  rootState => rootState.route.name,
-  rootState => rootState.selection,
-  rootState => rootState.filters,
-  (annotations, routeName, selection, filters) => {
-    const setFilters = /** @type {Object.<string,string>} */ ({});
-    // TODO FIXME
-    const userFilter = filters.focus.active && filters.focus.user.filter;
-    if (userFilter) {
-      setFilters.user = userFilter;
-    }
-    // TODO remove filterQuery, filters from this returned object
-    const selectionState = {
-      expanded: expandedMap(selection),
-      filterQuery: filters.query,
-      filters: setFilters,
-      forcedVisible: forcedVisibleAnnotations(selection),
-      selected: selectedAnnotations(selection),
-      sortKey: sortKey(selection),
-      selectedTab: selectedTab(selection),
-    };
-    return { annotations, route: routeName, selection: selectionState };
-  }
-);
-
 /**
  * Is any sort of filtering currently applied to the list of annotations? This
  * includes a search query, but also if annotations are selected or a user
@@ -462,12 +430,12 @@ const hasAppliedFilter = createSelector(
  * @prop {() => boolean} hasSelectedAnnotations
  * @prop {() => string[]} selectedAnnotations
  * @prop {() => string} selectedTab
+ * @prop {() => SelectionState} selectionState
  * @prop {() => string} sortKey
  * @prop {() => string[]} sortKeys
  *
  * // Root Selectors
  * @prop {() => boolean} hasAppliedFilter
- * @prop {() => ThreadState} threadState
  *
  */
 
@@ -493,12 +461,12 @@ export default {
     hasSelectedAnnotations,
     selectedAnnotations,
     selectedTab,
+    selectionState,
     sortKey,
     sortKeys,
   },
 
   rootSelectors: {
     hasAppliedFilter,
-    threadState,
   },
 };

src/sidebar/store/modules/test/annotations-test.js

@@ -318,6 +318,19 @@ describe('sidebar/store/modules/annotations', function () {
     });
   });
 
+  describe('allAnnotations', () => {
+    it('returns all the annotations in the store', () => {
+      const store = createTestStore();
+      const annotation1 = fixtures.oldPageNote();
+      const annotation2 = fixtures.defaultAnnotation();
+      store.addAnnotations([annotation1, annotation2]);
+      assert.deepEqual(store.allAnnotations(), [
+        store.findAnnotationByID(annotation1.id),
+        store.findAnnotationByID(annotation2.id),
+      ]);
+    });
+  });
+
   describe('orphanCount', () => {
     it('returns number of orphaned annotations', () => {
       const orphan = Object.assign(fixtures.oldAnnotation(), { $orphan: true });

src/sidebar/store/modules/test/filters-test.js

-// import uiConstants from '../../../ui-constants';
 import createStore from '../../create-store';
 import filters from '../filters';
 import selection from '../selection';
-// import * as fixtures from '../../../test/annotation-fixtures';
 
 describe('sidebar/store/modules/filters', () => {
   let store;
@@ -115,4 +113,18 @@ describe('sidebar/store/modules/filters', () => {
       });
     });
   });
+
+  describe('userFilter', () => {
+    it('returns null if user focus inactive', () => {
+      assert.isNull(store.userFilter());
+    });
+
+    it('returns current user filter when user focus active', () => {
+      store.changeFocusModeUser({
+        username: 'filbert',
+        displayName: 'Pantomime Nutball',
+      });
+      assert.equal(store.userFilter(), 'filbert');
+    });
+  });
 });

src/sidebar/store/modules/test/selection-test.js

@@ -120,28 +120,12 @@ describe('sidebar/store/modules/selection', () => {
     });
   });
 
-  describe('threadState', () => {
-    it('returns the current annotations in rootState', () => {
-      const myAnnotation = fixtures.defaultAnnotation();
-      store.addAnnotations([myAnnotation]);
-
-      // `addAnnotations` injects some additional properties to annotations,
-      // so we can't compare objects
-      assert.equal(store.threadState().annotations[0].id, myAnnotation.id);
-      assert.lengthOf(store.threadState().annotations, 1);
-    });
-
-    it('returns the current route name from rootState', () => {
-      store.changeRoute('kamchatka');
-
-      assert.equal(store.threadState().route, 'kamchatka');
-    });
-
+  describe('selectionState', () => {
     it('returns relevant state about tab and sort', () => {
       store.selectTab('orphan');
       store.setSortKey('pyrrhic');
 
-      const selection = store.threadState().selection;
+      const selection = store.selectionState();
 
       assert.equal(selection.selectedTab, 'orphan');
       assert.equal(selection.sortKey, 'pyrrhic');
@@ -151,30 +135,14 @@ describe('sidebar/store/modules/selection', () => {
       store.selectAnnotations(['1', '2']);
       store.setExpanded('3', true);
       store.setExpanded('4', false);
+      store.setForcedVisible('5', true);
+      store.setForcedVisible('6', true);
 
-      const selection = store.threadState().selection;
+      const selection = store.selectionState();
 
       assert.deepEqual(selection.expanded, { 3: true, 4: false });
       assert.deepEqual(selection.selected, ['1', '2']);
-    });
-
-    it('returns the relevant state when user-focus mode is applied', () => {
-      store.changeFocusModeUser({
-        username: 'testuser',
-        displayName: 'Test User',
-      });
-
-      const selection = store.threadState().selection;
-
-      assert.deepEqual(selection.filters, { user: 'testuser' });
-    });
-
-    it('returns the relevant state when a filter query is applied', () => {
-      store.setFilterQuery('frappe');
-
-      const selection = store.threadState().selection;
-
-      assert.equal(selection.filterQuery, 'frappe');
+      assert.deepEqual(selection.forcedVisible, ['5', '6']);
     });
   });
 

src/sidebar/test/integration/threading-test.js

-import { Injector } from '../../../shared/injector';
+import { mount } from 'enzyme';
+import { createElement } from 'preact';
+import { act } from 'preact/test-utils';
 
+import { Injector } from '../../../shared/injector';
 import storeFactory from '../../store';
-import immutable from '../../util/immutable';
-import threadAnnotations from '../../util/thread-annotations';
 
-const fixtures = immutable({
+import { ServiceContext } from '../../util/service-context';
+import useRootThread from '../../components/hooks/use-root-thread';
+
+const fixtures = {
   annotations: [
     {
       $orphan: false,
@@ -30,9 +34,10 @@ const fixtures = immutable({
       updated: 100,
     },
   ],
-});
+};
 
-describe('annotation threading', function () {
+describe('integration: annotation threading', () => {
+  let lastRootThread;
   let store;
 
   beforeEach(function () {
@@ -46,25 +51,46 @@ describe('annotation threading', function () {
       .register('features', { value: fakeFeatures })
       .register('settings', { value: {} });
 
+    // Mount a dummy component to be able to use the `useRootThread` hook
+    // Do things that cause `useRootThread` to recalculate in the store and
+    // test them (hint: use `act`)
+    function DummyComponent() {
+      lastRootThread = useRootThread();
+    }
+
     store = container.get('store');
+    // Wrap the dummy component in a context so that it has access to the store
+    mount(
+      <ServiceContext.Provider value={container}>
+        <DummyComponent />
+      </ServiceContext.Provider>
+    );
   });
 
-  it('should display newly loaded annotations', function () {
+  it('should display newly loaded annotations', () => {
+    act(() => {
       store.addAnnotations(fixtures.annotations);
-    assert.equal(threadAnnotations(store.threadState()).children.length, 2);
     });
 
-  it('should not display unloaded annotations', function () {
+    assert.equal(lastRootThread.children.length, 2);
+  });
+
+  it('should not display unloaded annotations', () => {
+    act(() => {
       store.addAnnotations(fixtures.annotations);
       store.removeAnnotations(fixtures.annotations);
-    assert.equal(threadAnnotations(store.threadState()).children.length, 0);
+    });
+    assert.equal(lastRootThread.children.length, 0);
   });
 
-  it('should filter annotations when a search is set', function () {
+  it('should filter annotations when a search is set', () => {
+    act(() => {
       store.addAnnotations(fixtures.annotations);
       store.setFilterQuery('second');
-    assert.equal(threadAnnotations(store.threadState()).children.length, 1);
-    assert.equal(threadAnnotations(store.threadState()).children[0].id, '2');
+    });
+
+    assert.equal(lastRootThread.children.length, 1);
+    assert.equal(lastRootThread.children[0].id, '2');
   });
 
   [
@@ -77,14 +103,15 @@ describe('annotation threading', function () {
       expectedOrder: ['2', '1'],
     },
   ].forEach(testCase => {
-    it(`should sort annotations by ${testCase.mode}`, () => {
+    it(`should sort annotations by ${testCase.sortKey}`, () => {
+      act(() => {
         store.addAnnotations(fixtures.annotations);
         store.setSortKey(testCase.sortKey);
-      const actualOrder = threadAnnotations(store.threadState()).children.map(
-        function (thread) {
+      });
+
+      const actualOrder = lastRootThread.children.map(thread => {
         return thread.annotation.id;
-        }
-      );
+      });
       assert.deepEqual(actualOrder, testCase.expectedOrder);
     });
   });

src/sidebar/util/thread-annotations.js

@@ -5,9 +5,23 @@ import { generateFacetedFilter } from './search-filter';
 import filterAnnotations from './view-filter';
 import { shouldShowInTab } from './tabs';
 
+/** @typedef {import('../../types/api').Annotation} Annotation */
 /** @typedef {import('./build-thread').Thread} Thread */
 /** @typedef {import('./build-thread').Options} BuildThreadOptions */
-/** @typedef {import('../store/modules/selection').ThreadState} ThreadState */
+
+/**
+ * @typedef ThreadState
+ * @prop {Annotation[]} annotations
+ * @prop {Object} selection
+ *   @prop {Object<string,boolean>} selection.expanded
+ *   @prop {string|null} selection.filterQuery
+ *   @prop {Object<string,string>} selection.filters
+ *   @prop {string[]} selection.forcedVisible
+ *   @prop {string[]} selection.selected
+ *   @prop {string} selection.sortKey
+ *   @prop {'annotation'|'note'|'orphan'} selection.selectedTab
+ * @prop {string|null} route
+ */
 
 // Sort functions keyed on sort option
 const sortFns = {