search

elastic / kibana

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

[Fleet] Refactor Policy Editor UI #123819

Open juliaElastic opened 2 years ago

juliaElastic commented 2 years ago

Fleet's policy editor is its largest and most complex UI. In its current state, it is incredibly difficult to maintain, has led to numerous SDH's related to edge cases with package upgrades, variables, etc and hampers our team's velocity when our work crosses its path.

The policy editor code path starts here.

The policy editor's implementation resides largely in this directory within the Fleet UI codebase.

Overall, the implementation needs to be broken up into smaller components, and we should be utilizing React Context or another state management solution to pass around the agent/package policy object. Currently, there's a lot of verbosity and ceremony involved in writing updates back to this policy object as the user makes changes in the policy form that could be avoided with a "global state" style implementation. useContext in combination with useReducer (https://react.dev/reference/react/useReducer) can be a very potent combination for scalable state management in highly complex UIs like this.

If React's native Context API isn't suitable, we could consider a third party solution e.g.

It'd be great to be able to do things like this

const { packagePolicy, dispatch } = usePackagePolicyEditorContext() 

const handleVariableChange = (newVariableValue) => { 
  dispatch('UPDATE_PACKAGE_POLICY', { input: { streams: [ ... ]}
}

Dispatching action payloads with partial updates that can be handled upstream by a reducer would be a lot easier than what we currently do, which is pass update handlers down 3-4 levels of abstraction via props.

Naming is also a problem here, we use a concept of steps to break the policy form down into chunks, e.g.

What is the difference between StepConfigurePackagePolicy and StepDefinePackagePolicy? It is very difficult to determine which step correlates to what we see on the page from the names chosen.

Further, each of these steps is made up of smaller components like

It's similarly difficult to tell what each of these steps really means, and what UI elements it corresponds to on the policy editor page. We could vastly improve the mental overhead needed to maintain the policy editor by rethinking some of these names and the structure of components here.

Another issue is the odd coupling between the "edit existing" policy editor, the "create new" policy editor, and the "upgrade package policy" editor. There are numerous distinct concerns in each of these use cases, but the implementation today is very hacky and largely just bolts on conditionals to the "create package policy" editor. If the state management is abstracted to a common reducer/context style solution, it should be easier to have the nuance for each of these use cases stand on its own rather than be bolted on to various points in the "create package policy" editor components via conditional logic.

The policy editor as a whole also has many data fetching dependencies on multiple APIs. These operations should almost certainly be wrapped with react-query hooks to provide better state management around the network requests. react-query will allow us to unify network state by making requests depend on each other, provide a better loading experience, and prevent needless re-fetches and re-renders which greatly harm performance and UX.

elasticmachine commented 2 years ago

Pinging @elastic/fleet (Team:Fleet)

kpollich commented 2 years ago

++ to this from me. Working with the policy editor UI is becoming increasingly challenging, and it's confusing that our Edit flows rely on components that live in the create_package_policy_page directory as well. I think we should put some effort into hoisting components that are shared between these two views.

nchaulet commented 2 years ago

If we start refactoring this, it will be nice to move the logic outside and have more pure presentation component. It start to be hard to debug the side effect and what is updating a package policy.

joshdover commented 2 years ago

Taking a look at recent bugs in this area, I'm also +1 on this. I think we could get a lot of value out of extracting the fetching/merging/validation logic so that we can easily test this separate from the presentation of the UI. In general, I think any business logic here is highly valuable and not being able to test it separately from the UI is hurting us.

Here's a short list of recent bugs in this area:

Depending on the scope of the required changes for upcoming refactors to make this component more reusable, I think we may want to require that we do this type of extraction and testing first to be sure that we don't break any critical business logic as the UI is changed.

juliaElastic commented 2 years ago

EditPackagePolicyPage could use a splitting too, it is more than 800 lines. It has many smaller components in the same file like 4 Breadcrumb components, UpgradeStatusCallout. Also it has some duplicated functionality from CreatePackagePolicyPage.

kpollich commented 1 year ago

Refreshed the description and put this on our tech debt tracker