Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Security Solution] Migrate the rule management table to controlled table selection state API #170303

Open
xcrzx opened this issue Nov 1, 2023 · 3 comments
Labels
Feature:Rule Management Security Solution Detection Rule Management refactoring Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. technical debt Improvement of the software architecture and operational architecture

Comments

@xcrzx
Copy link
Contributor

xcrzx commented Nov 1, 2023

Historically, we've had a rather unpleasant code for controlling the rules table selection state (full version here):

  const isSelectAllCalled = useRef(false);

  // TODO Remove this synchronization logic after https://github.com/elastic/eui/issues/6184 is implemented
  // Synchronize selectedRuleIds with EuiBasicTable's selected rows
  useValueChanged((ruleIds) => {
    if (tableRef.current != null) {
      tableRef.current.setSelection(rules.filter((rule) => ruleIds.includes(rule.id)));
    }
  }, selectedRuleIds);

  const euiBasicTableSelectionProps = useMemo(
    () => ({
      selectable: (item: Rule) => !loadingRuleIds.includes(item.id),
      onSelectionChange: (selected: Rule[]) => {
        /**
         * EuiBasicTable doesn't provide declarative API to control selected rows.
         * This limitation requires us to synchronize selection state manually using setSelection().
         * But it creates a chain reaction when the user clicks Select All:
         * selectAll() -> setSelection() -> onSelectionChange() -> setSelection().
         * To break the chain we should check whether the onSelectionChange was triggered
         * by the Select All action or not.
         *
         */
        if (isSelectAllCalled.current) {
          isSelectAllCalled.current = false;
          // Handle special case of unselecting all rules via checkbox
          // after all rules were selected via Bulk select.
          if (selected.length === 0) {
            setIsAllSelected(false);
            setSelectedRuleIds([]);
          }
        } else {
          setSelectedRuleIds(selected.map(({ id }) => id));
          setIsAllSelected(false);
        }
      },
    }),
    [loadingRuleIds, setIsAllSelected, setSelectedRuleIds]
  );

That was mostly because EuiTable didn't support passing the selection state declaratively and only offered imperative handles for selection manipulations. But now that elastic/eui#6184 has been implemented, we can pass the selection state from our context more conveniently:

<EuiBasicTable 
  selection={{ selected }} 
/>

We can finally move away from the previous workaround.

This change could also help us fix some long-standing bugs, such as this one: #140180

@xcrzx xcrzx added technical debt Improvement of the software architecture and operational architecture Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Feature:Rule Management Security Solution Detection Rule Management Team:Detection Rule Management Security Detection Rule Management Team labels Nov 1, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detections-response (Team:Detections and Resp)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

@xcrzx
Copy link
Contributor Author

xcrzx commented Nov 1, 2023

The new API will become available in Kibana once Upgrade EUI to v90.0.0 #170179 is merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Rule Management Security Solution Detection Rule Management refactoring Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. technical debt Improvement of the software architecture and operational architecture
Projects
None yet
Development

No branches or pull requests

3 participants