fix(answerAPI): controller search listener

[dmgauthier]

https://coveord.atlassian.net/browse/SVCC-4254

SVCC-4254

headless-answerapi-generated-answer fix

Two triggers to fetchAnswer are made after an empty request. uselessly hitting the RTK query cache

Problem description

When the answer-api is enabled

TLDR
Whenever a search request was made after a precedent search request is made with an empty query, the answerAPI was called two times with the same parameters. hitting the RTK query cache while painting red in the console.

Further
The last params with which a search was triggered are kept in a variable used to compare with the current params the listener current run uses. The comparison between the lastParams and the current ones determines if we should call the answerAPI or not. Because the query in the state is not updated at the same time than the request ID,the listener is run twice. If we do not update the lastTriggerParams when the query is empty, the condition will receive a wrong request ID the first run and let it pass through, calling the answerAPI twice with the same params.

Fix description

The current fix is not changing the user behavior. Since RTK query was preventing the API to be called twice anyway. But we esteem that the implementation should not rely on RTK query being nice.

The lastTriggerParams are now updated even when the query is empty when the listener is called. Preventing the condition to let pass the query with the exact same parameters.

[github-actions]

Pull Request Report

PR Title

✅ Title follows the conventional commit spec.

Live demo links

• Storybook

• Playwright report

Bundle Size

File	Old (kb)	New (kb)	Change (%)
case-assist	236.6	236.6	0
commerce	339.4	339.4	0
search	412	412	0
insight	400	400.1	0
recommendation	248.8	248.8	0
ssr	405.7	405.7	0
ssr-commerce	351.6	351.6	0

SSR Progress

Use case	SSR (#)	CSR (#)	Progress (%)
search	39	44	89
recommendation	0	4	0
case-assist	0	6	0
insight	0	27	0
commerce	0	15	0

Detailed logs

search : buildInteractiveResult
search : buildInteractiveInstantResult
search : buildInteractiveRecentResult
search : buildInteractiveCitation
search : buildGeneratedAnswer
recommendation : missing SSR support
case-assist : missing SSR support
insight : missing SSR support
commerce : missing SSR support

[mmitiche]

Nice explanation and nice fix ✅

[fbeaudoincoveo]

You could instead split this into distinct tests:

describe('subscribeToSearchRequest', () => {
  it('when the requestId is empty, does not call fetchAnswer', () => { // ... });

  describe('when the requestId is not empty' () => {
    it('when the requestId is the same as the previous one, does not call fetchAnswer', () => { // ... });

    describe('when the requestId is different from the previous one', () => {
      it('when the query is emtpy, does not call fetchAnswer', () => { // ... });

      it('when query is not empty, calls fetchAnswer', () => { // ... });
    });
  });
});

Which means that instead of having a queryCounter to index over an array of queries, you'd do only the required setup before each test.

I personally think this structure would be better because each test would then be self-contained, and you wouldn't need to rely on code comments to explain each expect.

[alexprudhomme]

I think this would be better as well

[dmgauthier]

I tend to agree with the test separation. And wouldn't do the test like this if it was easy to do it otherwise.
Its not possible to reset the vi.mock before each test. That kind of mock cannot work with global state variables + cannot be declared in the test scope.
Any example in headless on how you would do this would be appreciated.

[alexprudhomme]

Thanks for the explanation

[alexprudhomme]

I think this would be better as well