🪟🎉 Connector builder: Allow defining inputs

[flash1293]

What

Built on top of #20385

Closes #20414

How

Add inputs management and write to underlying manifest:

The InputsView

Extended the BuilderView by a new view (handled like the others). The InputsView renders all the inputs using the formik context. When editing an input, a new form context is opened for the modal to do the editing in a separate state (called InputInEditing) to allow to cancel the action.

When mapping to the spec Array<{ key: string, definition: JSON7Schema, required: boolean }> is mapped to a json schema object conforming to the spec property of the low code manifest. This can later be done in reverse as well.

The InputInEditing shape holds a few more things than the raw json schema to handle some form-specific things:

• There is a type "enum" which translates into type: string and enum: < the list of possible values > in the json schema - for this reason the "type" needs to be handled outside of the actual json schema definition and mapped back once the user submits the modal form
• Additional state to show or hide the default value field

The field id (the key of the property in the final spec) is derived from the name. It also has a validation that blocks duplicated field ids. I had to touch the input styling a bit, but IMHO it's for the better in general (read-only looks like disabled now, before that it would look like a regular input)

There is a list of explicitly supported types for properties - if another one is used, the form is only partially rendered, with a warning that you need to switch to the yaml to edit this. It won't show right now but with our current plan for state management if will become important in the near future.

The ConfigMenu

If you can edit the spec while there is a config, you can run into weird situations. Added a reset button to reset the form if that happens to a user.

The BuilderField

To simplify a bit the inputs view code I aligned the types of a jsonschema property with the types the BuilderField component takes (this included renaming text to string). Seems like being consistent here doesn't hurt.

The state service

Patched the connector manifest so the spec is part of it (can be removed once #20422 is merged)

Also as there can't be invalid json anymore as the user can't edit the test input directly, we don't need to separate states to track the string and the json - simplified the code a bit.

The connector form

Added support for the reset button

I noticed that when adding default values, it's weird if they are not reflected in the config menu. However, the current logic does not allow to patch existing configs with new default values. I added an option for that which becomes active if the connector form is passed an unsaved draft of a connector definition spec (like in case of the connector builder). Also, for draft definition specs it will check whether the existing config can be cast into the new schema - if not it will reset automatically.

Recommended reading order

See the sections above

🚨 User Impact 🚨

User can configure inputs for their connector in the connector builder.

[lmossman]

Overall this is looking pretty great. Left some comments on issues I found when testing locally, so those should be addressed before merging

[lmossman]

Issues I found while testing locally:

1. Click Add new user input -> Toggle on Enable default value -> Type any value into Default value -> Switch type to array -> App crashes with Uncaught TypeError: fieldValue.map is not a function at TagInput.tsx:77:1
2. Click Add new user input -> Toggle on Enable default value -> Type any value into Default value -> Switch type to boolean -> Cannot modify the setting of the Default value toggle
   (both of the above may be solved by clearing the default value when switching the type)
3. I added an enum user input field, and when I opened up the Test Inputs screen, the dropdown just had ... selected instead of the first value
4. If you try to add a new user input with the same Field ID as another user input, the Create button is disabled but there is no explanation of why. It would be better UX if this displayed either a tooltip on the Create button which says that you can't add a second field with the same ID, or if that error was placed on the Field ID/name field directly

[flash1293]

What I did to fix these

• Clear default on type change as you suggested. I added a onChange prop to the builder field to do so as an effect hook will do it in another render cycle so the form would still render in the broken state
• For the dropdown with ... the issue was that the BuilderField would automatically select the first option if no value is set yet which is the wrong behavior IMHO (it looks like the first option is selected but this is not actually the case). So when you enable default values for an enum user input, it is not picking the first value, but instead you have to choose. The enum builder field is only used in one other place which is the http method picker and it always has a value by the default state of a stream.
• Errors about duplicate field IDs weren't shown right away because the field wasn't touched. I worked around this by auto-touching the key field and setting the title-derived key right away in the newly introduced onChange instead of waiting for a hook to trigger

[lmossman]

• Looks good
• That makes sense, I agree that it makes sense to make the user actually select a value if no default is set
• Nice, that seems to work well to me!

[lmossman]

I tried interpolating a test input (pokemon_name) that I added by setting the Path URL of a stream to /pokemon/{{pokemon_name}} , but it seems like the value I type into the Test Input menu is not being used.

Originally I thought this was because doing {{field_id}} was not actually the right way to interpolate it, as all of the examples in the low-code docs do {{config['field_id']}} to interpolate user input values. But even when I tried that, it still didn't work. (Though, this syntaxExample should still be updated to say something like {{config['my_input']}}, and we may want to file a separate issue for allowing users to just enter {{my_input}} instead -- thoughts?)

I just double checked and this is working on the feature/connector-builder branch: Open that branch, switch to the YAML view, and paste in this YAML:

version: "0.1.0"

streams:
  - type: DeclarativeStream
    $options:
      name: "pokemon"
      primary_key: "id"
      path: "/pokemon/{{config['pokemon_name']}}"
    primary_key: "id"
    retriever:
      requester:
        url_base: "https://pokeapi.co/api/v2"
        http_method: "GET"
      record_selector:
        extractor:
            field_pointer: []

check:
  stream_names:
    - "customers"

spec:
  documentation_url: https://docsurl.com
  connectionSpecification:
    title: Poke API Spec
    type: object
    required:
      - pokemon_name
    additionalProperties: true
    properties:
      pokemon_name:
        type: string
        description: Pokemon name

Then open the test inputs menu and enter ditto into the pokemon_name field. You can see that the URL is correctly pulling in the ditto value, and clicking Test properly fetches data about ditto.

But if you try the same thing on this branch, ditto isn't used in the URL. So this seems to be a regression specific to this PR.

[flash1293]

Yeah, I noticed this as well and fixed it in the authentication PR: https://github.com/airbytehq/airbyte/pull/20645/files#diff-05522b7f5df1df909f26423e40703073e6ab13f90f10ceeef5ca00a65561b351R88

Pulled it into this branch as well.

[lmossman]

Slightly weird UX here: If I open the config menu and all the inputs are blank, then I type a value into one of them, then click Reset, it doesn't clear the value.
It seems like it only clears values if they had been saved to the form before, likely because values that have not yet been saved arent set on ConfigJson so setting that to an empty object doesn't affect unsaved values.

Ideally this would always clear all values in all inputs when clicked.

[flash1293]

Fixed by also calling resetForm on the formik context to purge local formik state.

[lmossman]

nit: these labels currently look different from the labels in the other views, so we should make them consistent.

This should be achievable by changing this <div> to be a <Label> instead, so that the same styles are applied.

[flash1293]

done

[lmossman]

Also, not a must have but it could be cool if we showed the type of the field in smaller grey (maybe italicized?) text next to or underneath the title

[flash1293]

Let me split that out into a separate issue

[lmossman]

One small issue with using a here that I forgot about when I suggested it: Label has a small bottom padding in its styles, which means the label is now not lined up with the edit button in the UI here:

maybe you can override that bottom padding here or refactor those styles to remove it?

[lmossman]

One more small nit about the label padding, but otherwise this LGTM. Tested locally and it works great