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] Fix cursor jumping to end of text area when editing Rule Action Message #150823

Merged
merged 6 commits into from Feb 16, 2023

Conversation

spong
Copy link
Member

@spong spong commented Feb 9, 2023

Summary

Resolves: #149885

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

As mentioned in #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).

The fix here is a modification of the fix provided in #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:

After:

@spong spong added bug Fixes for quality problems that affect the customer experience release_note:fix Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detection Rule Management Security Detection Rule Management Team v8.7.0 v8.8.0 labels Feb 13, 2023
@spong spong marked this pull request as ready for review February 13, 2023 22:57
@spong spong requested a review from a team as a code owner February 13, 2023 22:57
@spong spong requested a review from maximpn February 13, 2023 22:57
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@spong spong added the Feature:Rule Actions Security Solution Rule Actions feature label Feb 13, 2023
@@ -21,7 +21,8 @@ export const getSchema = ({
actions: {
validations: [
{
validator: validateRuleActionsField(actionTypeRegistry),
// Throttled validator is necessary here to prevent error validation flashing when first adding an action
validator: throttledValidateRuleActionsField(actionTypeRegistry),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lodash throttle potentially could've been used here instead, however it doesn't work with async functions, so just deferred to leveraging actual cancellable async validation via the hooks form lib itself (docs).

Copy link
Contributor

@maximpn maximpn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spong the changes look good, thank you for fixing such and annoying bug 👍

I have some general concerns. It was time consuming to follow to the root cause. For me as a library consumer I expect straightforward usage but having things like an isInitializingAction flag or setTimeout with zero delay looks like a workaround for some problems. I see that some Kibana teams use react-hook-form which looks quite similar to Kibana's form lib. I'm wonder if we can achieve the same functionality with react-hook-form but reduce the codebase size and avoid problems we got here and need to overcome.

data: ValidationFuncArg<ActionsStepRule | RuleActionsFormData>
): Promise<ValidationError | void | undefined> => {
let isCanceled = false;
const promise: Promise<ValidationError | void | undefined> & { cancel?(): void } = new Promise(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: it should be possible to use ValidationCancelablePromise here for type declaration

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! 👍 Was also able to use ValidationResponsePromise<ERROR_CODE> above on #36 and ValidationResponsePromise on #72 as well 🙂

*
* @param actionTypeRegistry
*/
export const throttledValidateRuleActionsField =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't wanna be naggy here but rather wanna have clear understanding on what's going on under the hood. I didn't go into deep implementation details of form lib but I have an impression that it's rather debouncing than throttling if look at the code. If validations happens too fast then isCancelled flag is set to true before evaluation of the if condition. This means the validation will be performed only after timeout and new validation requests don't come up which is basically debouncing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh you're not being naggy at all -- I appreciate the thoroughness @maximpn as I may've gotten a little myopic after trying to hunt down a fix here. 😅

This is indeed behaving as a debounce, nice catch! I'll update the function name and jsdoc to reflect that it's a debounce and clearly mention that we're essentially leveraging the async validation cancellation behavior from the hook_form_lib as such.

@spong
Copy link
Member Author

spong commented Feb 16, 2023

@spong the changes look good, thank you for fixing such and annoying bug 👍

I have some general concerns. It was time consuming to follow to the root cause. For me as a library consumer I expect straightforward usage but having things like an isInitializingAction flag or setTimeout with zero delay looks like a workaround for some problems. I see that some Kibana teams use react-hook-form which looks quite similar to Kibana's form lib. I'm wonder if we can achieve the same functionality with react-hook-form but reduce the codebase size and avoid problems we got here and need to overcome.

Absolutely, I 100% agree @maximpn. Having not been in this section of the codebase for some time it was hard to grok the control flow between the security code, trigger_actions_ui, and our (outdated) usage of the hook_form_lib. I was hoping for a more surgical root cause fix of #142217, but I wasn't able to find one without requiring a bit more refactoring.

As such, I went ahead and opened #151202 to capture this tech debt/refactoring effort, so we can re-evaluate our implementation (control flow, re-use of trigger_actions components, form libs, etc) as part of this 🙂

@spong spong enabled auto-merge (squash) February 16, 2023 19:58
@spong spong merged commit 736759a into elastic:main Feb 16, 2023
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
securitySolution 13.9MB 13.9MB +856.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @spong

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Feb 16, 2023
…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)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.7

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

@spong spong deleted the fix-action-message-cursor-issue branch February 16, 2023 22:14
kibanamachine added a commit that referenced this pull request Feb 16, 2023
… editing Rule Action Message (#150823) (#151518)

# Backport

This will backport the following commits from `main` to `8.7`:
- [[Security Solution] Fix cursor jumping to end of text area when
editing Rule Action Message
(#150823)](#150823)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Garrett
Spong","email":"spong@users.noreply.github.com"},"sourceCommit":{"committedDate":"2023-02-16T21:05:26Z","message":"[Security
Solution] Fix cursor jumping to end of text area when editing Rule
Action Message (#150823)\n\n## Summary\r\n\r\nResolves:
#149885 additional
details, please
see\r\nhttps://github.com//pull/141811#discussion_r981346237
and\r\nhttps://github.com//issues/142217.\r\n\r\nAs
mentioned in #142217,
this\r\nissue is the result of managing stale events and timings
(renderings +\r\nfield updates) between the Detections
`RuleActionsField` component,\r\nvalidation within the hooks-form lib,
and field updates coming from the\r\n`trigger_actions_ui`
components.\r\n\r\nNote: this behavior is explicit to the Edit Rule flow
(`ActionsStepRule`\r\nform), and not the Bulk Actions flyout
(`RuleActionsFormData` form) as\r\nevents flow as intended in the
latter, presumably because of all the\r\nnested components and use of
`useFormData` within the Edit Rule flow\r\n(see [form
libs\r\ndocs](https://docs.elastic.dev/form-lib/examples/listening-to-changes#forward-the-form-state-to-a-parent-component)).\r\n\r\nThe
fix here is a modification of the fix provided
in\r\nhttps://github.com//pull/141811 with `setTimeout`,
but\r\ninstead of always pushing the params update to be within a
`setTimeout`,\r\nit now only does it when initially loading `Actions`
with empty\r\n`Params`. Since this fix also has the unintended
side-effect of\r\nflickering the validation error callout when first
adding an action,\r\nvalidation is now throttled to 100ms intervals,
which also helps with\r\nextraneous re-renders.\r\n\r\nBefore initial
fix (with cursor issue) / Before throttle fix w/ flashing\r\nerror
callout:\r\n<p align=\"center\">\r\n<img
width=\"350\"\r\nsrc=\"https://user-images.githubusercontent.com/2946766/215585424-8c056440-5906-4168-b997-aea3690c7ea5.gif\"\r\n/>
<img
width=\"242\"\r\nsrc=\"https://user-images.githubusercontent.com/2946766/217961284-704013b7-2f0a-4a24-95a4-4183348448f4.gif\"\r\n/>\r\n</p>\r\n\r\nAfter:\r\n<p
align=\"center\">\r\n<img
width=\"242\"\r\nsrc=\"https://user-images.githubusercontent.com/2946766/218591986-e52456e0-e374-4d8e-bc84-9fb6da69f1f9.gif\"\r\n/>\r\n</p>","sha":"736759af76623966684e6dbb06009f1517a8d8a8","branchLabelMapping":{"^v8.8.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:fix","Team:Detections
and Resp","Team: SecuritySolution","Feature:Rule
Actions","Team:Detection
Rules","v8.7.0","v8.8.0"],"number":150823,"url":"#150823
Solution] Fix cursor jumping to end of text area when editing Rule
Action Message (#150823)\n\n## Summary\r\n\r\nResolves:
#149885 additional
details, please
see\r\nhttps://github.com//pull/141811#discussion_r981346237
and\r\nhttps://github.com//issues/142217.\r\n\r\nAs
mentioned in #142217,
this\r\nissue is the result of managing stale events and timings
(renderings +\r\nfield updates) between the Detections
`RuleActionsField` component,\r\nvalidation within the hooks-form lib,
and field updates coming from the\r\n`trigger_actions_ui`
components.\r\n\r\nNote: this behavior is explicit to the Edit Rule flow
(`ActionsStepRule`\r\nform), and not the Bulk Actions flyout
(`RuleActionsFormData` form) as\r\nevents flow as intended in the
latter, presumably because of all the\r\nnested components and use of
`useFormData` within the Edit Rule flow\r\n(see [form
libs\r\ndocs](https://docs.elastic.dev/form-lib/examples/listening-to-changes#forward-the-form-state-to-a-parent-component)).\r\n\r\nThe
fix here is a modification of the fix provided
in\r\nhttps://github.com//pull/141811 with `setTimeout`,
but\r\ninstead of always pushing the params update to be within a
`setTimeout`,\r\nit now only does it when initially loading `Actions`
with empty\r\n`Params`. Since this fix also has the unintended
side-effect of\r\nflickering the validation error callout when first
adding an action,\r\nvalidation is now throttled to 100ms intervals,
which also helps with\r\nextraneous re-renders.\r\n\r\nBefore initial
fix (with cursor issue) / Before throttle fix w/ flashing\r\nerror
callout:\r\n<p align=\"center\">\r\n<img
width=\"350\"\r\nsrc=\"https://user-images.githubusercontent.com/2946766/215585424-8c056440-5906-4168-b997-aea3690c7ea5.gif\"\r\n/>
<img
width=\"242\"\r\nsrc=\"https://user-images.githubusercontent.com/2946766/217961284-704013b7-2f0a-4a24-95a4-4183348448f4.gif\"\r\n/>\r\n</p>\r\n\r\nAfter:\r\n<p
align=\"center\">\r\n<img
width=\"242\"\r\nsrc=\"https://user-images.githubusercontent.com/2946766/218591986-e52456e0-e374-4d8e-bc84-9fb6da69f1f9.gif\"\r\n/>\r\n</p>","sha":"736759af76623966684e6dbb06009f1517a8d8a8"}},"sourceBranch":"main","suggestedTargetBranches":["8.7"],"targetPullRequestStates":[{"branch":"8.7","label":"v8.7.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.8.0","labelRegex":"^v8.8.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/150823","number":150823,"mergeCommit":{"message":"[Security
Solution] Fix cursor jumping to end of text area when editing Rule
Action Message (#150823)\n\n## Summary\r\n\r\nResolves:
#149885 additional
details, please
see\r\nhttps://github.com//pull/141811#discussion_r981346237
and\r\nhttps://github.com//issues/142217.\r\n\r\nAs
mentioned in #142217,
this\r\nissue is the result of managing stale events and timings
(renderings +\r\nfield updates) between the Detections
`RuleActionsField` component,\r\nvalidation within the hooks-form lib,
and field updates coming from the\r\n`trigger_actions_ui`
components.\r\n\r\nNote: this behavior is explicit to the Edit Rule flow
(`ActionsStepRule`\r\nform), and not the Bulk Actions flyout
(`RuleActionsFormData` form) as\r\nevents flow as intended in the
latter, presumably because of all the\r\nnested components and use of
`useFormData` within the Edit Rule flow\r\n(see [form
libs\r\ndocs](https://docs.elastic.dev/form-lib/examples/listening-to-changes#forward-the-form-state-to-a-parent-component)).\r\n\r\nThe
fix here is a modification of the fix provided
in\r\nhttps://github.com//pull/141811 with `setTimeout`,
but\r\ninstead of always pushing the params update to be within a
`setTimeout`,\r\nit now only does it when initially loading `Actions`
with empty\r\n`Params`. Since this fix also has the unintended
side-effect of\r\nflickering the validation error callout when first
adding an action,\r\nvalidation is now throttled to 100ms intervals,
which also helps with\r\nextraneous re-renders.\r\n\r\nBefore initial
fix (with cursor issue) / Before throttle fix w/ flashing\r\nerror
callout:\r\n<p align=\"center\">\r\n<img
width=\"350\"\r\nsrc=\"https://user-images.githubusercontent.com/2946766/215585424-8c056440-5906-4168-b997-aea3690c7ea5.gif\"\r\n/>
<img
width=\"242\"\r\nsrc=\"https://user-images.githubusercontent.com/2946766/217961284-704013b7-2f0a-4a24-95a4-4183348448f4.gif\"\r\n/>\r\n</p>\r\n\r\nAfter:\r\n<p
align=\"center\">\r\n<img
width=\"242\"\r\nsrc=\"https://user-images.githubusercontent.com/2946766/218591986-e52456e0-e374-4d8e-bc84-9fb6da69f1f9.gif\"\r\n/>\r\n</p>","sha":"736759af76623966684e6dbb06009f1517a8d8a8"}}]}]
BACKPORT-->

Co-authored-by: Garrett Spong <spong@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Rule Actions Security Solution Rule Actions feature release_note:fix 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. v8.7.0 v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Security Solution] Editing the Action Message of a detection rule results in cursor jumping to end
5 participants