[Logs UI] Fix errors during navigation

[afgomez]

Summary

Closes #69854.

Avoid setting state when the container hooks are not mounted.

React warns if a component tries to call useState or dispatch after a hook has been unmounted. This might happen if the call to any of those methods happens as a result of an async operation finishing after a component has been unmounted.

To solve this, we check if the component is mounted before trying to set the state or dispatch actions.

How to reproduce in master (and test for correctness in the branch)

• Open your browser's devtools, in the console tab.
• Go to the logs app.
• While the entries are loading, go somewhere else.
• A warning should show up in the console.

• While in this branch, nothing should show up in the console.

[elasticmachine]

Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui)

[Kerry350]

Fixes the issue 👍

Question on the implementation: Would it have been possible to handle this in useTrackedPromise alone, rather than adding the isMounted awareness and onReject callbacks to all the individual hooks?

E.g. where we have:

if (onResolve) {
  onResolve(value);
}

in useTrackedPromise could that have been:

if (onResolve && !isMounted) {
  onResolve(value);
}

? useTrackedPromise is already "aware" of mounted vs unmounted for the useEffect.

Anyway, just curious, as this way does mean we need to add extra bits when we use the hook, but if there's an explicit reason then ignore this 😄

[weltenwort]

@Kerry350 I've almost suggested this as well. It makes sense to do that if we assume the onResolve and onReject handlers will always do something that is sensitive to the React component lifecycle. Looking at the current usage this is mostly the case, but it doesn't have to. It could become an option one could opt out of?

[afgomez]

Would it have been possible to handle this in useTrackedPromise alone, rather than adding the isMounted awareness and onReject callbacks to all the individual hooks?

That's a good question. My reasoning to not handle it in useTrackedPromise is that it may or may not matter if the component is mounted or not.

Consider this example:

function SomeComponent() {
  const [_, createPromise] = useTrackedPromise({
	createPromise: () => Promise.resolve(),
    onResolve: () => { console.log("will I be called?") },
  });
  
  useLayoutEffect(() => { createPromise() });

  return null;
}

useMounted() internally uses useEffect(). This example uses useLayoutEffect(), which is called before the component renders.

If we call useMounted() inside useTrackedPromise() and use it as a dependency inside, the callback in the example might never be called (or maybe yes! I'm not sure how React schedules these things).

This was my reasoning for it, but maybe this will never be a problem. Right now we are not using useLayoutEffect() anywhere

~/code/kibana[master]% git grep useLayoutEffect -- x-pack/plugins/infra | wc -l
       0

...and we might never will 🤷, but it felt more right to move the responsibility of checking for "mountedness" to the components themselves.

[afgomez]

@elasticmachine merge upstream

[Kerry350]

but it felt more right to move the responsibility of checking for "mountedness" to the components themselves.

Yeah, in terms of a perfect design and where responsibility should lie what you've done feels right. However, because of the reality of our use cases (which I think would be 90%+ of the time we want the mount awareness) it feels like a lot of extra code.

useLayoutEffect() (and others) is a good example, but I think it's optimising for something we won't ever - or will rarely - use in reality.

Could we try the opt-out option? If it's not ideal, or presents issues, it can be reverted and we can merge this solution.

[afgomez]

Could we try the opt-out option?

I'll give it a shot 👍

[afgomez]

@elasticmachine merge upstream

[afgomez]

@elasticmachine merge upstream

[Kerry350]

LGTM 🎉 Thanks for taking the time to switch around the solutions, and sorry for the delay in re-review.

[afgomez]

@elasticmachine merge upstream

[afgomez]

Thank you @Kerry350 🙇

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 911df0b

Metrics [docs]

async chunk count

id	before	after	diff
infra	14	15	+1

async chunks size

id	before	after	diff
infra	2.6MB	2.6MB	-22.1KB

distributable file count

id	before	after	diff
default	42763	42766	+3

page load bundle size

id	before	after	diff
infra	174.0KB	174.2KB	+151.0B

History

• 💚 Build #83995 succeeded 0d2a91b
• 💚 Build #82456 succeeded fb579e9
• 💚 Build #82397 succeeded cae268f
• 💚 Build #81887 succeeded 43f98d0
• 💚 Build #81278 succeeded f3098a5

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream