[SecuritySoltuon] fix unnecessary requests from the alert flyout

[janmonschke]

Summary

Fixes #195607

As described in the issue above, whenever a click was registered in the alert flyout, some logic in Kibana would trigger 5 HTTP requests.

After some debugging, it became apparent that it was caused by a misconfigured ReactQuery client stemming from the cases plugin. The cases client does not specify a behaviour for focus changes which was causing the extra HTTP requests.

Security solution's ReactQuery client comes with some defaults that make sure that unnecessary requests due to focus changes don't occur. However, since the cases context was overriding the QueryClientProvider, these defaults were overwritten.

<SecuritySolutionApp>
  <...>
  <QueryClientProvider>
     <...>
     <CasesContext>
       <QueryClientProvider>
         <!-- the actual components -->
       </QueryClientProvider>
     </CasesContext>
  </QueryClientProvider>
</SecuritySolutionApp>

The fix was to add queryClient to the CaseContext's value.

Checklist

• Unit or functional tests were updated or added to match the most common scenarios

[elasticmachine]

💛 Build succeeded, but was flaky

• Buildkite Build
• Commit: 84ff56b

Failed CI Steps

• FTR Configs #90

Test Failures

• [job] [logs] FTR Configs #90 / Entity Analytics - Risk Engine @ess @serverless @serverlessQA init_and_status_apis status api should disable / enable risk engine

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
securitySolution	20.7MB	20.7MB	+45.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
cases	151.2KB	151.3KB	+139.0B

[elasticmachine]

Pinging @elastic/security-threat-hunting-investigations (Team:Threat Hunting:Investigations)

[adcoelho]

This was a great find!

Security solution's ReactQuery client comes with some defaults that make sure that unnecessary requests due to focus changes don't occur.

Do you mean setting refetchOnWindowFocus: false? I wonder if we have similar behavior somewhere in cases 🤔

I'll look into updating it in x-pack/plugins/cases/public/components/cases_context/query_client.ts

[janmonschke]

Do you mean setting refetchOnWindowFocus: false? I wonder if we have similar behavior somewhere in cases 🤔

I'll look into updating it in x-pack/plugins/cases/public/components/cases_context/query_client.ts

Yep, that's the one :)

[adcoelho]

Sorry @janmonschke I had a chat with the team about this PR and having some concerns we decided to block the PR for now 😅

We are worried about unexpected side effects of replacing the cases query client and will have a chat about it tomorrow. I'll get back to you.

[janmonschke]

@adcoelho Understood, looking forward to your conclusion.

Since you're talking about being worried about side effects for cases, not fixing this, will have side effects for everywhere else cases are used ;)

A quick fix would be to add the same defaults to cases but then we might have problems further down the line when the defaults are changed in either place.

Zooming out, this is not just an issue in cases <-> security solution, but an overall issue whenever plugins/packages etc use a <QueryClientProvider />. A rough count brought up 60-70 places in Kibana.

[adcoelho]

Since you're talking about being worried about side effects for cases, not fixing this, will have side effects for everywhere else cases are used ;)

Well, yes and no. That specific setting exists for a reason, and its usage(or not) is not necessarily tied to unexpected consequences. What about the other side of the picture, where query data is stale?

A quick fix would be to add the same defaults to cases but then we might have problems further down the line when the defaults are changed in either place.

We thought a more limited fix would rather allow this setting to be passed as props and used by cases when initializing their query client. We are also concerned with having multiple queryClients holding the cache for Cases' queries(I believe we have had problems with this in the past).

Zooming out, this is not just an issue in cases <-> security solution, but an overall issue whenever plugins/packages, etc use a . A rough count brought up 60-70 places in Kibana.

Again, the query client is used in around 60-70 places but its initialization without the refetchOnWindowFocus can be by design. Yes, there can be some unnecessary queries in places(I don't dispute there are too many on the linked issue 😅 ) but on the other hand, the user is not constantly faced with stale data. There's a trade-off there somewhere, we just wanna find the best one for cases.

[janmonschke]

I see what you mean. Of course it's possible to not enable refetchOnWindowFocus by design. However, the current solution of overwriting the queryClient will have side effects, no matter what.

The requests that are repeated can be quite costly in certain scenarios and they are not even owned by us. I just stumbled over this by accident ;)

I wonder if there's a better way to isolate the queryClient. But I'm not sure about this at the moment. One way to do it is to not have the client provider on the top level in the cases provider. Moving the provider down closer to the components that need it will make sure the side effects are kept to a minimum. Wdyt?

[cnasikas]

Btw, clicking from the DevTools to the page and vice-versa is considered a windows focus by React Query. Based on the video, it seems that annoyance is only when you use the DevTools (clicking from and to) and not something the users will experience. Could we verify that that's the case?

[janmonschke]

@cnasikas Ahhhhh, good point, that's the case. I'll close the issue and the PR :)

[kqualters-elastic]

@janmonschke @cnasikas if it ever proves useful, there's a setting under the "Rendering" tool of chromium dev tools, 'emulate a focused page' which will prevent focus from leaving the page when interacting with devtools