Skip to content

fix(redirect): race condition with mismatched query #1310

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

Merged
merged 14 commits into from
Apr 24, 2025
Merged

fix(redirect): race condition with mismatched query #1310

merged 14 commits into from
Apr 24, 2025

Conversation

drodriguln
Copy link
Contributor

@drodriguln drodriguln commented Apr 15, 2025

Summary

There is a race condition caused by requests being resolved without concern for order. For example, here's a local example with the redirect plugin missing the redirect item with the correct query. The first two logged strings in each log are the query stored in the state (which the redirect plugin leverages) and the query from the resolved request. The last log is the response actually used by autocomplete and the plugin though we would expect the second from last one as it has a matched query from the state and response. Side note: the playground in the screenshot was modified to keep the dropdown open, which normally will not happen.

image

Result
This is a bit of a mixed PR as having to touch more of the redirect plugin has exposed some opportunity for test and typing cleanup.

  • Updates the redirect plugin's hook into onResolve to return early if the query used in the state (i.e. the input query value) does not match the query returned in the resolved response
    • Accomplished by updating the transformResponse function to return an object instead of the redirect url as a string
    • This means defaulting to an Algolia source should work out of the box, but custom sources should be overridable from this function--which was needed anyway for the redirect url mapping
  • Cleans up the unit tests for the redirect url plugin since I had to make updates in there anyway
    • Relying on checking values directly instead of relying on inline snapshots
    • Uses expect({}).toHaveTextContent(...) instead of expect({}.textContent).toBe(...)
    • Change the confusingly named hit "query" value to "name" to differentiate from the actual query values
  • Adds doc comments to the new and some of the existing redirect types

Testing
This happens especially with Safari, though still not always. See the video recording shared in the CR ticket: https://algolia.atlassian.net/browse/CR-8225

  1. Open up Safari for the redirect url example currently available (not running this PRs code), have the network tab open to track the last resolved requests, and type "algolia" very quickly into the input.
  2. With enough tries (you may have to close and reopen the tab/window to reproduce), you should see the dropdown briefly show the redirect item and then disappear. I personally really struggled with the existing app config and opted to borrow the customer's appId, apiKey, and index to test with. I think there's got to be something with the shorter "faq" text to type as it is faster to type after focusing the input. Feel free to reach out to me or grab the info from the customer's site link on the CR ticket linked above.
Screen.Recording.2025-04-23.at.2.30.18.PM.mov
  1. Now try to reproduce using the redirect url example running the code from this PR. You should no longer see the issue.

Copy link

codesandbox-ci bot commented Apr 15, 2025

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 31f1287:

Sandbox Source
@algolia/autocomplete-example-github-repositories-custom-plugin Configuration
@algolia/autocomplete-example-instantsearch Configuration
@algolia/autocomplete-example-playground Configuration
@algolia/autocomplete-example-preview-panel-in-modal Configuration
@algolia/autocomplete-example-starter-algolia Configuration
@algolia/autocomplete-example-starter Configuration
@algolia/autocomplete-example-reshape Configuration
@algolia/autocomplete-example-vue Configuration

@drodriguln drodriguln changed the title fix(redirect): item mismatch with last search fix(redirect): race condition with mismatched query Apr 16, 2025
@drodriguln drodriguln requested review from a team, dhayab and shaejaz and removed request for a team April 22, 2025 19:12
@drodriguln drodriguln marked this pull request as ready for review April 23, 2025 20:40
Copy link
Contributor

@Haroenv Haroenv left a comment

Choose a reason for hiding this comment

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

perfect, thanks for handling this edge case!

@Haroenv Haroenv merged commit 531b078 into next Apr 24, 2025
10 checks passed
@Haroenv Haroenv deleted the EMERCH-1995 branch April 24, 2025 15:16
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.

2 participants