Skip to content

Commit

Permalink
[Security Solution] Fix cursor jumping to end of text area when editi…
Browse files Browse the repository at this point in the history
…ng Rule Action Message (elastic#150823)

## Summary

Resolves: elastic#149885

For additional details, please see
elastic#141811 (comment) and
elastic#142217.

As mentioned in elastic#142217, this
issue is the result of managing stale events and timings (renderings +
field updates) between the Detections `RuleActionsField` component,
validation within the hooks-form lib, and field updates coming from the
`trigger_actions_ui` components.

Note: this behavior is explicit to the Edit Rule flow (`ActionsStepRule`
form), and not the Bulk Actions flyout (`RuleActionsFormData` form) as
events flow as intended in the latter, presumably because of all the
nested components and use of `useFormData` within the Edit Rule flow
(see [form libs
docs](https://docs.elastic.dev/form-lib/examples/listening-to-changes#forward-the-form-state-to-a-parent-component)).

The fix here is a modification of the fix provided in
elastic#141811 with `setTimeout`, but
instead of always pushing the params update to be within a `setTimeout`,
it now only does it when initially loading `Actions` with empty
`Params`. Since this fix also has the unintended side-effect of
flickering the validation error callout when first adding an action,
validation is now throttled to 100ms intervals, which also helps with
extraneous re-renders.

Before initial fix (with cursor issue) / Before throttle fix w/ flashing
error callout:
<p align="center">
<img width="350"
src="https://user-images.githubusercontent.com/2946766/215585424-8c056440-5906-4168-b997-aea3690c7ea5.gif"
/> <img width="242"
src="https://user-images.githubusercontent.com/2946766/217961284-704013b7-2f0a-4a24-95a4-4183348448f4.gif"
/>
</p>

After:
<p align="center">
<img width="242"
src="https://user-images.githubusercontent.com/2946766/218591986-e52456e0-e374-4d8e-bc84-9fb6da69f1f9.gif"
/>
</p>

(cherry picked from commit 736759a)
  • Loading branch information
spong committed Feb 16, 2023
1 parent 2c8c7f9 commit 176eff5
Show file tree
Hide file tree
Showing 5 changed files with 88 additions and 25 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ import {
import { getAllActionMessageParams } from '../../../../../../detections/pages/detection_engine/rules/helpers';

import { RuleActionsField } from '../../../../../../detections/components/rules/rule_actions_field';
import { validateRuleActionsField } from '../../../../../../detections/containers/detection_engine/rules/validate_rule_actions_field';
import { debouncedValidateRuleActionsField } from '../../../../../../detections/containers/detection_engine/rules/validate_rule_actions_field';

const CommonUseField = getUseField({ component: Field });

Expand All @@ -61,7 +61,10 @@ const getFormSchema = (
actions: {
validations: [
{
validator: validateRuleActionsField(actionTypeRegistry),
// Debounced validator not explicitly necessary here as the `RuleActionsFormData` form doesn't exhibit the same
// behavior as the `ActionsStepRule` form outlined in https://github.com/elastic/kibana/issues/142217, however
// additional renders are prevented so using for consistency
validator: debouncedValidateRuleActionsField(actionTypeRegistry),
},
],
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,10 @@ export const RuleActionsField: React.FC<Props> = ({ field, messageVariables }) =
triggersActionsUi: { getActionForm },
} = useKibana().services;

// Workaround for setAlertActionsProperty being fired with prevProps when followed by setActionIdByIndex
// For details see: https://github.com/elastic/kibana/issues/142217
const [isInitializingAction, setIsInitializingAction] = useState(false);

const actions: RuleAction[] = useMemo(
() => (!isEmpty(field.value) ? (field.value as RuleAction[]) : []),
[field.value]
Expand All @@ -83,6 +87,9 @@ export const RuleActionsField: React.FC<Props> = ({ field, messageVariables }) =
const setActionIdByIndex = useCallback(
(id: string, index: number) => {
const updatedActions = [...(actions as Array<Partial<RuleAction>>)];
if (isEmpty(updatedActions[index].params)) {
setIsInitializingAction(true);
}
updatedActions[index] = deepMerge(updatedActions[index], { id });
field.setValue(updatedActions);
},
Expand All @@ -98,24 +105,30 @@ export const RuleActionsField: React.FC<Props> = ({ field, messageVariables }) =
(key: string, value: RuleActionParam, index: number) => {
// validation is not triggered correctly when actions params updated (more details in https://github.com/elastic/kibana/issues/142217)
// wrapping field.setValue in setTimeout fixes the issue above
// and triggers validation after params have been updated
setTimeout(
() =>
field.setValue((prevValue: RuleAction[]) => {
const updatedActions = [...prevValue];
updatedActions[index] = {
...updatedActions[index],
params: {
...updatedActions[index].params,
[key]: value,
},
};
return updatedActions;
}),
0
);
// and triggers validation after params have been updated, however it introduced a new issue where any additional input
// would result in the cursor jumping to the end of the text area (https://github.com/elastic/kibana/issues/149885)
const updateValue = () => {
field.setValue((prevValue: RuleAction[]) => {
const updatedActions = [...prevValue];
updatedActions[index] = {
...updatedActions[index],
params: {
...updatedActions[index].params,
[key]: value,
},
};
return updatedActions;
});
};

if (isInitializingAction) {
setTimeout(updateValue, 0);
setIsInitializingAction(false);
} else {
updateValue();
}
},
[field]
[field, isInitializingAction]
);

const actionForm = useMemo(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import { i18n } from '@kbn/i18n';

import type { ActionTypeRegistryContract } from '@kbn/triggers-actions-ui-plugin/public';
import { validateRuleActionsField } from '../../../containers/detection_engine/rules/validate_rule_actions_field';
import { debouncedValidateRuleActionsField } from '../../../containers/detection_engine/rules/validate_rule_actions_field';

import type { FormSchema } from '../../../../shared_imports';
import type { ActionsStepRule } from '../../../pages/detection_engine/rules/types';
Expand All @@ -21,7 +21,9 @@ export const getSchema = ({
actions: {
validations: [
{
validator: validateRuleActionsField(actionTypeRegistry),
// Debounced validator is necessary here to prevent error validation
// flashing when first adding an action. Also prevents additional renders
validator: debouncedValidateRuleActionsField(actionTypeRegistry),
},
],
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@
*/

export { validateRuleActionsField } from './validate_rule_actions_field';
export { debouncedValidateRuleActionsField } from './validate_rule_actions_field';
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,22 @@

/* istanbul ignore file */

import type {
ValidationCancelablePromise,
ValidationFuncArg,
ValidationResponsePromise,
} from '@kbn/es-ui-shared-plugin/static/forms/hook_form_lib';
import type {
RuleAction,
ActionTypeRegistryContract,
} from '@kbn/triggers-actions-ui-plugin/public';
import type { ValidationFunc, ERROR_CODE, ValidationError } from '../../../../../shared_imports';
import type { RuleActionsFormData } from '../../../../../detection_engine/rule_management_ui/components/rules_table/bulk_actions/forms/rule_actions_form';
import type { ActionsStepRule } from '../../../../pages/detection_engine/rules/types';
import type { ValidationFunc, ERROR_CODE } from '../../../../../shared_imports';
import { getActionTypeName, validateMustache, validateActionParams } from './utils';

export const DEFAULT_VALIDATION_TIMEOUT = 100;

export const validateSingleAction = async (
actionItem: RuleAction,
actionTypeRegistry: ActionTypeRegistryContract
Expand All @@ -26,9 +35,7 @@ export const validateSingleAction = async (

export const validateRuleActionsField =
(actionTypeRegistry: ActionTypeRegistryContract) =>
async (
...data: Parameters<ValidationFunc>
): Promise<ValidationError<ERROR_CODE> | void | undefined> => {
async (...data: Parameters<ValidationFunc>): ValidationResponsePromise<ERROR_CODE> => {
const [{ value, path }] = data as [{ value: RuleAction[]; path: string }];

const errors = [];
Expand All @@ -51,3 +58,40 @@ export const validateRuleActionsField =
};
}
};

/**
* Debounces validation by canceling previous validation requests. Essentially leveraging the async validation
* cancellation behavior from the hook_form_lib. Necessary to prevent error validation flashing when first adding an
* action until root cause of https://github.com/elastic/kibana/issues/142217 is found
*
* See docs for details:
* https://docs.elastic.dev/form-lib/examples/validation#cancel-asynchronous-validation
*
* Note: _.throttle/debounce does not have async support, and so not used https://github.com/lodash/lodash/issues/4815.
*
* @param actionTypeRegistry
* @param defaultValidationTimeout
*/
export const debouncedValidateRuleActionsField =
(
actionTypeRegistry: ActionTypeRegistryContract,
defaultValidationTimeout = DEFAULT_VALIDATION_TIMEOUT
) =>
(data: ValidationFuncArg<ActionsStepRule | RuleActionsFormData>): ValidationResponsePromise => {
let isCanceled = false;
const promise: ValidationCancelablePromise = new Promise((resolve) => {
setTimeout(() => {
if (isCanceled) {
resolve();
} else {
resolve(validateRuleActionsField(actionTypeRegistry)(data));
}
}, defaultValidationTimeout);
});

promise.cancel = () => {
isCanceled = true;
};

return promise;
};

0 comments on commit 176eff5

Please sign in to comment.