[Discuss][Cloud Security][Benchmark Rules] Refactor the Benchmark Rules page to use the State-Reducer or State-Context pattern

Question/Proposal

While I was investigating a performance issue within the Cloud Security Benchmark Rules page I realized that for every user interaction re-fetching all the rules and re-rendering almost all the components on the page, therefore we can visibly see a performance issue when interacting with components on the page.

I would like to propose that we use either a State-Context or State-Reducer-Context pattern for the Benchmark Rules page. Ideally, we should start with the less complex State-Context pattern and see if it resolves the performance issue and then transition to the State-Reducer pattern, which should be pretty simple to do as you will see in the examples below.

Background The current architecture of the Benchmark Rules follows the prop drilling pattern where the parent container keeps the main state of the page and passes properties to the child components with a data grid, table header, rules counters, and a fly-out. This becomes very hard to follow as a developer, additionally, React will require a lot of performance helpers like memoization which adds more complexity for React to render the Javascript code.

Every time the container is rendered it makes an HTTP request for a new set of benchmark rules from the backend. This creates a new instance of the data and most of the subsequent memoized functions need to be run again. When we enable or disable a rule in the table we have 4 HTTP requests to resolve before we can show the switch was enabled or disabled. Additionally, when we enable or disable a rule we force the ReactQuery hook to fetch all the rules again, this will cause React to re-render the components whose props reference the data.

The prop drilling pattern may be contributing to the performance issue as well because we are passing a new function to the Benchmark Rules components from the container

Proposal details here

DOD:

[ ] Agreement from the team to proceed with this proposal
[ ] POC the state reducer pattern and demo to the Cloud Security team
[ ] Document the patterns and what is the best case to use either.
[ ] Work breakdown detailed in follow-up tech debt tickets

Pinging @elastic/kibana-cloud-security-posture (Team:Cloud Security)

Hi @seanrathier

Great initiative with this proposal!

As we initially discussed the topics in the team sync today, there were some discussions involving on how can we ensure design consistency by using this pattern. After reglecting for a while I guess we should consider reinforcing that the proposal does not enforce to use this pattern from now on for everything, as the refactor happens in two major steps:

Unifying the business logic into the RulesPageContext
Unifying the state in a central localtion with a reducer function. And aftee that we update all components to remove logic and only consume the context.

I think the majority of the components that face the similar issues with a lot of prop drilling will already have a huge benefit from moving all the logic into the context, but doesn't necessarily means it need to use a reducer function, as state management is simpler using react query within a context than setting up a reducer function.

But for this particular case in the Benchmarks Rules page, reduce seems to be a good option, as there are complex business logics we want to consolidate and we are fetching data from multiple APIs and adding it to the state, controlling that from a central position makes sense to me.

So if that makes sense, I think we can consider adding to the proposal as well the following:

When to use the state reducer pattern
When to not use it (as not every case will need)
How can we ensure a consistent implementation when using it (eg by creating a doc in our team docs, plus some tutorial or workshop if needed)

And also that open rooms for more discussions that we cana address separately as when to just move the logic to the context instead.

@opauloh, good points, I will add them to the proposal

You are right, we should NOT prescribe this pattern to everything we do. Maybe you read the version before I bolded the "not". 😆 Your points make sense, I will edit the proposal with your notes.

After reviewing the proposal to transition our Cloud Security Benchmark Rules page from a prop-drilling architecture to either a State-Context or a State-Reducer-Context pattern, I'm leaning toward beginning with the State-Context pattern. This approach not only simplifies our current complexity but is also more accessible for those unfamiliar with more advanced React patterns.

The motivation behind this change is clear. Our current architecture, which heavily relies on prop drilling, results in performance bottlenecks due to repeated HTTP requests and re-rendering upon user interactions. By integrating the State-Context pattern, we can centralize and manage the state more efficiently. This not only reduces unnecessary re-renders as identified by @seanrathier but also decreases the load of HTTP requests by maintaining a consistent state across re-renders until explicitly changed.

I would say that starting with the State-Context is beneficial as it presents a gentler learning curve and provides an immediate solution to our performance issues. It allows for more structured state management without the initial complexity of reducers, making it easier for the team to adapt and understand.

And then, once the team is adapted to the State-Context pattern and its benefits are realized, we can consider advancing to the State-Reducer pattern. The State-Reducer pattern, while similar, offers more granular control over state updates and is better suited for handling state logic that comes from multiple sources, and transitioning to this pattern after gaining familiarity with the basic concepts of Context will be much smoother and more intuitive.

Proposal Details

