Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(project): update collections search asynchronously #6127

Merged

Conversation

eatyourgreens
Copy link
Contributor

@eatyourgreens eatyourgreens commented Jun 10, 2024

Make the collections search, in the popup collections modal, asynchronous by using a ref to store the current search text. This fixes a bug where search results were not correctly synchronised to the stored search term (in component state.)

If you typed 4 characters into the search box, all your collections were shown, because the component didn't wait for new search results before updating the dropdown menu. Now, it waits for new search results from the Panoptes API before updating.

I've also refactored the minimum required search text length (for full-text search) into MIN_SEARCH_LENGTH, which makes it easier to manage. 3 is the minimum required by Postgres, but fuzzy text searching can give weird results for 4 or 5-character strings, so increasing it might make the component easier to use. If you increase the value, I'd suggest also increasing the same value in zooniverse/Panoptes-Front-End#7124, so that the search behaviour matches in PFE.

Please request review from @zooniverse/frontend team or an individual member of that team.

Package

  • app-project

Linked Issue and/or Talk Post

How to Review

Open the collections modal and try any search that is exactly 4 characters. The list of collections now only shows results returned from the Panoptes full-text-search API.

Note that the API response can still miss collection names that include the search term.

Screen.Recording.2024-06-10.at.18.10.23.mov

Checklist

PR Creator - Please cater the checklist to fit the review needed for your code changes.
PR Reviewer - Use the checklist during your review. Each point should be checkmarked or discussed before PR approval.

General

  • Tests are passing locally and on Github
  • Documentation is up to date and changelog has been updated if appropriate
  • You can yarn panic && yarn bootstrap or docker-compose up --build and FEM works as expected
  • FEM works in all major desktop browsers: Firefox, Chrome, Edge, Safari (Use Browserstack account as needed)
  • FEM works in a mobile browser

General UX

Example Staging Project: i-fancy-cats

  • All pages of a FEM project load: Home Page, Classify Page, and About Pages
  • Can submit a classification
  • Can sign-in and sign-out
  • The component is accessible

Bug Fix

  • The PR creator has listed user actions to use when testing if bug is fixed
  • The bug is fixed
  • Unit tests are added or updated

@eatyourgreens eatyourgreens force-pushed the fix-async-collection-search branch from 485ceef to 408eb9a Compare June 10, 2024 16:09
@coveralls
Copy link

Coverage Status

coverage: 79.153% (+0.01%) from 79.142%
when pulling 485ceef on eatyourgreens:fix-async-collection-search
into b4935ba on zooniverse:master.

@coveralls
Copy link

Coverage Status

coverage: 79.145% (+0.003%) from 79.142%
when pulling 408eb9a on eatyourgreens:fix-async-collection-search
into b4935ba on zooniverse:master.

@coveralls
Copy link

Coverage Status

coverage: 79.145% (+0.003%) from 79.142%
when pulling 5c5e976 on eatyourgreens:fix-async-collection-search
into b4935ba on zooniverse:master.

@coveralls
Copy link

Coverage Status

coverage: 79.145% (+0.003%) from 79.142%
when pulling 5c5e976 on eatyourgreens:fix-async-collection-search
into b4935ba on zooniverse:master.

@coveralls
Copy link

Coverage Status

coverage: 79.136% (-0.006%) from 79.142%
when pulling a6a4e34 on eatyourgreens:fix-async-collection-search
into b4935ba on zooniverse:master.

Tune this value to determine when a search term
is long enough to use Panoptes full-text search.
*/
const MIN_SEARCH_LENGTH = 3;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Increasing this might make the search results more intuitive.

@eatyourgreens eatyourgreens force-pushed the fix-async-collection-search branch from a6a4e34 to 7e46e7d Compare June 17, 2024 13:15
@coveralls
Copy link

Coverage Status

coverage: 79.192% (-0.02%) from 79.214%
when pulling 7e46e7d on eatyourgreens:fix-async-collection-search
into 37bef25 on zooniverse:master.

@coveralls
Copy link

Coverage Status

coverage: 79.186% (-0.03%) from 79.214%
when pulling 2342465 on eatyourgreens:fix-async-collection-search
into 37bef25 on zooniverse:master.

@eatyourgreens eatyourgreens force-pushed the fix-async-collection-search branch from 2342465 to cd49294 Compare June 17, 2024 14:50
@coveralls
Copy link

Coverage Status

coverage: 79.211% (+0.03%) from 79.18%
when pulling cd49294 on eatyourgreens:fix-async-collection-search
into 81eb200 on zooniverse:master.

@eatyourgreens eatyourgreens changed the title fix(project): collections search is asynchronous fix(project): update collections search asynchronously Jun 17, 2024
@eatyourgreens eatyourgreens force-pushed the fix-async-collection-search branch from cd49294 to 8e1f127 Compare June 22, 2024 08:27
@coveralls
Copy link

Coverage Status

coverage: 79.14% (-0.04%) from 79.176%
when pulling 8e1f127 on eatyourgreens:fix-async-collection-search
into 09cd24e on zooniverse:master.

Make the collections search, in the popup collections modal, asynchronous. This fixes a bug where search results were not correctly synchronised to the stored search term (in component state.)
We don't want the component to rerender when the search text changes, only when new search results are received from Panoptes.
@eatyourgreens eatyourgreens force-pushed the fix-async-collection-search branch from 8e1f127 to be09e4a Compare July 23, 2024 12:40
@coveralls
Copy link

coveralls commented Jul 23, 2024

Coverage Status

coverage: 79.026% (+0.01%) from 79.015%
when pulling 00832e5 on eatyourgreens:fix-async-collection-search
into 3275064 on zooniverse:master.

@mcbouslog mcbouslog self-requested a review August 7, 2024 14:02
@mcbouslog mcbouslog self-assigned this Aug 7, 2024
const search = text.trim()
onSearch({
await onSearch({
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix for the bug is to wait on the API request before updating state.

@eatyourgreens eatyourgreens force-pushed the fix-async-collection-search branch from 0863241 to 8a5cdd9 Compare August 7, 2024 17:00
Copy link
Contributor

@mcbouslog mcbouslog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The collections search is working more as expected with these changes 👍 .

The panoptes full-text search yields some unexpected results, as noted in this comment - #6120 (comment) - but that's unrelated to these changes and the specific issue of seeing all the collections with 4 characters.

Thanks for the fix!

@mcbouslog mcbouslog merged commit 0effc47 into zooniverse:master Aug 7, 2024
10 checks passed
@eatyourgreens eatyourgreens deleted the fix-async-collection-search branch August 7, 2024 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants