POC: Improve filter behavior when creating new annotations or replies

[lyzadanger]

This draft PR is a POC implementation of some changes to some behavior when creating new annotations or replies when there is one or more applied filter in the client (selected annotations, query filter, or focused-on-a-user). There is some complexity here, so lots of words.

---

Motivation

The main tenets here is that new annotations (or replies) created while filter (s) is/are applied should:

• Be guaranteed to be visible to the user, even after saving to the service
• Not yank the user out of the filtered mode entirely
• Be explained in the context of the currently-applied filter(s)

A ticket was opened (#2324) recently indicating what I believe to be a not-uncommon use case: a user selects an annotation (or annotations) by clicking on highlighted text in the page and then wants to respond to that annotation. Yanking them fully out of the “selected-filter” mode is jarring and confusing. Likewise, performing a search that results in a bunch of excluded replies in a nested thread and then replying to one of the visible results: because this yanks you out of the filter entirely, it has the nasty effect of expanding a bunch of hidden reply threads and confusing the user as to the context of the reply.

In the case of user-focus mode, the situation is, I’d argue, more problematic: if a user who is not the focused-upon-user attempts to reply to one of that user’s annotations, their reply is immediately hidden! That’s bad.

As a result of these changes, creating a new annotation or replying to an annotation when in a filtered mode does not reset filters, and that annotation/reply will stay visible until filters are (eventually) reset.

Other changes here are in support of that behavior.

I’m looking primarily for feedback on the behavior, not the technical implementation, which is messy for the moment.

Out of scope for the moment, but valid needs:

• What happens when editing an annotation that matches the current filters, but the edited annotation no longer matches the filters.
• Behavior when creating a new highlight in any of these filtered contexts.
• Visual design improvements for delineating which annotations/replies match the current filters and which don't

Before and After

When filter query is active

Creating a new annotation

Before:

After:

Creating a reply

Before:

After:

Screenshot of result, after:

When there are selected annotations

Creating an annotation

Before:

After:

Screen shot, after:

Creating a reply

Before:

After:

When user focus mode is active

Creating an annotation

Before:

After:

Creating a reply

Before:

After:

Technical details

Making newly-created annotations and replies reliably show up without resetting filtering entirely involves updating the forcedVisible collection in the selection store instead of using clearSelection on annotation/reply creation, and making sure that forcedVisible annotations are respected and presented in the proper contexts (forcedVisible collections are reset upon clearing selections/filters).

Some wrinkles:

• New annotations have a locally-set $tag property but do not yet have an id. This $tag needs to be added to the forcedVisible collection initially to assure the new annotation/reply remains visible, but after save, that entry needs to be updated to use the server-assigned id such that it matches to a real thread in the rootThread. If this sounds odd (i.e. “can’t we use $tag or id interchangeably here?”) it’s a little complex to explain the “no” answer in short summary, but I’m happy to elaborate, preferably with voices. These changes implement this updating from $tag -> $id in the selection module (hastily, without elegance; I’ll refactor).
• There has been an oversight in the selection module’s reducer for REMOVE_ANNOTATIONS. It has been correctly removing removed annotation ids from the set of selected annotations, but had not been removing references to removed annotations from forcedVisible and expanded. This was essentially a bug, but didn’t manifest until some of the other changes on this branch happened.
• Yeah, I can see that FilterStatus really needs to be refactored into multiple components. It’s getting too brittle and complex. There will be some duplicated logic, likely, between those components, but, eh.
• These changes introduce a new filter state: “selected annotations with forced-visible threads” (as newly-created annotations/replies are considered forced-visible). This has necessitated a bit of nuance around some wording in the FilterStatus component. And it also highlights the difference between how we count “annotations” in different modes (not a new problem). In non-filtered modes and when there are selected annotations, only top-level annotations are “counted”: the counts next to tabs and in the “show all” button when viewing selected annotations exclude replies. Counts when filtering by query or user, however, include both annotations and replies. I’ve been trying to manage juggling this as best I can to this point…

Next Steps

• Gather feedback on behavior here, if anyone has any
• Clean up implementation
• Refactor FilterStatus
• Update filter-states UX documentation to account for any changes that land

[robertknight]

Be guaranteed to be visible to the user, even after saving to the service
Not yank the user out of the filtered mode entirely
Be explained in the context of the currently-applied filter(s)

These principles make complete sense.

This $tag needs to be added to the forcedVisible collection initially to assure the new annotation/reply remains visible, but after save, that entry needs to be updated to use the server-assigned id such that it matches to a real thread in the rootThread.

Maybe forcedVisible should actually be a collection of tags rather than IDs, since all annotations have tags but not all annotations have server-assigned IDs. It looks like the contents of this collection are only examined or added in a few locations that could be changed to work with tags instead:

client/src/sidebar/util/build-thread.js

Line 347 in 4c477b2

opts.forcedVisible.indexOf(annotationId(thread.annotation)) !== -1

client/src/sidebar/services/threads.js

Line 20 in 4c477b2

store.setForcedVisible(thread.id, true);

[lyzadanger]

Maybe forcedVisible should actually be a collection of tags rather than IDs, since all annotations have tags but not all annotations have server-assigned IDs. It looks like the contents of this collection are only examined or added in a few locations that could be changed to work with tags instead:

I don't think the change you suggested would quite work, as annotationId(...) will prefer id to $tag and will return id one exists. That wouldn't match the $tag in the forcedVisible collection. Again, this affects newly-saved annotations, which would have only had a $tag but "now" have a $tag and an id.

(p.s. Your approach was what I tried first but then ran into the issue I described).

(Edited: On third thought, I think I see that the change is perhaps simpler than I'm thinking it is because, as you mention, the contents are only written or "used" in a couple of places. I shall consider!).

[lyzadanger]

Updated to simplify forcedVisible handling: it's all $tags now. Because the selection store module deals with client-side concerns only, it probably makes sense to make all of its collections deal in $tags.

[LMS007]

From just a behavoir standpoint, I attempted to try various combinations of searching, filtering, editing deleting etc. Everything looks reasonable and works intuitively. This is much better behavoir than what currently exists. I think if the the code changes are substantially small, then I see no reason to not move this forward with haste. 👍

[lyzadanger]

Proper PR here: #2437