fix(ui): Prevent a React error for input fields

[mnonnenmacher]

Make sure that the input value is never null, this prevents the error:

A component is changing an uncontrolled input to be controlled.

[mnonnenmacher]

@lamppu @Etsija This change prevents the error message, but I'm not sure if it is correct or if it is intended that the value can be undefined in some situations?

[Etsija]

I'm not sure I'd change the component itself like this, as undefined values have the advantage that the corresponding value isn't sent to back-end from the form, but omitted completely. If value can be '' which is non-null, then additional logic would be needed in forms to prevent sending empty strings as objects.

There are also some dangerous data structures in the back-end, like for environment variables, where sending empty names and/or values is quite acceptable, but leads to Analyzer failure.

All in all, I see value in your PR, but I think we'd better think of a good and concise way to always return meaningful values from forms. Luckily, we do validate each form with Zod, but I think those validations are a bit imperfect in our UI atm.

Your PR would actually make sense, as long as we always prevent empty strings being sent from forms where this restriction is needed, and we can do this easily by validating an Input field which allows for an empty string, with

const nonEmptyStringSchema = z.string().min(1).optional()

or something similar.

[mnonnenmacher]

I'm not sure I'd change the component itself like this, as undefined values have the advantage that the corresponding value isn't sent to back-end from the form, but omitted completely. If value can be '' which is non-null, then additional logic would be needed in forms to prevent sending empty strings as objects.

There are also some dangerous data structures in the back-end, like for environment variables, where sending empty names and/or values is quite acceptable, but leads to Analyzer failure.

All in all, I see value in your PR, but I think we'd better think of a good and concise way to always return meaningful values from forms. Luckily, we do validate each form with Zod, but I think those validations are a bit imperfect in our UI atm.

Your PR would actually make sense, as long as we always prevent empty strings being sent from forms where this restriction is needed, and we can do this easily by validating an Input field which allows for an empty string, with

const nonEmptyStringSchema = z.string().min(1).optional()

or something similar.

I think even the current implementation is not always behaving as intended, because as soon as you click on an input field the React error is triggered, meaning that React is changing the default undefined to something else. For the user that change is not visible.

The FAQ does not explain what is the consequence of the error, but it clearer states that the current implementation is wrong:
https://react.dev/reference/react-dom/components/input#im-getting-an-error-a-component-is-changing-an-uncontrolled-input-to-be-controlled

Do you have some more examples for text fields that should make that distinction between undefined and empty string?

[Etsija]

I'll continue the discussion tomorrow, but meanwhile, here's some good reading about the issue: shadcn-ui/ui#690, shadcn-ui/ui#410

[Etsija]

In PR #1947 you can get rid of the uncontrolled->controlled errors coming from name and value fields by specifying default values for them in the useForm hook. Here's an example for the organization secrets:

  const form = useForm<z.infer<typeof formSchema>>({
    resolver: zodResolver(formSchema),
    defaultValues: {
      name: '',
      value: '',
    },
  });

The description field is a bit trickier, due to it being specified as optional and nullable in the OpenAPI spec exposed to the UI:

export type CreateSecret = {
  description?: string | null;
  name: string;
  value: string;
};

If you just set a default value of '' for it in the useForm hook, then we can get rid of the uncontrolled->controlled error also for that field. The drawback is, even if we leave the description field empty, we always send it in the form payload:

{
  "name": "aa",
  "value": "bb",
  "description": ""
}

If we are happy with that, then the problem is solved for now. In case we don't want to send optional fields with defaults (like empty strings) in the form payload, the issues for shadcn-ui linked above do give some nice looking solutions, which could be a topic for a future PR.

---

When it comes to this PR, rather than changing the Input component as you suggested, I'd get rid of the React errors by simply specifying defaults to the useForm hook as described above. Come to think of it, this PR would probably also bring us into trouble when numeric input is required, since I don't think a number (like a port number, which is requested for in the Notifier section of the ORT run creation form) can be defaulted to a ''.

[mnonnenmacher]

If we are happy with that, then the problem is solved for now. In case we don't want to send optional fields with defaults (like empty strings) in the form payload, the issues for shadcn-ui linked above do give some nice looking solutions, which could be a topic for a future PR.

I think that should be fine, but the fix is not urgent so how about discussing it in the community call next week?

[Etsija]

I think that should be fine, but the fix is not urgent so how about discussing it in the community call next week?

Fine by me.