search

hypothesis / client

The Hypothesis web-based annotation client.
Other
643 stars 197 forks source link

Only apply selector filters to threads, not replies #6562

Closed robertknight closed 1 month ago

robertknight commented 1 month ago

Add a property to filter facets which indicate whether they apply to annotations and replies or only top-level annotations. Use this property to apply the filter either to all annotations or only the root annotation in a thread respectively.

Queries based on the user or annotation body for example can apply to both. Queries based on a selector such as page number, book chapter or quote only make sense as thread-level filters.

Fixes https://github.com/hypothesis/support/issues/158

Testing:

  1. Go to http://localhost:3000/document/vitalsource-epub. Create annotations in chapter one and chapter two and add replies to both
  2. Toggle the "Selected chapter" filter on/off. The filter should be applied to the threads based on whether the root annotation matches the chapter. When the root annotation is shown, replies in the thread should be shown as well.

In Step 2 there is a known issue that the "Show replies" toggle disappears when the filter is active. The reply should nevertheless be visible. In contrast on the main branch, then the "Selected chapter" filter is active, the reply will initially be hidden and shown under "Show N more in conversation".

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.43%. Comparing base (76040e7) to head (0b1e540). Report is 3 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #6562 +/- ## ======================================= Coverage 99.43% 99.43% ======================================= Files 270 270 Lines 10227 10243 +16 Branches 2420 2426 +6 ======================================= + Hits 10169 10185 +16 Misses 58 58 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

robertknight commented 1 month ago

I noticed a weird behavior when testing this, which is that the annotation creation card does not show while the filter is applied:

Ah. It turns out there was another existing issue where thread-level filtering did not take into account the set of "forced visible" threads which are shown even if they don't match the filter. Annotation-level filtering did handle this. See https://github.com/hypothesis/client/pull/6562/commits/0b1e5401366a672846fb6c18cd6b71a96fbfcd25.

There is something else wrong here though, because in this context it shouldn't matter if the annotation is in the "forced visible" set or not. The annotation was created on a page that matches the filter, and the page info is present in the new annotation. Update: The explanation for this is https://github.com/hypothesis/client/pull/6565.