The State-Context and State-Reduce-Context patterns have a commonality: React’s Context is best explained as a Provider that can give child components access to shared states and functions. The Cloud Security team should be familiar with this pattern as there is evidence of its usage in our responsibilities in Kibana.

The most important aspect of this change is to have the Benchmark Rules page's React components consume as many properties as they can that are basic types so the components themselves don't need to transform the data on every render. Doing this should provide better performance and make this complex page's flow easier to follow, refactor and resolve issues. Additionally, this pattern will make it easier to create unit tests because the components do not depend on business logic and will only be concerned about displaying state.

Using either of these patterns instead of prop drilling should not be a solution for everything we do. We should not use this for simple pages that pass a handful of properties to its children. Also, if you have a component whose state is independent, for example, fetching data for a drop-down button, we do not need to add this to the context.

State-Context

The State-Context pattern will have us move all the shared state and business logic that affects the state to the Benchmark Rules page Context.

Here is a high level example of how we could the the State-Context pattern.

import React from 'react';
interface BenchMarkRule {
  id: string;
  version: string;
  name: string;
  description: string;
  muted: boolean;
}

interface RulesPageState {
  rules: BenchMarkRule[];
  ruleCount: number;
  enabledRuleCount: number;
  disabledRuleCount: number;
  status: 'idle' | 'loading' | 'error' | 'success';
}

interface RulesPageContext extends RulesPageState {
  toggleRuleMuted: (rule: BenchMarkRule) => void;
}

const RulePageContext = React.createContext<RulesPageContext | undefined>(undefined);

function RulesPageProvider({ children }: { children: React.ReactNode }) {
  const { mutedRules, setMutesRules } = React.useState(
    // assume we are getting all the muted rules from the backend when the page loads
    useMutedBenchmarkRules()
  );
  const { status, ruleData } = useCspBenchmarkRules();

  const toggleRuleMuted = (rule: BenchMarkRule) => {
    // some logic to add or delere the rule in the frontend
    const newMutedRules = rule.muted ? (rules.map((r) => r.id === rule.id).muted = !rule.muted) : mutedRules.filter((r) => r !== rule.id);
    setMutesRules(newMutedRules);
    // Post action to update the rule in the backend
    useUpdateMutedBenchmarkRules(newMutedRules);
  };

  const rules = ruleData
    ? ruleData.map((rule) => ({
        id: rule.id,
        version: rule.version,
        name: rule.name,
        description: rule.description,
        muted: mutedRules.includes(rule.id),
      })) : [];

  const pageState = React.useMemo(() => {
    return {
        status,
        rules,
        ruleCount: rules.length,
        enabledRuleCount: rules.filter((r) => r.muted).length,
        disabledRuleCount: rules.filter((r) => !r.muted).length,
        toggleRuleMuted, // this should include the backend call to update the rule
    }
  }, [status, rules, toggleRuleMuted]);

  return <RulePageContext.Provider value={pageState}>{children}</RulePageContext.Provider>;
}

function useRulesPageContext() {
  const context = React.useContext(RulePageContext);
  if (context === undefined) {
    throw new Error('useRulesPage must be used within a RulesPageProvider');
  }
  return context;
}

export function BenchmarkRulePage() {
  return (
    <RulesPageProvider>
      <h1>Benchmark Rules</h1>
      <BenchmarkRulesHeader />
      <BenchmarkRulesTable />
    </RulesPageProvider>
  );
}

function BenchmarkRulesHeader() {
  const { status, ruleCount, enabledRuleCount, disabledRuleCount } = useRulesPageContext();
  if (status === 'success') {
    return (
      <div>
        <div>Total Count: {ruleCount}</div>
        <div>Enabled: {enabledRuleCount}</div>
        <div>Disabled: {disabledRuleCount}</div>
      </div>
    );
  } else {
    return <div>loading...</div>;
  }
}

function BenchmarkRulesTable() {
  const { status, rules } = useRulesPageContext();
  if (status === 'success') {
    return (
      <div>
        {rules?.map((rule) => (
          <BenchMarkRuleComponent key={rule.id} rule={rule} />
        ))}
      </div>
    );
  } else {
    return <div>loading...</div>;
  }
}

function BenchMarkRuleComponent({ rule }: { rule: BenchMarkRule }) {
  const { toggleRuleMuted } = useRulesPageContext();
  return (
    <div>
      <h3>{rule.name}</h3>
      <p>{rule.description}</p>
      <button onClick={() => toggleRuleMuted({ ...rule, muted: !rule.muted)}>{rule.muted ? 'Enable' : 'Disable'}</button>
    </div>
  );
}

When to use the State-Context pattern

This is a subjective rule, as is most things React. ;)

