fix: enable non-case-sensitive redirects from the Input component

[jkaho]

Currently, adding a string-lowercase step to the query/autocomplete pipeline enables the query Tires to return the redirect for tires, e.g.:

redirects: { 
    tires: { 
        id: "abc", 
        target: "https://foo.com",
        token: "123"
    } 
}

But the react-search-ui Input component will only look for redirects based on the raw query Tires, not the transformed version tires.

Prefer to use the (possibly transformed) query returned from the search response over the raw input query to look up redirects.

DAU-249

---

DAU-249.mov

[changeset-bot]

🦋 Changeset detected

Latest commit: cdec372

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@sajari/react-hooks	Patch
@sajari/react-search-ui	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[jonathaningram]

I suspect we're going to want to make a test, because it's not really clear to me that this change doesn't break subtle behavior we previously expected: We're changing the input change handler to not look at the changed value anymore, but instead use the evaluated/output variable.

Long shot, but we don't have any concept of feature flags in here do we? E.g. can't just put this behind a flag and test it out for a specific customer?

[jonathaningram]

You're returning response because you need to access values (read: variables) in the calling code. Could we instead return variables from this? variables is a sibling of redirects, and we return redirects off this, so could we also not return variables?

Or if we need to distinguish these variables from the input ones maybe we need to name them outputVariables or evaluatedVariables

[jkaho]

There don't seem to be any variables returned in the search response, just values (which looks like it might be the equivalent of variables in the v4 search response?), e.g., this is what all the responses I'm getting look like (minus redirects for some):

{
    "values": {
        "q": "tires",
        "q.original": "tires"
    },
    "redirects": {
        "tires": {
            "id": "abc",
            "target": "https://www.performanceplustire.com/tires-for-sale/",
            "token": "123"
        }
    }
}

And the variables I think you're referring to are just default params that we set

{
    "q": "Tires",
    "resultsPerPage": 15,
    "fields": "",
}

[jonathaningram]

Sorry values is the v2 term. We renamed to variables in v4. So they are one and the same.

[jonathaningram]

So previously, if the user typed in tir (imagine they were intending to type in tire) and then they stopped, does the existing behavior mean that it was always trying to look up redirects['tir']? Which does seem odd to me.

[jkaho]

Yes, that's the behaviour I was seeing.

To clarify, what exactly doesn't seem right about that to you? I would think that, for the most part, we'd want to search for redirects that match the exact query string the user has inputted (with the exception of bypassing the whole case-sensitivity thing), no? What if there really was a redirect for tir?

[jonathaningram]

What if there really was a redirect for tir?

Yep you're right. If the user types tire and there is a redirect for tire, then we should expect the redirect to trigger, rather than autocomplete kick it, correct it to, say, tired (which incidentally may also have a redirect of its own and it's unclear in that case who would win out of tire or tired). All that is to say that, looking at the value the user typed makes sense. Not sure why I thought it odd.

I guess it's not clear to me if changing to look at q.original breaks stuff. Because the old behavior is such that on key down it uses the value in the input field and looks up redirects. And that value is available / changes on every on key down event. But the new behavior is such that it is going to ignore what the user typed, and look at q.original which is only available and only updated once a Search API call is done. And the time between the response to that API call and the user typing in the input is varied. And because of that, I'm not sure of the consequences of changing the behavior of this so (what appears to be) subtly.

[jonathaningram]

Just quickly, I'm not saying this change is wrong, I'm just unsure of the consequences :|

[jkaho]

Fair callouts 🙂 Better to be safe than sorry. I'll do more testing and look into writing a test, though I'm not sure rn how your concerns would be captured in said test

[jkaho]

Also, no concept of feature flagging here unfortunately. Not yet anyway

[jkaho]

Perhaps we could do one of those old env var flags we used to do in the console

[jkaho]

But the new behavior is such that it is going to ignore what the user typed, and look at q.original which is only available and only updated once a Search API call is done. And the time between the response to that API call and the user typing in the input is varied.

After thinking about this - the redirects we're referring to in this line

const redirectValue = redirectsRef.current[responseQuery.current ?? value];

come from the same search API call as the q.original value, no? Would this not mean that if we have a value for redirectsRef from the search response then we should also have one for responseQuery?

[jonathaningram]

Perhaps we could do one of those old env var flags we used to do in the console

It would allow for quickly disabling it if we had to, but we'd still need to roll out the release and have it propagate to all end users. So there's still a chance of breaking users and having no fast rollback. Probably not going to be feasible to do any sort of feature flagging here.

[jkaho]

Closing in favour of less bug-prone solution: #875