Add user filtering to the Notebook

[lyzadanger]

This is a draft PR of all the bits and pieces needed to add rudimentary user filtering to the Notebook. What I'd like to ascertain with this draft PR is:

• General approach OK?
• Would reviewer preference be to break up this into smaller PRs? Each commit here is self-sufficient, so it can be chunked up in arbitrary ways. (Decided: keep as one PR)

A Few Screenshots

On launch (not in user-focus mode), Notebook looks like this:

When filtering by user, the count of matches changes to use a "results" unit:

"Force-expanding" a non-matching thread will update the results count similar to in the sidebar:

When user-focus mode is active (e.g. in grading mode), opening the Notebook will filter by the currently-focused user initially:

(but the user can change which user to filter by, or set it to "Everybody").

A List

You should expect these changes to accomplish the following:

• A user filter should appear in the notebook. This simple filter shows one user per "line"
• On initial opening, the user filter should indicate that “Everybody” ’s annotations are showing
• The user filter uses the "profile" icon to indicate its user-y-ness (but also has an accessible title)
• The user menu should include all users who have at least one annotation in the current group
• Users should be in alphabetical order
  • If the client_display_names feature flag is enabled, the user’s display names will display, and will be alphabetized
  • If not, the names shown will be (alphabetized) usernames
• Selecting a user from the filter menu should filter the annotations shown to annotations by that user
  • The count of matches should change from X threads (y annotations) to X results (or 1 result if singular)
  • The user selected should be represented in the filter label
  • Opening the filter again should show the user visibly selected
  • Clicking on any “Show x more in conversation” buttons will update the match count to X results (and y more) similarly to how the sidebar handles this state
• Selecting “Everybody” from the user filter should show all annotations in the group again
• When in a user-focused mode:
  • On Notebook launch, that focused user will be selected in the user filter and their annotations shown
  • The focused user will show up in the user filter even if that user has no annotations in the current group
  • Other users, or “Everybody” can be selected as above to change the user filtering

What's not included

Literally everything else, including things like URL hash/history management, handling really long lists of users, handling empty result sets more gracefully, anything but the most elementary visual styling, etc., etc.

Edited: Moving out of draft now

Part of #2731

[lyzadanger]

@robertknight I've removed one commit from this PR—the "fix" to the totalChildren property in the build-thread module, and am just using rootThread.children.length. Diff is slightly smaller now.

[codecov]

Codecov Report

Merging #2835 (2aaef59) into master (898d237) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2835      +/-   ##
==========================================
+ Coverage   97.72%   97.74%   +0.01%     
==========================================
  Files         202      206       +4     
  Lines        7615     7670      +55     
  Branches     1683     1694      +11     
==========================================
+ Hits         7442     7497      +55     
  Misses        173      173              

Impacted Files	Coverage Δ
src/sidebar/components/notebook-view.js	100.00% <ø> (ø)
src/sidebar/components/filter-select.js	100.00% <100.00%> (ø)
src/sidebar/components/hooks/use-filter-options.js	100.00% <100.00%> (ø)
src/sidebar/components/notebook-filters.js	100.00% <100.00%> (ø)
src/sidebar/components/notebook-result-count.js	100.00% <100.00%> (ø)
src/sidebar/store/modules/filters.js	100.00% <100.00%> (ø)
src/sidebar/store/modules/selection.js	100.00% <100.00%> (ø)
... and 1 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 898d237...2aaef59. Read the comment docs.

[robertknight]

Overall this looks good and I was able to follow it quite easily. I had some requests for changes on various minor details and I also suggested slight name changes to some of the components to better fit the pattern of other components.

[robertknight]

Since this logic only depends on state from the current module, it could potentially be handled in the reducer as well. In other words, the SET_FILTER reducer would combine the results of the SET_FOCUS_MODE reducer with whatever other changes it was going to make.

[lyzadanger]

For some reason I'm having trouble wrapping my head around what you're envisioning, would you mind elaborating a little bit?

[robertknight]

If you want one action to incorporate the effects of another, then one option is to use a thunk with dispatch as you have done here. Another is to call one reducer from within another:

diff --git a/src/sidebar/store/modules/filters.js b/src/sidebar/store/modules/filters.js
index e36e7eff5..c47b6f5a8 100644
--- a/src/sidebar/store/modules/filters.js
+++ b/src/sidebar/store/modules/filters.js
@@ -119,6 +119,15 @@ const update = {
   },
 
   SET_FILTER: function (state, action) {
+    // If there is a filter conflict with focusFilters, deactivate focus
+    // mode to prevent unintended collisions and let the new filter value
+    // take precedence.
+    let newState = /** @type {typeof state} */ ({});
+    if (state.focusFilters?.[action.filterName]) {
+      // eslint-disable-next-line new-cap
+      newState = update.SET_FOCUS_MODE(state, { active: false });
+    }
+
     const updatedFilters = {
       ...state.filters,
       [action.filterName]: action.filterOption,
@@ -127,7 +136,9 @@ const update = {
     if (action.filterOption?.value === '') {
       delete updatedFilters[action.filterName];
     }
-    return { filters: updatedFilters };
+    newState.filters = updatedFilters;
+
+    return newState;
   },
 
   SET_FILTER_QUERY: function (state, action) {
@@ -170,22 +181,7 @@ function changeFocusModeUser(user) {
  * @param {FilterOption} filterOption
  */
 function setFilter(filterName, filterOption) {
-  return (dispatch, getState) => {
-    // If there is a filter conflict with focusFilters, deactivate focus
-    // mode to prevent unintended collisions and let the new filter value
-    // take precedence.
-    if (getState().filters.focusFilters?.[filterName]) {
-      dispatch({
-        type: actions.SET_FOCUS_MODE,
-        active: false,
-      });
-    }
-    dispatch({
-      type: actions.SET_FILTER,
-      filterName,
-      filterOption,
-    });
-  };
+  return { type: actions.SET_FILTER, filterName, filterOption };
 }
 
 /** Set the query used to filter displayed annotations. */

The above pattern has the advantage that the reducers get passed the local state so they don't have to extract it from the root state. I guess the downside is that it's just not a pattern that we currently use a lot.

[lyzadanger]

Admittedly did not know this was even possible, so this has value just as educational content :).

[lyzadanger]

@robertknight Due to the number of modules that got renamed as a result of integrating feedback, etc, I ended up rewriting commit history entirely. I hope this doesn't slow you down.

I addressed all of your feedback items except for one that I left unresolved that I had a confusion over. Thanks!

[robertknight]

This is almost ready to go. A few minor issues I highlighted:

• I suggest making the value property of user filter options always be usernames, to avoid needing to deal with a mix of account IDs and usernames.
• There is a mismatch between CSS class names, .scss file name and Preact component name for one component

Once the above are addressed this is good to merge.

[robertknight]

If the <span> element is not styled in any way then perhaps a Fragment could be used instead.

[lyzadanger]

Can you help me understand the tradeoffs of using a <Fragment> versus using a containing element?

[robertknight]

The main benefit is that it avoids adding clutter to the DOM tree that doesn't serve any purpose for semantic or styling purposes. It's only a small benefit in many cases though.