Allow adding multiple ssh keys at once

[jelly]

The initial version of the ssh key functionality would only add the first key if multiple where provided. Allow adding multiple keys in via a new helper, the validation now needs to happen via SshKeysRow as they are added dynamically and the value provided via properties.

Closes #1300

[cockpituous]

These 2 added lines are not executed by any test. Details

[cockpituous]

These 2 added lines are not executed by any test. Details

[mvollmer]

This is true. The test with a \n between two SSH keys never creates a second item. The \n does not make it into the textrea with our set_input_text function on Chromium, one has to use \r ro produce a line break.

The test still passes since the check for expected keys is pretty sloppy and doesn't consider line breaks either.

[martinpitt]

I think the original debounce comment still applies here -- this useEffect() re-runs on every input, so it creates multiple parallel timers and calls parseKey() for each one as well. Some debouncing still needs to happen.

[jelly]

The issue with a global debounce is that it is used for multiple components, so if you add two keys quickly only one will get parsed as the other call is debounced. So debouncing needs to happen per Component and we can't keep the debounce function in the component as every re-render it would be re-instantiated. So I believe the current approach with setTimeout debounces correctly, when a new as a new value would cause a re-render so clearTimeout() to be called?

If I'm wrong I'm happy to fix it if there is a better way.

[martinpitt]

I'm not saying that the original wrapping of parseKey() into debounce was right -- it's not, it's a hack which doesn't belong there at all. I'm saying that I dislike useEffect() here. It's too disconnected from the actual input that it wants to debounce, it duplicates debouncing logic, has setState() calls in it which are prone to recursion, is too hard to understand, and above of all, it really shouldn't be necessary here.

I mean, what is this trying to do? It's a controlled component, so onChange must immediately call setKeyValue() so that the UI doesn't lag. The expensive bit is the validation because of parseKey(), so the onChange handler should wrap that part into a debunce(). None of that requires useEffect()?

(That said: of course I may miss something here)

[jelly]

We can not make onChange call setKeyValue simply because onChange will add a new row if multiple keys are added using additem(). That is the hard part, dynamically creating a new row component.

[martinpitt]

That sounds weird -- in JSX it's just a single TextArea. If this somehow multiplies the TextArea, this needs fixing -- either we shouldn't multiply it, or have a full-blown dynamic set of input lines where each one just has a single key, and ➕ 🗑️ buttons

[mvollmer]

we can't keep the debounce function in the component as every re-render it would be re-instantiated.

We can use a hook to avoid the re-instantiation on re-render. Like this:

diff --git a/src/components/create-vm-dialog/createVmDialog.jsx b/src/components/create-vm-dialog/createVmDialog.jsx
index 8a2dd72..88ce41a 100644
--- a/src/components/create-vm-dialog/createVmDialog.jsx
+++ b/src/components/create-vm-dialog/createVmDialog.jsx
@@ -37,6 +37,7 @@ import { Spinner } from "@patternfly/react-core/dist/esm/components/Spinner";
 import { ExternalLinkAltIcon, TrashIcon } from '@patternfly/react-icons';
 
 import { DialogsContext } from 'dialogs.jsx';
+import { useInit } from "hooks.js";
 import cockpit from 'cockpit';
 import store from "../../store.js";
 import { MachinesConnectionSelector } from '../common/machinesConnectionSelector.jsx';
@@ -752,13 +753,16 @@ const UsersConfigurationRow = ({
     );
 };
 
-// This method needs to be outside of component as re-render would create a new instance of debounce
-const parseKey = debounce(500, (key, setKeyObject, setKeyInvalid) => {
-    if (isEmpty(key))
-        return;
+const useParsedKey = (value) => {
+    const [keyObject, setKeyObject] = useState();
+    const [keyInvalid, setKeyInvalid] = useState(false);
+
+    const parseKey = (key) => {
+        if (isEmpty(key))
+            return;
 
-    // Validate correctness of the key
-    return cockpit.spawn(["ssh-keygen", "-l", "-f", "-"], { err: "message" })
+        // Validate correctness of the key
+        return cockpit.spawn(["ssh-keygen", "-l", "-f", "-"], { err: "message" })
             .input(key)
             .then(() => {
                 setKeyInvalid(false);
@@ -776,21 +780,33 @@ const parseKey = debounce(500, (key, setKeyObject, setKeyInvalid) => {
                 setKeyInvalid(true);
                 console.warn("Could not validate the public key");
             });
-});
+    };
+
+    const debouncedParseKey = useInit(() => debounce(500, parseKey));
+    useInit(() => debouncedParseKey(value), [ value ]);
+
+    return [keyObject, keyInvalid];
+};
 
 const SshKeysRow = ({
-    id, item, onChange, idx, removeitem,
+    id, item, onChange, idx, removeitem, additem
 }) => {
-    const [keyObject, setKeyObject] = useState();
-    const [keyInvalid, setKeyInvalid] = useState(false);
+    const [keyObject, keyInvalid] = useParsedKey(item.value);
 
     const onChangeHelper = (value) => {
-        // Some users might want to input multiple keys into the same field
-        // While handling that in the future might be a nice user experience, now we only parse one key out of input
-        value = value.split(/\r?\n/)[0];
+        const lines = value.split(/\r?\n/);
 
+        // The first line is for us.
+        value = lines[0];
         onChange(idx, "value", value);
-        parseKey(value, setKeyObject, setKeyInvalid);
+
+        // If there are more lines, create additional entries for them.
+        for (let i = 1; i < lines.length; i++) {
+            if (lines[i].length > 0) {
+                additem();
+                onChange(idx + i, "value", lines[i]);
+            }
+        }
     };
 
     return (

[KKoukiou]

Does this allow to input multiple keys in one text area field? If so, this seems a bit off by design perspective.

[mvollmer]

My version of this: #1515

[mvollmer]

This means that the last character in the textarea can never be deleted.

[jelly]

Fixed in #1515