[Defend Workflows][Trusted Apps] Advanced mode toggle and form

[parkiino]

Summary

• UI portion for advanced mode form in Trusted Apps page adds a toggle for the user to switch between the original basic conditions mode or the advanced mode.
• If the user toggles between forms after building some conditions, the state is saved per form while the flyout is open.
• When editing an existing trusted app, the saved form type is pre-populated in the conditions section
• Advanced mode contains additional text to warn the user of the implications of using advanced mode for trusted apps
• Updates search strategy and suggestions api to include trusted apps
• Unit tests

Screenshots

[szwarckonrad]

The form is getting stuck in infinite re-rendering when toggling to Advanced mode because the handleOnBuilderChange callback keeps triggering itself. Every time it runs, it updates state variables that were in its dependency array, causing React to re-run the callback again endlessly.

My attempt at solving this issue:

1. Combined related state into a single conditionsState object to reduce re-renders and to perform single update on state instead of several small ones that prove to be hard to follow and debug.
2. Used useRef to get stable references of items and break dependency chains
3. Stabilized callbacks by removing problematic dependencies from their dependency arrays
4. Added early returns in handleOnBuilderChange to skip unnecessary processing

With the changes posted below, the Advanced mode toggle works without killing the browser.

Diff with current state

diff --git a/x-pack/solutions/security/plugins/security_solution/public/management/pages/trusted_apps/view/components/form.tsx b/x-pack/solutions/security/plugins/security_solution/public/management/pages/trusted_apps/view/components/form.tsx
index 3095727723b..9628eb0c1e9 100644
--- a/x-pack/solutions/security/plugins/security_solution/public/management/pages/trusted_apps/view/components/form.tsx
+++ b/x-pack/solutions/security/plugins/security_solution/public/management/pages/trusted_apps/view/components/form.tsx
@@ -6,7 +6,7 @@
  */
 
 import type { ChangeEventHandler } from 'react';