We should only use this pattern when we have many child components (or grandchildren) that need a state that is consumed by their parents. Another gate for this pattern could be where business logic execution in a child component that affected the parent and siblings should be encapsulated in the context

State-Reducer

The React hook useReducer is not often used because it reminds people of Redux and the tremendous amount of boilerplate code needed, and it was hard for newcomers to the React framework to understand because it added more complexity to the flow.

The difference in this pattern is that we are using dispatching as a way to communicate that we want to change the state. This will abstract the dispatching aspect of the reducer pattern help us to manage a state that depends on other state values, and coordinate fetching data from backend services.

Here is a high-level example of what the Benchmark Rules page would be after consuming the state-reducer-context pattern that can be dropped into a React application in Kibana

import React, { useReducer } from 'react';

interface BenchMarkRule {
  id: string;
  version: string;
  name: string;
  description: string;
  muted: boolean;
  checked: boolean;
}

interface RulesPageState {
  rules: BenchMarkRule[];
  ruleCount: number;
  enabledRuleCount: number;
  disabledRuleCount: number;
  status: 'idle' | 'loading' | 'error' | 'success';
}

interface RulesPageContext extends RulesPageState {
  loadRules: () => void;
  toggleRuleMuted: (rule: BenchMarkRule) => void;
}

type RulesPageAction = { type: 'LOAD_RULES' } | { type: 'TOGGLE_RULE_MUTED'; rule: BenchMarkRule };

function rulesPageReducer(state: RulesPageState, action: RulesPageAction): RulesPageState {
  switch (action.type) {
    case 'LOAD_RULES':
      const newState: RulesPageState = {
        rules: [], // fetch the rules from the backend and format the rules and dispatch the action
        ruleCount: 10, // fetch the rules count from the backend
        enabledRuleCount: 2, // fetch the enabled rules count from the backend
        disabledRuleCount: 2, // fetch the disabled rules count from the backend
        status: 'success',
      };
      return {
        ...state,
        ...newState,
      };
    case 'TOGGLE_RULE_MUTED':
      return {
        ...state,
        // toggle to rule in memory
        rules: state.rules?.map((rule) =>
          rule.id === action.rule?.id ? { ...rule, checked: !rule.checked } : rule
        ),
        enabledRuleCount: state.rules?.filter((rule) => rule.checked).length,
        disabledRuleCount: state.rules?.filter((rule) => !rule.checked).length,
        // post the rule to the backend
        // if there is an error, revert the rule in memory
      };
    default:
      return state;
  }
}

const RulePageContext = React.createContext<RulesPageContext | undefined>(undefined);

function RulesPageProvider({ children }: { children: React.ReactNode }) {
  const [state, dispatch] = useReducer(rulesPageReducer, {
    rules: [],
    ruleCount: 0,
    enabledRuleCount: 0,
    disabledRuleCount: 0,
    status: 'idle',
  });

  const loadRules = () => dispatch({ type: 'LOAD_RULES' }); // rules fetched from the backend });

  const toggleRuleMuted = (rule: BenchMarkRule) => {
    dispatch({ type: 'TOGGLE_RULE_MUTED', rule });
    // update the rule in the backend then if we need to fetch all the rules again
    loadRules();
  };

  React.useEffect(() => {
    loadRules();
  }, []);

  const pageState = {
    ...state,
    loadRules, // this should include the backend call to fetch the rules
    toggleRuleMuted, // this should include the backend call to update the rule
  };

  return <RulePageContext.Provider value={pageState}>{children}</RulePageContext.Provider>;
}

function useRulesPageContext() {
  const context = React.useContext(RulePageContext);
  if (context === undefined) {
    throw new Error('useRulesPage must be used within a RulesPageProvider');
  }
  return context;
}

export function BenchmarkRulePage() {
  return (
    <RulesPageProvider>
      <h1>Benchmark Rules</h1>
      <BenchmarkRulesHeader />
      <BenchmarkRulesTable />
    </RulesPageProvider>
  );
}

function BenchmarkRulesHeader() {
  const { status, ruleCount, enabledRuleCount, disabledRuleCount } = useRulesPageContext();
  if (status === 'success') {
    return (
      <div>
        <div>Total Count: {ruleCount}</div>
        <div>Enabled: {enabledRuleCount}</div>
        <div>Disabled: {disabledRuleCount}</div>
      </div>
    );
  } else {
    return <div>loading...</div>;
  }
}

