Skip to content

Migrate parsesSearchQuery to query state store (take 2)

Administrator requested to merge fkling/migrate-parsedsearchquery-take2 into main

Created by: fkling

This PR restores #29449 by reverting #29565 . See the original PR for information about the actual changes.

Additionally this PR includes a fix for what the original PR got wrong. This is all in the second commit so it should suffice to review that:

The issue with the previous implementation was that the route render function wasn't run again after the search was submitted and thus the search results component was never rendered.

This commit fixes this issue by wrapping the conditional logic in an actual component which can properly subscribe to the query state store and gets re-rendered when a search query is present in the query (or not). After chatting with other frontend devs, this seems to be the most sensible solution at this time.

This wasn't a problem in the past because all data was passed down via props, guaranteeing that the parent component would re-render if anything changes. But this is different now that we move things to stores and directly reading data from the stores is prone to such errors.

I tried to create an integration to recreate this issue, but I wasn't successful. The integration test does render the search results page correctly, even with the faulty logic. There might be something else happening that causes the whole app to re-render and therefor "it works". However, while it wasn't useful to determine what was wrong, the one integration test that failed in the previous PR doesn't fail anymore with this fix so I reverted it to it's original logic, so that's at least some indicator.

Merge request reports