-import React, { memo, useCallback, useMemo, useState, useEffect } from 'react';
+import React, { memo, useCallback, useMemo, useState, useRef } from 'react';
 import { isEqual } from 'lodash';
 import type { EuiFieldTextProps, EuiSuperSelectOption } from '@elastic/eui';
 import {
@@ -24,7 +24,6 @@ import {
   EuiSpacer,
 } from '@elastic/eui';
 import { i18n } from '@kbn/i18n';
-import { FormattedMessage } from '@kbn/i18n-react';
 import type { AllConditionEntryFields, EntryTypes } from '@kbn/securitysolution-utils';
 import type { ExceptionListItemSchema } from '@kbn/securitysolution-io-ts-list-types';
 import {
@@ -286,17 +285,22 @@ export const TrustedAppsForm = memo<ArtifactFormComponentProps>(
       useState<ArtifactFormComponentProps['item']>(item);
     const [advancedFormConditions, setAdvancedFormConditions] =
       useState<ArtifactFormComponentProps['item']>(item);
-    const [areConditionsValid, setAreConditionsValid] = useState(!!item.entries.length);
+
+    // Combine related state into a single object to reduce re-renders
+    const [conditionsState, setConditionsState] = useState({
+      areValid: !!item.entries.length,
+      hasDuplicateFields: false,
+      hasWildcardWithWrongOperator: hasWrongOperatorWithWildcard([item]),
+      hasPartialCodeSignatureWarning: hasPartialCodeSignatureEntry([item]),
+    });
+
     const [validationResult, setValidationResult] = useState<ValidationResult>(() =>
       validateValues(item)
     );
-    const [hasDuplicateFields, setHasDuplicateFields] = useState<boolean>(false);
-    const [hasWildcardWithWrongOperator, setHasWildcardWithWrongOperator] = useState<boolean>(
-      hasWrongOperatorWithWildcard([item])
-    );
-    const [hasPartialCodeSignatureWarning, setHasPartialCodeSignatureWarning] = useState<boolean>(
-      hasPartialCodeSignatureEntry([item])
-    );
+
+    // Use ref to prevent unnecessary re-renders in callbacks
+    const itemRef = useRef(item);
+    itemRef.current = item;
 
     const { http } = useKibana().services;
     const getSuggestionsFn = useCallback<ValueSuggestionsGetFn>(
@@ -324,25 +328,29 @@ export const TrustedAppsForm = memo<ArtifactFormComponentProps>(
       return isFormAdvancedMode ? 'advancedMode' : 'basicMode';
     }, [isFormAdvancedMode]);
 
-    const advancedModeToggle = [
-      {
-        id: 'basicMode',
-        label: i18n.translate('xpack.securitySolution.trustedApps.flyoutForm.basicMode', {
-          defaultMessage: 'Basic',
-        }),
-        iconType: selectedFormType === 'basicMode' ? 'checkInCircleFilled' : 'empty',
-        'data-test-subj': 'basicModeButton',
-      },
-      {
-        id: 'advancedMode',
-        label: i18n.translate('xpack.securitySolution.trustedApps.flyoutForm.advancedMode', {
-          defaultMessage: 'Advanced',
-        }),
-        iconType: selectedFormType === 'advancedMode' ? 'checkInCircleFilled' : 'empty',
-        'data-test-subj': 'advancedModeButton',
-      },
-    ];
+    const advancedModeToggle = useMemo(
+      () => [
+        {
+          id: 'basicMode',
+          label: i18n.translate('xpack.securitySolution.trustedApps.flyoutForm.basicMode', {
+            defaultMessage: 'Basic',
+          }),
+          iconType: selectedFormType === 'basicMode' ? 'checkInCircleFilled' : 'empty',
+          'data-test-subj': 'basicModeButton',
+        },
+        {
+          id: 'advancedMode',
+          label: i18n.translate('xpack.securitySolution.trustedApps.flyoutForm.advancedMode', {
+            defaultMessage: 'Advanced',
+          }),
+          iconType: selectedFormType === 'advancedMode' ? 'checkInCircleFilled' : 'empty',
+          'data-test-subj': 'advancedModeButton',
+        },
+      ],
+      [selectedFormType]
+    );
 
+    // Stabilized processChanged callback with minimal dependencies
     const processChanged = useCallback(
       (updatedFormValues: ArtifactFormComponentProps['item']) => {
         const updatedValidationResult: ValidationResult = validateValues(updatedFormValues);
@@ -356,7 +364,7 @@ export const TrustedAppsForm = memo<ArtifactFormComponentProps>(
 
         onChange({
           item: updatedFormValues,
-          isValid: updatedValidationResult.isValid && areConditionsValid && hasFormChanged,
+          isValid: updatedValidationResult.isValid && conditionsState.areValid,
           confirmModalLabels: updatedValidationResult.extraWarning
             ? CONFIRM_WARNING_MODAL_LABELS(
                 i18n.translate('xpack.securitySolution.trustedApps.flyoutForm.confirmModal.name', {
@@ -366,7 +374,7 @@ export const TrustedAppsForm = memo<ArtifactFormComponentProps>(
             : undefined,
         });
       },
-      [isFormAdvancedMode, areConditionsValid, hasFormChanged, onChange]
+      [isFormAdvancedMode, conditionsState.areValid, onChange]
     );
 
     const handleEffectedPolicyOnChange: EffectedPolicySelectProps['onChange'] = useCallback(
@@ -555,58 +563,74 @@ export const TrustedAppsForm = memo<ArtifactFormComponentProps>(
         ? (item.entries as TrustedAppConditionEntry[])
         : [defaultConditionEntry()];
 
-      setAreConditionsValid(!!ta.entries.length);
+      setConditionsState((prev) => ({ ...prev, areValid: !!ta.entries.length }));
       return ta;
     }, [item]);
 
-    // Advanced mode conditions handler
-    // this function is called so many times - why
+    // Stabilized advanced mode conditions handler with proper change detection
     const handleOnBuilderChange = useCallback(
       (arg: OnChangeProps) => {
-        const isCalledWithoutChanges =
-          (!hasFormChanged && arg.exceptionItems[0] === undefined) ||
-          isEqual(arg.exceptionItems[0]?.entries, trustedApp?.entries);
-
-        console.log('is called without change', !hasFormChanged, arg.exceptionItems[0]?.entries === undefined)
-        if (isCalledWithoutChanges) {
-          const addedFields = arg.exceptionItems[0]?.entries.map((e) => e.field) || [''];
+        // Early return for unnecessary calls to prevent infinite loops
+        if (!arg.exceptionItems?.[0] && !hasFormChanged) {
+          return;
+        }
 
-          setHasDuplicateFields(computeHasDuplicateFields(getAddedFieldsCounts(addedFields)));
+        const currentItem = itemRef.current;
+        const newEntries = arg.exceptionItems[0]?.entries;
+
+        // More robust change detection
+        const hasActualChanges =
+          newEntries && (!currentItem.entries || !isEqual(newEntries, currentItem.entries));
+
+        if (!hasActualChanges && hasFormChanged) {
+          // Only handle duplicate field detection for unchanged forms
+          if (newEntries) {
+            const addedFields = newEntries.map((e) => e.field) || [''];
+            setConditionsState((prev) => ({
+              ...prev,
+              hasDuplicateFields: computeHasDuplicateFields(getAddedFieldsCounts(addedFields)),
+            }));
+          }
           return;
-        } else {
-          setHasDuplicateFields(false);
         }
 
-        setHasWildcardWithWrongOperator(hasWrongOperatorWithWildcard(arg.exceptionItems));
-        setHasPartialCodeSignatureWarning(hasPartialCodeSignatureEntry(arg.exceptionItems));
+        // Batch all condition state updates
+        setConditionsState((prev) => ({
+          ...prev,
+          hasDuplicateFields: false,
+          hasWildcardWithWrongOperator: hasWrongOperatorWithWildcard(arg.exceptionItems),
+          hasPartialCodeSignatureWarning: hasPartialCodeSignatureEntry(arg.exceptionItems),
+          areValid:
+            arg.exceptionItems[0] !== undefined
+              ? !(arg.errorExists && !arg.exceptionItems[0]?.entries?.length)
+              : false,
+        }));
 
         const updatedItem: ArtifactFormComponentProps['item'] =
           arg.exceptionItems[0] !== undefined
             ? ({
                 ...arg.exceptionItems[0],
-                name: trustedApp?.name ?? '',
-                description: trustedApp?.description ?? '',
-                comments: trustedApp?.comments ?? [],
-                os_types: trustedApp?.os_types ?? [OperatingSystem.WINDOWS],
-                tags: trustedApp?.tags ?? [],
-                meta: trustedApp.meta,
+                name: currentItem?.name ?? '',
+                description: currentItem?.description ?? '',
+                comments: currentItem?.comments ?? [],
+                os_types: currentItem?.os_types ?? [OperatingSystem.WINDOWS],
+                tags: currentItem?.tags ?? [],
+                meta: currentItem.meta,
               } as ArtifactFormComponentProps['item'])
             : {
-                ...trustedApp,
+                ...currentItem,
                 entries: [{ field: '', operator: 'included', type: 'match', value: '' }],
               };
-        const hasValidConditions =
-          arg.exceptionItems[0] !== undefined
-            ? !(arg.errorExists && !arg.exceptionItems[0]?.entries?.length)
-            : false;
 
-        setAreConditionsValid(hasValidConditions);
         processChanged(updatedItem);
-        if (!hasFormChanged) setHasFormChanged(true);
+        if (!hasFormChanged) {
+          setHasFormChanged(true);
+        }
       },
-      [trustedApp, hasFormChanged, processChanged]
+      [hasFormChanged, processChanged]
     );
 
+    // Stabilized memoization with minimal dependencies
     const exceptionBuilderComponentMemo = useMemo(
       () =>
         getExceptionBuilderComponentLazy({
@@ -629,7 +653,7 @@ export const TrustedAppsForm = memo<ArtifactFormComponentProps>(
           osTypes: trustedApp.os_types,
           showValueListModal: ShowValueListModal,
         }),
-      [autocompleteSuggestions, handleOnBuilderChange, http, indexPatterns, trustedApp]
+      [http, indexPatterns, trustedApp.entries, trustedApp.os_types, handleOnBuilderChange]
     );
 
     if (isIndexPatternLoading || !trustedApp) {
@@ -773,6 +797,13 @@ export const TrustedAppsForm = memo<ArtifactFormComponentProps>(
             </EuiFormRow>
           </>
         ) : null}
+        <>
+          <EuiSpacer size="l" />
+          <EuiHorizontalRule />
+          <EuiSpacer size="l" />
+          {conditionsState.hasWildcardWithWrongOperator && <WildCardWithWrongOperatorCallout />}
+          {conditionsState.hasPartialCodeSignatureWarning && <PartialCodeSignatureCallout />}
+        </>
       </EuiForm>
     );
   }

If you’d like to try out these changes, you can download the patch file and apply it using git apply diff.patch.

[elasticmachine]

Pinging @elastic/security-defend-workflows (Team:Defend Workflows)

[parkiino]

removed this test because it seems like a duplicate of the test 'should call on visited for field input'

[elasticmachine]

💚 Build Succeeded

• Buildkite Build
• Commit: cc87354

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
@kbn/securitysolution-list-utils	162	161	-1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
lists	124.8KB	125.3KB	+502.0B
securitySolution	9.8MB	9.8MB	+7.3KB
total			+7.8KB

Count of Enzyme imports

Enzyme is no longer supported, and we should switch to @testing-library/react instead.

id	before	after	diff
securitySolution	190	189	-1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
securitySolution	94.3KB	94.3KB	+30.0B

Unknown metric groups

async chunk count

id	before	after	diff
securitySolution	101	102	+1

History

• 💚 Build #324126 succeeded 178ee56
• 💛 Build #324094 was flaky 1c5b629
• 💔 Build #323521 failed 8b8da62
• 💔 Build #323145 failed 558feea
• 💔 Build #322657 failed 212af77
• 💔 Build #320674 failed 53b8c96