function BenchmarkRulesTable() {
  const { status, rules } = useRulesPageContext();
  if (status === 'success') {
    return (
      <div>
        {rules?.map((rule) => (
          <BenchMarkRuleComponent key={rule.id} rule={rule} />
        ))}
      </div>
    );
  } else {
    return <div>loading...</div>;
  }
}

function BenchMarkRuleComponent({ rule }: { rule: BenchMarkRule }) {
  const { toggleRuleMuted } = useRulesPageContext();
  return (
    <div>
      <h3>{rule.name}</h3>
      <p>{rule.description}</p>
      <button onClick={() => toggleRuleMuted(rule)}>{rule.checked ? 'Disable' : 'Enable'}</button>
    </div>
  );
}

When to use the state-reducer pattern

Either of these patterns would solve our problems. However, those were a couple of use cases, in reality for the Benchmark Rules page we also can check and uncheck rules for bulk enabling and disabling, filtering of the visible rules, and selecting a rule for the fly out.

Using the reducer pattern is subjective, like most things in React as stated previously, however, the links I posted in the issue description describes it well in more detail when to use useState or useReducer.

"When one element of your state relies on the value of another element of your state in order to update: useReducer. Separate state logically by domain. If it changes together, it's likely better to keep together in a reducer. If something is pretty independent from other elements of state in that hook/component, then putting it with other elements of state is just adding unnecessary complexity and noise to that reducer and you'd be better off leaving it out on its own."

Advantages

Clarity
Eventual familiarity
Components are declaritive instead of imperative.
Easier to write tests

Drawbacks

Work Effort: One of the drawbacks I can think of is the work that needs to be dedicated to refactoring current work.
Learning curve: It may be harder for people unfamiliar with React to understand these patterns, however, these patterns are becoming normal for training and learning.

Thank you for this proposal @seanrathier, I would like to share some thoughts of mine regarding the proposed solutions and also another solution which we can take into consideration:

React Context: from my experience with the built in react useContext, it will re-render every component subscribed to the context which is under the provider. In this case I'm not sure if this is actually the optimal solution since we might be ending up re-rendering more components than we wanted and might even not solve the current lag that we have right now. And while not being a very complex solution, I would like us to consider another option which I will mention later
Reducers: This will probably solve the problem as it should only re-render the needed component, in our case, a single rule. The only downside is that its the most complex solution.
Utilize react query: This would be my preferred solution. React query is already a state management tool that we use on our day to day, which makes me expect this to be the easiest to understand and implement solution. On my original comment in the ticket I've suggested to use optimistic updates, which are simply updating the cache directly on click and only later wait for the http call to finish, and of course in case of a fail, revert back and pop an error toaster.

Lastly, I took a look and it seems like the problem is with the API for muting taking quite a bit of time: the bulk action call is the call we make in order to mute a rule (or rules) and we can see that it alone takes more than a second. after that we fetch the benchmarks (the actual rules) and the rules states (mute states) which only takes about 200ms each, which is fine, but fetching the benchmark do seems redundant at this point. right after the calls are done, the screen renders immediately, which leads me to believe this is not a problem with rendering but rather with long API calls. If thats the case, i believe no state management solution would solve this problem, we either need to make the API calls shorter by searching what can we optimise on the back end side, save 200ms or so by removing the refetch of the benchmarks rules, or we can simply use optimistic rendering.

@JordanSh, thanks for the feedback I will respond to your comments and I will summarize our offline discussion at the end

React Context: Yes you are right, the data that is returned from a React Context Provider will re-render the components to the DOM, even if the data did not change. However, I did not want to throw in too many concepts in the explanation of the pattern because I thought it would make what I am trying to show more complex, you can see in the PageRulesProvider that there is a pageState that is memoized, this should, in theory, make using the Provider more performant that would make this more performant
Reducers: I agree that it is a more complex solution, and we should only do that if just using a context does not resolve the issue.
ReactQuery: The intention was always to use ReactQuery, however, this was an implementation detail I wanted to avoid to make the pattern clear. I intended to promote using optimistic updates in the toggleRuleMuted function that the Provider is returning to the components, I guess that was not clearer than I hoped, apologies. If I were to implement this I would use the useMutation function in ReactQuery to mutate the rules and then post the data to the backend.

Summary of @JordanSh and @seanrathier's offline conversation

We both agree that using ReactQuery is a solution that could resolve the performance by using it's useMutation hook.
We both agree, that using the Context Provider would help us with code readability, reusability, and maintenance.
This will also reduce our component's dependency on React performance helpers, which add more code paths to execute which can introduce themselves to performance issues.

elastic / kibana