[ResponseOps][Rules] Add loading state to rule params data views selector

[umbopepato]

Summary

Introduces a loading state in the data views select popover and renders a loading indicator when DVs are not available yet. This makes sure that even if the savedObjectsClient.find call of the data views service takes a long time, we don't show an empty popover in the meantime.

Screen.Recording.2024-12-10.at.18.34.41.mov

References

Fixes #198502

Release note

Fix race condition in alerting rules data view selector

[elasticmachine]

Pinging @elastic/response-ops (Team:ResponseOps)

[cnasikas]

I looked at the code and wanted to discuss some ideas along with what you did.

1. Stop relying on cache.

const ids = await dataViews.getIds(true); <--- refreshes the cache. We will always make a call to get the data views.

2. Load data views only on mount.

useEffect(() => {
    loadPersistedDataViews();
  }, []); <--- remove the loadPersistedDataViews dependency. We want to run on mount once.

3. Could we use React Query? I assume the effort is big as the components are being shared in various solutions.

4. Remove this check.

if (!allDataViewItems) {
    return null;
  }

[umbopepato]

1. Stop relying on cache.

const ids = await dataViews.getIds(true); <--- refreshes the cache. We will always make a call to get the data views.

Yes, that's what I did in order to manage the case where the DV cache is initialized but empty. In hindsight, I think we also need a separate loading state because the initialized but empty cache would still fall under the case of "ready" data. This could be the reason why the issue is not always reproducible the same way!

2. Load data views only on mount.

useEffect(() => {
    loadPersistedDataViews();
  }, []); <--- remove the loadPersistedDataViews dependency. We want to run on mount once.

I agree, the effect should be the same though, it's just the exhaustive deps lint rule that advises to add the callback to the dependencies, but it should be referentially stable and execute one time only.

3. Could we use React Query? I assume the effort is big as the components are being shared in various solutions.

Of course that was my first thought 😂 I agree that it would take a bit of time though, so how about we start with an intermediate solution and then add a work item for the migration to RQ?

4. Remove this check.

if (!allDataViewItems) {
    return null;
  }

Right, it should never execute since allDataViewItems is always defined, even if empty?

[cnasikas]

I agree, the effect should be the same though, it's just the exhaustive deps lint rule that advises to add the callback to the dependencies, but it should be referentially stable and execute one time only.

The useEffect relies on the useCallback which in turn relies on the dataViews service. Are we sure that the dataViews service is the same instance every time?

Of course that was my first thought 😂 I agree that it would take a bit of time though, so how about we start with an intermediate solution and then add a work item for the migration to RQ?

Totally agree. Let's do the intermediate solution and fix the bug.

[umbopepato]

The useEffect relies on the useCallback which in turn relies on the dataViews service. Are we sure that the dataViews service is the same instance every time?

It seems stable 🙂

Totally agree. Let's do the intermediate solution and fix the bug.

Ok! I left the error management empty for now. I don't know if the saved objects client already shows a toast but when we move to RQ we could maybe consider using an in-place Prompt or Callout

[elasticmachine]

💚 Build Succeeded

• Buildkite Build
• Commit: fee4836

Metrics [docs]

Page load bundle

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

id	before	after	diff
stackAlerts	25.1KB	25.2KB	+103.0B

History

• 💛 Build #260251 was flaky f7085bd
• 💚 Build #259341 succeeded 6a25a60
• 💚 Build #258944 succeeded 358d77a

[kibanamachine]

Starting backport for target branches: 7.17, 8.16, 8.17, 8.x

https://github.com/elastic/kibana/actions/runs/12356146557

[kibanamachine]

💔 Some backports could not be created

Status	Branch	Result
❌	7.17	Backport failed because of merge conflicts
✅	8.16
✅	8.17
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Manual backport

To create the backport manually run:

node scripts/backport --pr 203654

Questions ?

Please refer to the Backport tool documentation

[mistic]

This PR didn't make it on time to be in the latest 8.16.2 BC. Updating the labels.