[EDR Workflows] Error message 'the value already exists' shown for duplicate values with the 'is one of' operator on Blocklist tab #196071

szwarckonrad · 2024-10-14T09:41:00Z

These changes ensure that the warning displayed to the user for duplicate values remains visible after onBlur. I’ve opted to keep it as a warning rather than an error since entering a duplicate value doesn’t allow the user to “save” it as a selected item in the combo box—it remains an active string in the input field. We don’t block form submission due to this warning; any unselected value in the combo box is stripped out. The red border on the combo box is part of EUI’s behavior when attempting to select a duplicate, and I don’t see an easy way to modify this at the moment.

Screen.Recording.2024-10-14.at.11.25.47.mov

elasticmachine · 2024-10-14T09:46:57Z

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

joeypoon · 2024-10-14T15:45:04Z

...ugins/security_solution/public/management/pages/blocklist/view/components/blocklist_form.tsx

-      value: false,
-    });
+    const [nameVisited, setNameVisited] = useState(false);
+    const [valueVisited, setValueVisited] = useState({ value: false }); // Use object to trigger re-render


So take this with a grain of salt 🧂 because I haven't really touched frontend code in nearly 3 years but this feels a little bit weird. Not a blocker or anything but I do wonder if there's a better approach.

i agree with joey here, this doesn't look like a good practice.

i guess it is needed, because warnings and errors are stored in reference values, but i don't see why we need to store those in refs instead of good ol' states. i'd suggest to try to remove the refs, and use states (or maybe one state if they always change together) to improve this file

gergoabraham

thanks for the changes! it works well 👍

could you add a test to cover the fixed behaviour?

also, if you have time for it, i think it would be great to improve on this file and remove the unnecessary looking refs

gergoabraham · 2024-10-21T12:53:51Z

...ugins/security_solution/public/management/pages/blocklist/view/components/blocklist_form.tsx

-      value: false,
-    });
+    const [nameVisited, setNameVisited] = useState(false);
+    const [valueVisited, setValueVisited] = useState({ value: false }); // Use object to trigger re-render


i agree with joey here, this doesn't look like a good practice.

i guess it is needed, because warnings and errors are stored in reference values, but i don't see why we need to store those in refs instead of good ol' states. i'd suggest to try to remove the refs, and use states (or maybe one state if they always change together) to improve this file

… into fix-is-one-of-combo-box-warning

szwarckonrad · 2024-10-22T10:47:00Z

@gergoabraham
I agree that we should drop refs in favor of state, and that rerendering as a side effect of a state change needs to be addressed. However, it seems like this would require a larger refactor, during which we should also rework the logic for handling the form in general. I would prefer this PR to address existing issue.

I've added an unit to cover this changes.

gergoabraham

alright, let's 🚢 it then. thanks for the tests! 🙇

elasticmachine · 2024-10-22T13:46:43Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 34494df

Failed CI Steps

Test Failures

[job] [logs] Serverless Osquery Cypress Tests #2 / Alert Event Details - Response Actions Form adds response actions with osquery with proper validation and form values adds response actions with osquery with proper validation and form values
[job] [logs] FTR Configs #32 / Alerting alerts_as_data alerts as data flapping should allow rule specific flapping to override space flapping

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`	20.7MB	20.7MB	+148.0B