Pod creation dialog state handling is broken

[martinpitt]

See #1832 (comment) for the whole investigation so far.

So @jelly concludes that we are doing all the state interaction wrong with validation..

The PodCreateModal has a state validationFailed and setValidationFailed setter.

validationFailed is pushed to the DynamicListForm as a property:

            <DynamicListForm id='create-pod-dialog-publish'
                        emptyStateString={_("No ports exposed")}
                        formclass='publish-port-form'
                        label={_("Port mapping")}
                        actionLabel={_("Add port mapping")}
                        validationFailed={validationFailed.publish}
                        onValidationChange={value => dynamicListOnValidationChange('publish', value)}
                        onChange={value => setPublish(value)}
                        default={{ IP: null, containerPort: null, hostPort: null, protocol: 'tcp' }}
                        itemcomponent={ <PublishPort />} />

This component has dynamicListOnValidationChange which is this function that modifies the state:

    const dynamicListOnValidationChange = (key, value) => {
        console.log("validationchange", key, value);
        setValidationFailed(prevState => {
            prevState[key] = value;
            if (prevState[key].every(a => a === undefined))
                delete prevState[key];
            return prevState;
        });
    };

The value passed comes from PublishPort:

                <TextInput id={id + "-container-port"}
                            type='number'
                            step={1}
                            min={1}
                            max={MAX_PORT}
                            validated={validationFailed?.containerPort ? "error" : "default"}
                            value={item.containerPort || ''}
                            onChange={(_event, value) => {
                                console.log("onChange", validationFailed, _event, value);
                                utils.validationClear(validationFailed, "containerPort", onValidationChange);
                                console.log({ ...validationFailed, containerPort: validatePublishPort(value, "containerPort") });
                                utils.validationDebounce(() => onValidationChange({ ...validationFailed, containerPort: validatePublishPort(value, "containerPort") }));
                                onChange(idx, 'containerPort', value);
                            }} />

So this modifies React state and then passes it to onValidationChange which is also a same named function in DynamicListForm:

                                return React.cloneElement(this.props.itemcomponent, {
                                    idx,
                                    item,
                                    id: id + "-" + idx,
                                    key: idx,
                                    onChange: this.onItemChange,
                                    removeitem: this.removeItem,
                                    additem: this.addItem,
                                    options: this.props.options,
                                    validationFailed: validationFailed && validationFailed[idx],
                                    onValidationChange: value => {
                                        // Dynamic list consists of multiple rows. Therefore validationFailed object is presented as an array where each item represents a row
                                        // Each row/item then consists of key-value pairs, which represent a field name and it's validation error
                                        const delta = validationFailed ? [...validationFailed] : [];
                                        // Update validation of only a single row
                                        delta[idx] = value;

                                        // If a row doesn't contain any fields with errors anymore, we delete the item of the array
                                        // Deleting an item of an array replaces an item with an "empty item".
                                        // This guarantees that an array of validation errors maps to the correct rows
                                        if (Object.keys(delta[idx]).length == 0)
                                            delete delta[idx];

                                        onValidationChange?.(delta);
                                    },
                                });

Modifies state again. This seems to be what causes all the inconsistent state issues

Originally posted by @jelly in #1832 (comment)

[jelly]

Lowering the debounce timeout from 500 to 50 or 100 ms makes the tests pass again on Firefox. Of course this is no silver bullet but it shows what the issue is. We debounce and set a value on an old validationFailed state

The debounce can be cancelled, but then we need to keep it in state, worth a shot.

[jelly]

Spent half a day on this there are so very "fun" problems:

validationFailed state is a mix:

{ 
    "publish": [{"hostPort: "lalalala"}]
    "podName": "string"
}

So one way I thought to fix this was introduce a validationClear which just clears the value but in PodCreateModal and not in the DynamicListForm.

                            onValidationClear("IP", idx);

This.. I thought worked but doesn't at all because, this is saved in state as { "publish": [{"IP": "whatever"}]}

So that's not great is it, we could add "publish" to the callback but then we still have:

utils.validationDebounce(() => onValidationUpdate("IP", idx, validatePublishPort(value, "IP")));

Another big problem, has no idea of "publish".

But in the end, let's ask the question. Why do we have validationFailed in PodCreateModal, well because of this:

        // If at least one group is invalid, then the whole form is invalid
        return validationFailed.publish?.some(groupHasError) ||
            !!validationFailed.podName;

Does that mean we have to export all internals back to PodCreateModal, not really right?

So can we keep validation state in DynamiListForm, this also needs to handle the removal so keeping things there would be easier. Only downside is calling back to the parent that the component is invalid.

Alternative is passing a useRef() that feels like a hack.

The benefit of moving validation to the DynamicListForm is that we can easily implement this in cockpit without breaking the changing much of the current API.