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.

Sign up for GitHub

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

Jump to bottom

Bugfix: Refresh search results when clearing category filter #142853

Merged
merged 1 commit into from
Oct 6, 2022
Merged

Bugfix: Refresh search results when clearing category filter #142853

merged 1 commit into from
Oct 6, 2022

Conversation

hop-dev
Copy link
Contributor

@hop-dev hop-dev commented Oct 6, 2022 •
edited by kibanamachine
Loading

Summary

When clearing a category filter, results were not being refreshed. Here, after clearing the communications category, the aws results should be shown again:

broken.mov

Removing the useEffect fixes this:

fixed.mov

@hop-dev hop-dev added bug Fixes for quality problems that affect the customer experience release_note:fix Team:Fleet Team label for Observability Data Collection Fleet team labels Oct 6, 2022
@hop-dev hop-dev self-assigned this Oct 6, 2022
@hop-dev hop-dev requested a review from a team as a code owner October 6, 2022 13:45
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

hop-dev
hop-dev commented Oct 6, 2022
View reviewed changes
x-pack/plugins/fleet/public/applications/integrations/hooks/use_local_search.tsx
@@ -16,13 +16,11 @@ export const fieldsToSearch = ['name', 'title'];
export function useLocalSearch(packageList: IntegrationCardItem[]) {
const localSearchRef = useRef<LocalSearch>(new LocalSearch(searchIdField));

useEffect(() => {
Copy link
Contributor Author

@hop-dev hop-dev Oct 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I am not sure why this useEffect was needed? is it best practice to only change ref.current in a useEffect? I can't find anything which says so. I've tested the search behavour and everything still works.

The bug was a race condition, the results are rendered before the useEffect is called, meaning the ref contains the stale search resaults. Removing the useEffect means the results are instantly updated removing the race.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch!
I searched about the use of ref in useEffect and actually it seems that they don't work like one would expect, causing this type of race conditions. There are some examples in this article: https://medium.com/@teh_builder/ref-objects-inside-useeffect-hooks-eb7c15198780

Thanks, TIL :)

criamico
criamico approved these changes Oct 6, 2022
View reviewed changes
Copy link
Contributor

@criamico criamico left a comment

Choose a reason for hiding this comment

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

LGTM

@hop-dev hop-dev enabled auto-merge (squash) October 6, 2022 14:18
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
fleet 912.5KB 912.5KB -39.0B

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @hop-dev

@hop-dev hop-dev merged commit 022e59f into elastic:main Oct 6, 2022
@kibanamachine kibanamachine added v8.6.0 backport:skip This commit does not require backporting labels Oct 6, 2022
@hop-dev hop-dev deleted the bugfix-refresh-search-on-category-change branch October 6, 2022 15:12
@jen-huang jen-huang added v8.5.0 and removed backport:skip This commit does not require backporting labels Oct 7, 2022
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 10, 2022
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 142853 locally

@hop-dev hop-dev added the backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) label Oct 10, 2022
@kibanamachine kibanamachine mentioned this pull request Oct 10, 2022
Merged
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 10, 2022
@hop-dev
Bugfix: Refresh search results when clearing category filter (elastic…
98eb182 
…#142853)

(cherry picked from commit 022e59f)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.5

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Oct 10, 2022
@kibanamachine @hop-dev
Bugfix: Refresh search results when clearing category filter (#142853) (
bba1cbf 
#142960)

(cherry picked from commit 022e59f)

Co-authored-by: Mark Hopkin <[email protected]>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 10, 2022
WafaaNasr pushed a commit to WafaaNasr/kibana that referenced this pull request Oct 11, 2022
@hop-dev @WafaaNasr
Bugfix: Refresh search results when clearing category filter (elastic…
b953e8b 
…#142853)
WafaaNasr pushed a commit to WafaaNasr/kibana that referenced this pull request Oct 14, 2022
@hop-dev @WafaaNasr
Bugfix: Refresh search results when clearing category filter (elastic…
837b428 
…#142853)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@criamico criamico criamico approved these changes

Assignees

@hop-dev hop-dev

Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) bug Fixes for quality problems that affect the customer experience release_note:fix Team:Fleet Team label for Observability Data Collection Fleet team v8.5.0 v8.6.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@hop-dev @elasticmachine @kibana-ci @kibanamachine @criamico @jen-huang