Address tech debt in `selection` pertaining to tabs and sort options

- Remove logic yanking user away from orphans tab when clearing a
  selection
- Make sure all `selection` module actions use `setTab` when changing
  tab value
- Simplify tab constants
- Remove `sortKeysAvailable` from state and add selector instead

src/sidebar/components/sort-menu.js

@@ -17,9 +17,7 @@ export default function SortMenu() {
   const sortKey = useStore(store => store.getState().selection.sortKey);
   // All available sorting options. These change depending on current
   // "tab" or context.
-  const sortKeysAvailable = useStore(
-    store => store.getState().selection.sortKeysAvailable
-  );
+  const sortKeysAvailable = useStore(store => store.sortKeys());
 
   const menuItems = sortKeysAvailable.map(sortOption => {
     return (

src/sidebar/components/test/sort-menu-test.js

@@ -18,12 +18,12 @@ describe('SortMenu', () => {
     fakeState = {
       selection: {
         sortKey: 'Location',
-        sortKeysAvailable: ['Newest', 'Oldest', 'Location'],
       },
     };
     fakeStore = {
       setSortKey: sinon.stub(),
       getState: sinon.stub().returns(fakeState),
+      sortKeys: sinon.stub().returns(['Newest', 'Oldest', 'Location']),
     };
 
     $imports.$mock(mockImportedComponents());
@@ -41,8 +41,8 @@ describe('SortMenu', () => {
 
     const menuItems = wrapper.find('MenuItem');
 
-    assert.lengthOf(menuItems, fakeState.selection.sortKeysAvailable.length);
-    fakeState.selection.sortKeysAvailable.forEach(sortKey => {
+    assert.lengthOf(menuItems, fakeStore.sortKeys().length);
+    fakeStore.sortKeys().forEach(sortKey => {
       assert.lengthOf(
         menuItems.filterWhere(menuItem => menuItem.prop('label') === sortKey),
         1

src/sidebar/services/test/root-thread-test.js

@@ -43,8 +43,6 @@ describe('rootThread', function () {
         selection: {
           filterQuery: null,
           highlighted: [],
-          sortKey: 'Location',
-          sortKeysAvailable: ['Location'],
         },
         route: {
           name: 'sidebar',
@@ -214,7 +212,6 @@ describe('rootThread', function () {
       it(`sort order is correct when sorting by ${testCase.order}`, () => {
         fakeBuildThread.reset();
         fakeStore.state.selection.sortKey = testCase.order;
-        fakeStore.state.selection.sortKeysAvailable = [testCase.order];
 
         rootThread.thread(fakeStore.state);
         const sortCompareFn = fakeBuildThread.args[0][1].sortCompareFn;

src/sidebar/store/modules/selection.js

@@ -16,34 +16,14 @@ import * as metadata from '../../util/annotation-metadata';
 import { countIf } from '../../util/array';
 import * as util from '../util';
 
-/**
- * Default starting tab.
- */
-const TAB_DEFAULT = uiConstants.TAB_ANNOTATIONS;
-
 /**
  * Default sort keys for each tab.
  */
-const TAB_SORTKEY_DEFAULT = {};
-TAB_SORTKEY_DEFAULT[uiConstants.TAB_ANNOTATIONS] = 'Location';
-TAB_SORTKEY_DEFAULT[uiConstants.TAB_NOTES] = 'Oldest';
-TAB_SORTKEY_DEFAULT[uiConstants.TAB_ORPHANS] = 'Location';
-
-/**
- * Available sort keys for each tab.
- */
-const TAB_SORTKEYS_AVAILABLE = {};
-TAB_SORTKEYS_AVAILABLE[uiConstants.TAB_ANNOTATIONS] = [
-  'Newest',
-  'Oldest',
-  'Location',
-];
-TAB_SORTKEYS_AVAILABLE[uiConstants.TAB_NOTES] = ['Newest', 'Oldest'];
-TAB_SORTKEYS_AVAILABLE[uiConstants.TAB_ORPHANS] = [
-  'Newest',
-  'Oldest',
-  'Location',
-];
+const TAB_SORTKEY_DEFAULT = {
+  [uiConstants.TAB_ANNOTATIONS]: 'Location',
+  [uiConstants.TAB_NOTES]: 'Oldest',
+  [uiConstants.TAB_ORPHANS]: 'Location',
+};
 
 /**
  * Utility function that returns all of the properties of an object whose
@@ -80,29 +60,6 @@ function initialSelection(settings) {
   return selection;
 }
 
-const setTab = (selectedTab, newTab) => {
-  // Do nothing if the "new tab" is not a valid tab.
-  if (
-    [
-      uiConstants.TAB_ANNOTATIONS,
-      uiConstants.TAB_NOTES,
-      uiConstants.TAB_ORPHANS,
-    ].indexOf(newTab) === -1
-  ) {
-    return {};
-  }
-  // Shortcut if the tab is already correct, to avoid resetting the sortKey
-  // unnecessarily.
-  if (selectedTab === newTab) {
-    return {};
-  }
-  return {
-    selectedTab: newTab,
-    sortKey: TAB_SORTKEY_DEFAULT[newTab],
-    sortKeysAvailable: TAB_SORTKEYS_AVAILABLE[newTab],
-  };
-};
-
 function init(settings) {
   return {
     /**
@@ -138,7 +95,6 @@ function init(settings) {
     highlighted: {},
 
     filterQuery: settings.query || null,
-    selectedTab: TAB_DEFAULT,
     focusMode: {
       enabled: settings.hasOwnProperty('focus'),
       focused: true,
@@ -146,25 +102,30 @@ function init(settings) {
       config: { ...(settings.focus ? settings.focus : {}) },
     },
 
+    selectedTab: uiConstants.TAB_ANNOTATIONS,
     // Key by which annotations are currently sorted.
-    sortKey: TAB_SORTKEY_DEFAULT[TAB_DEFAULT],
-    // Keys by which annotations can be sorted.
-    sortKeysAvailable: TAB_SORTKEYS_AVAILABLE[TAB_DEFAULT],
+    sortKey: TAB_SORTKEY_DEFAULT[uiConstants.TAB_ANNOTATIONS],
   };
 }
 
-const update = {
-  CLEAR_SELECTION: function (state) {
-    let selectedTab = state.selectedTab;
-    if (selectedTab === uiConstants.TAB_ORPHANS) {
-      selectedTab = uiConstants.TAB_ANNOTATIONS;
+const setTab = (newTab, oldTab) => {
+  // Do nothing if the "new tab" is not a valid tab or is the same as the
+  // tab already selected. This will avoid resetting the `sortKey`, too.
+  if (!uiConstants.TABS.includes(newTab) || oldTab === newTab) {
+    return {};
   }
-    const tabSettings = setTab(state.selectedTab, selectedTab);
+  return {
+    selectedTab: newTab,
+    sortKey: TAB_SORTKEY_DEFAULT[newTab],
+  };
+};
+
+const update = {
+  CLEAR_SELECTION: function () {
     return {
       filterQuery: null,
       forcedVisible: {},
       selected: {},
-      ...tabSettings,
     };
   },
 
@@ -235,7 +196,7 @@ const update = {
   },
 
   SELECT_TAB: function (state, action) {
-    return setTab(state.selectedTab, action.tab);
+    return setTab(action.tab, state.selectedTab);
   },
 
   /**
@@ -251,7 +212,7 @@ const update = {
 
     const haveOnlyPageNotes = noteCount === topLevelAnnotations.length;
     if (action.currentAnnotationCount === 0 && haveOnlyPageNotes) {
-      return { selectedTab: uiConstants.TAB_NOTES };
+      return setTab(uiConstants.TAB_NOTES, state.selectedTab);
     }
     return {};
   },
@@ -263,16 +224,18 @@ const update = {
         delete selection[annotation.id];
       }
     });
-    let selectedTab = state.selectedTab;
+    let newTab = state.selectedTab;
+    // If the orphans tab is selected but no remaining annotations are orphans,
+    // switch back to annotations tab
     if (
-      selectedTab === uiConstants.TAB_ORPHANS &&
+      newTab === uiConstants.TAB_ORPHANS &&
       countIf(action.remainingAnnotations, metadata.isOrphan) === 0
     ) {
-      selectedTab = uiConstants.TAB_ANNOTATIONS;
+      newTab = uiConstants.TAB_ANNOTATIONS;
     }
     return {
+      ...setTab(newTab, state.selectedTab),
       selected: selection,
-      selectedTab: selectedTab,
     };
   },
 
@@ -379,11 +342,15 @@ function highlightAnnotations(ids) {
   };
 }
 
-/** Set the type annotations to be displayed. */
-function selectTab(type) {
+/**
+ * Set the currently-selected tab to `tabKey`.
+ *
+ * @param {'annotation'|'note'|'orphan'} tabKey
+ */
+function selectTab(tabKey) {
   return {
     type: actions.SELECT_TAB,
-    tab: type,
+    tab: tabKey,
   };
 }
 
@@ -586,6 +553,20 @@ const selectedAnnotations = createSelector(
   selection => trueKeys(selection)
 );
 
+/**
+ * Retrieve applicable sort options for the currently-selected tab.
+ *
+ * @return {string[]}
+ */
+function sortKeys(state) {
+  const sortKeysForTab = ['Newest', 'Oldest'];
+  if (state.selection.selectedTab !== uiConstants.TAB_NOTES) {
+    // Location is inapplicable to Notes tab
+    sortKeysForTab.push('Location');
+  }
+  return sortKeysForTab;
+}
+
 export default {
   init: init,
   namespace: 'selection',
@@ -622,5 +603,6 @@ export default {
     hasAppliedFilter,
     hasSelectedAnnotations,
     selectedAnnotations,
+    sortKeys,
   },
 };

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

@@ -423,28 +423,17 @@ describe('sidebar/store/modules/selection', () => {
 
     it('allows sorting annotations by time and document location', function () {
       store.selectTab(uiConstants.TAB_ANNOTATIONS);
-      assert.deepEqual(getSelectionState().sortKeysAvailable, [
-        'Newest',
-        'Oldest',
-        'Location',
-      ]);
+      assert.deepEqual(store.sortKeys(), ['Newest', 'Oldest', 'Location']);
     });
 
     it('allows sorting page notes by time', function () {
       store.selectTab(uiConstants.TAB_NOTES);
-      assert.deepEqual(getSelectionState().sortKeysAvailable, [
-        'Newest',
-        'Oldest',
-      ]);
+      assert.deepEqual(store.sortKeys(), ['Newest', 'Oldest']);
     });
 
     it('allows sorting orphans by time and document location', function () {
       store.selectTab(uiConstants.TAB_ORPHANS);
-      assert.deepEqual(getSelectionState().sortKeysAvailable, [
-        'Newest',
-        'Oldest',
-        'Location',
-      ]);
+      assert.deepEqual(store.sortKeys(), ['Newest', 'Oldest', 'Location']);
     });
 
     it('sorts annotations by document location by default', function () {

src/sidebar/store/test/index-test.js

@@ -87,25 +87,6 @@ describe('store', function () {
       assert.equal(store.getState().selection.sortKey, 'Location');
     });
 
-    it('sets `sortKeysAvailable` to available annotation sort keys if set to Orphans', () => {
-      store.selectTab(uiConstants.TAB_ORPHANS);
-      store.clearSelection();
-      assert.deepEqual(store.getState().selection.sortKeysAvailable, [
-        'Newest',
-        'Oldest',
-        'Location',
-      ]);
-    });
-
-    it('sets `selectedTab` to Annotations if set to Orphans', () => {
-      store.selectTab(uiConstants.TAB_ORPHANS);
-      store.clearSelection();
-      assert.equal(
-        store.getState().selection.selectedTab,
-        uiConstants.TAB_ANNOTATIONS
-      );
-    });
-
     it('does not change `selectedTab` if set to something other than Orphans', () => {
       store.selectTab(uiConstants.TAB_NOTES);
       store.clearSelection();

src/sidebar/ui-constants.js

@@ -6,6 +6,7 @@ export default {
   PANEL_HELP: 'help',
   PANEL_LOGIN_PROMPT: 'loginPrompt',
   PANEL_SHARE_ANNOTATIONS: 'shareGroupAnnotations',
+  TABS: ['annotation', 'note', 'orphan'],
   TAB_ANNOTATIONS: 'annotation',
   TAB_NOTES: 'note',
   TAB_ORPHANS: 'orphan',