search

elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.69k stars 8.12k forks source link

Alerting flyouts don't handle changing props particularly well #83417

Open gmmorris opened 3 years ago

gmmorris commented 3 years ago

Some items from the original issue description have been addressed. Remaining tech debt:

our components are trying to do too many things at once (AlertsForm is both the form for choosing an AlertType and wraps the behaviour for rendering after a type has been selected), and could be addressed by breaking these down into smaller, more specific, components. This should also help in long term maintenance and sustainability of our codebase.

Original issue description

I've encountered a range of UI bugs caused by the state between AlertAdd and AlertForm going out of sync. This seems to be caused by race conditions between the useState in these two components and is hard to reliably reproduce (I've been able to reproduce it in Uptime quite regularly due to their unique method of selecting an alertTypeId, but it isn't only there), but is definitely present.

I'd recommend abandoning the show/hide approach, which is error prone, and instead going down a render/dont-reender approach which would mean we don't need to maintain state as often as we do now. While this is, potentially, less performant if the flyout is constantly created/destoryed, this seems unlikely as we don't expect users to regularly create/modify alerts at a high rate. This means that we might actually find this to be more performant as we would avoid instantiating these components in the first place for most users (unlike the current implementation).

A couple of notes:

  1. I've already addressed this in that Connectors flyouts in TriggersActionsUI (we now only render these components when needed) in this PR: https://github.com/elastic/kibana/pull/82126. Making the same change in alerting would be a little harder as it's used throughout solutions - we'll have to work with them to address this.

  2. Some of these bugs are hidden from sight because our type checking isn't quite true to reality (for example, this cast causes one of these state issues to be hidden). This is often because our components are trying to do too many things at once (AlertsForm is both the form for choosing an AlertType and wraps the behaviour for rendering after a type has been selected), and could be addressed by breaking these down into smaller, more specific, components. This should also help in long term maintenance and sustainability of our codebase. 🎉

elasticmachine commented 3 years ago

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

ymao1 commented 3 years ago

Alerts flyout was changed from the show/hide approach to the render/don't-render approach with this PR

gmmorris commented 3 years ago

Alerts flyout was changed from the show/hide approach to the render/don't-render approach with this PR

Awesome. With that in mind, what's actually left to do in this issue? Is it just down to cleaning up the Typescript signature to make this less brittle?

ymao1 commented 3 years ago

Yes, I believe we discussed just addressing #2 from the issue description by improving type checking since #1 has been addressed.

2. Some of these bugs are hidden from sight because our type checking isn't quite true to reality (for example, this cast causes one of these state issues to be hidden). This is often because our components are trying to do too many things at once (AlertsForm is both the form for choosing an AlertType and wraps the behaviour for rendering after a type has been selected), and could be addressed by breaking these down into smaller, more specific, components. This should also help in long term maintenance and sustainability of our codebase. 🎉

ymao1 commented 3 years ago

It looks like some work has already been done to tighten up the typings as part of this PR. Is that sufficient to resolve this issue? There's more that could be done in terms of breaking apart the components but at this point, that seems more like a tech debt issue than a bug that needs to be addressed immediately.

gmmorris commented 3 years ago

It looks like some work has already been done to tighten up the typings as part of this PR. Is that sufficient to resolve this issue? There's more that could be done in terms of breaking apart the components but at this point, that seems more like a tech debt issue than a bug that needs to be addressed immediately.

I think that's a fair assessment.

Can we update the description at the top to reflect that? Once that's done I think it can move to Code Debt 👍