Skip to content

[Security Solution] fixes rule preview works for form's invalid state#213570

Closed
denar50 wants to merge 4 commits intomainfrom
173930-enhance-rule-preview-enabled-state-check
Closed

[Security Solution] fixes rule preview works for form's invalid state#213570
denar50 wants to merge 4 commits intomainfrom
173930-enhance-rule-preview-enabled-state-check

Conversation

@denar50
Copy link
Contributor

@denar50 denar50 commented Mar 7, 2025

Summary

Enhance the function getIsRulePreviewDisabled so that it takes into account whether the threshold fields in the define step form are valid or not.
Additionally, this PR fixes the check for machine_learning inside getIsRulePreviewDisabled to return true when the learning job id list is empty.

Why not using the isValid property from the define step form to check whether the preview button is enabled or not?

That property is not populated until the validate function of the form is invoked. That function is asynchronous and therefore it is hard to call it on every render to determine if the preview button is disabled or not.
Alternatively we can change the preview button to act as a submit button. In this case it will always be enabled, and when the user clicks, the validation is triggered and the user will see the validation errors on the left hand side of the screen. The problem with this approach is that since the rule type selection buttons might be so big that it might be hard for the user to notice there are errors without scrolling the page.

This solves issue #173930

@denar50 denar50 requested a review from a team as a code owner March 7, 2025 14:06
@denar50 denar50 requested a review from nkhristinin March 7, 2025 14:06
@denar50 denar50 changed the title [Security Solution][Detection Engine] fixes rule preview works for form's invalid state (#173930) [Security Solution] fixes rule preview works for form's invalid state Mar 7, 2025
@denar50 denar50 added backport:version Backport to applied version labels v8.17.4 v9.0.0 v8.18.0 v9.0.1 and removed v9.0.1 labels Mar 7, 2025
@denar50 denar50 added Feature:Detection Rules Security Solution rules and Detection Engine Feature:Detection Rule Preview Security Solution Detection Rule Preview feature Team:Detection Engine Security Solution Detection Engine Area labels Mar 7, 2025
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detection-engine (Team:Detection Engine)

@denar50 denar50 self-assigned this Mar 7, 2025
@elasticmachine
Copy link
Contributor

elasticmachine commented Mar 7, 2025

💔 Build Failed

Failed CI Steps

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 8.9MB 8.9MB +349.0B

History

cc @denar50

Copy link
Contributor

@nkhristinin nkhristinin left a comment

Choose a reason for hiding this comment

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

For me, it looks like we should have just one place that determines whether a rule is valid or not.

I like the idea of performing async validation when we click the Preview button.

Maybe, if the form is not valid, we can show a general message in the preview layout indicating that the rule is invalid.

Otherwise, we might face a similar situation in the future when adding new fields or validations, requiring us to duplicate the logic.

@denar50
Copy link
Contributor Author

denar50 commented Mar 10, 2025

For me, it looks like we should have just one place that determines whether a rule is valid or not.

I like the idea of performing async validation when we click the Preview button.

Maybe, if the form is not valid, we can show a general message in the preview layout indicating that the rule is invalid.

Otherwise, we might face a similar situation in the future when adding new fields or validations, requiring us to duplicate the logic.

Totally agree with you. I opened a new PR with the implementation of the new approach #213801

@denar50 denar50 closed this Mar 10, 2025
@sophiec20 sophiec20 deleted the 173930-enhance-rule-preview-enabled-state-check branch February 9, 2026 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:version Backport to applied version labels Feature:Detection Rule Preview Security Solution Detection Rule Preview feature Feature:Detection Rules Security Solution rules and Detection Engine release_note:fix Team:Detection Engine Security Solution Detection Engine Area v8.17.4 v8.18.0 v9.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants