ClickHouse and overall connector improvement

[lovincyrus]

Closes https://linear.app/rilldata/issue/APP-22/improve-the-add-clickhouse-modal-to-account-for-dynamic-field-inputs

This pull request improves the data connector form with a new right panel to highlight the connection preview and help text. We'll also move the SubmissionError to top of the right panel once https://github.com/rilldata/rill/pull/7441 lands. Info icon with contextual message will be in a follow-up.

Checklist:

• [ ] Covered by tests
• [x] Ran it and it works as intended
• [x] Reviewed the diff before requesting a review
• [ ] Checked for unhandled edge cases
• [x] Linked the issues it closes
• [ ] Checked if the docs need to be updated. If so, create a separate Linear DOCS issue
• [ ] Intend to cherry-pick into the release branch
• [x] I'm proud of this work!

[ericokuma]

@jkhwu, FYI.

Some initial notes:

• [x] Match the height of the dialog in the mock. The height shouldn’t change based on the tab selection

• [x] The form field background should be white? Currently it looks like the fields are disabled

• [x] Remove the following text:

  

• [x] Let's have the connection preview display what we would in the actual YAML file. Example for Clickhouse:

  

[lovincyrus]

• Let's have the connection preview display what we would in the actual YAML file. Example for Clickhouse:

This currently displays the form states in YAML format, just like the interaction behavior in the embedded v0 posted in notion. What is the discrepancy here?

https://github.com/user-attachments/assets/e29cb9d6-5670-49e9-b178-626bcdc6cb38

[lovincyrus]

• The form field background should be white? Currently it looks like the fields are disabled

This is unrelated to the scope of this PR. It'd be great if we could keep the scope to the clickhouse connector improvement. We could follow up on the input field background color tweak so that the change is distinct and easily reversible. We should also tweak the icon spacing in the password field while we're on the form component nits.

Update: https://linear.app/rilldata/issue/ENG-815/refine-add-data-form-components

[lovincyrus]

• Remove the following text:

Shall we hide the connection preview section altogether when there is no form state?

[ericokuma]

I'm not able to get the entire functionality in v0 to be 100% detailed and correct. That being said, what you have looks good with the exception that the default text on load should just include the connector: clickhouse

As for the input field background color, happy to create a separate ticket to address that. I'm just wondering why it's even grey to begin with? Is that a problem with our default state of our components? cc @jkhwu

Lastly, the connection preview section is meant to preview the actual YAML configuration so default placeholder will just be:

connector: {connector_name}

another consideration is to always show the required fields but i'm not sure if that's necessary.

Thoughts?

[lovincyrus]

I'm not able to get the entire functionality in v0 to be 100% detailed and correct. That being said, what you have looks good with the exception that the default text on load should just include the connector: clickhouse

ah, got it!

another consideration is to always show the required fields but i'm not sure if that's necessary.

It's better to reflect the form states

[ericokuma]

okay, sounds good!

[lovincyrus]

Addressed the feedback @ericokuma

[ericokuma]

lgtm! I'm guessing the contextual help tooltips are coming later?

[lovincyrus]

lgtm! I'm guessing the contextual help tooltips are coming later?

yes! https://linear.app/rilldata/issue/APP-165/add-info-icon-to-form-fields-with-contextual-help

[jkhwu]

My 2 cents:

1. The eyeball here should be gray and more inset from the right edge.
2. It's not apparent visually which of the two tabs is currently selected, the gray or the white side? Is there a reason we couldn't match the mocks more precisely there?
3. The checkbox color should be indigo.
4. Input fields should be white to match the mocks.

[ericokuma]

@lovincyrus, i'm guessing these will go into APP-166?

[lovincyrus]

@lovincyrus, i'm guessing these will go into APP-166?

yes!

[ericokuma]

Just reviewed with @mike that we can probably remove the managed checkbox from the Clickhouse form

@begelundmuller, any objection to this?

[lovincyrus]

• It'd be nice to account for this common scenario: a self-managed ClickHouse cluster in prod, but a Rill-managed ClickHouse cluster in dev. At the bottom of the form for self-managed ClickHouse, there could be a checkbox indicating "Use a Rill-managed ClickHouse instance in development"

Ideally, we can scope this into a follow-up.

[ericokuma]

Some feedback on the ClickHouse updates:

• [x] Have self-managed ClickHouse be the first option in the dropdown vs Rill-managed
• [x] When self-managed ClickHouse is selected, display the following text before the dropdown:

• [x] Update "Deployment Type" to "Connector Type"
• [x] Placeholder text for password should be "Your ClickHouse databased password"

• [ ] Port, SSL, and Cluster should have the (optional) label only
• [ ] Missing the new Cluster form field
• [x] Update Host placeholder text to "your-instance.clickhouse.cloud or your.clickhouse.server.com"

[lovincyrus]

• Missing the new Cluster form field

We'll have to descope this. Cluster isn't added to the config properties yet, https://github.com/rilldata/rill/blob/main/runtime/drivers/clickhouse/clickhouse.go#L36

[lovincyrus]

• Port, SSL, and Cluster should have the (optional) label only

On SSL being optional. I'll delegate that to https://github.com/rilldata/rill/blob/main/runtime/drivers/clickhouse/clickhouse.go#L100

[ericokuma]

@begelundmuller, how crucial is the Cluster config? Do we need to ever display it?

[begelundmuller]

@begelundmuller, how crucial is the Cluster config? Do we need to ever display it?

Cluster is rather important, it's critical for self-managed Clickhouses that have more than one server node in the cluster.

The property is already supported on the backend (see configProperties in the clickhouse driver, @lovincyrus feel free to document it in the external ConfigProperties as well).

[lovincyrus]

@begelundmuller, how crucial is the Cluster config? Do we need to ever display it?

Cluster is rather important, it's critical for self-managed Clickhouses that have more than one server node in the cluster.

The property is already supported on the backend (see configProperties in the clickhouse driver, @lovincyrus feel free to document it in the external ConfigProperties as well).

I see, okay! I'll have this added to the external ConfigProperties and match the schema validation.

[ericokuma]

lgtm! Only feedback is that username and password shouldn't be marked as optional

[lovincyrus]

Tested self-managed and rill-managed flows ✔️

[ericpgreen2]

Tested self-managed and rill-managed flows ✔️

I see edits to the e2e tests, but did you get them to pass locally? For one, I am hitting this test failure locally:

[lovincyrus]

Tested self-managed and rill-managed flows ✔️

I see edits to the e2e tests, but did you get them to pass locally? For one, I am hitting this test failure locally:

I manually tested both parameters and connection string! Nice to see what's failing, on it!

[ericokuma]

Okay some feedback:

• We are missing tooltips for Database and SSL (although I think you had mentioned this would be covered in a separate ticket?)
• It took a while to test and connect. Do we have a button component with a loading spinner in the button avaiable in this new design system?
• On a successful connection, we should definitely display a success toast as well. The current experience, I just see the modal close and i'm not sure what happened. In addition, RD should automatically navigate to the .yaml file created

[ericokuma]

When a self-managed ClickHouse connection has been added, I can't seem to add a rill-managed clickhouse connection. The "Connect" button serves as a dead click

[lovincyrus]

• We are missing tooltips for Database and SSL (although I think you had mentioned this would be covered in a separate ticket?)

Addressed! These were previously handled in a separate pull request but since the other branches are quite outdated, I’ve cherry-picked the updates into this branch.

[ericokuma]

Hmm, i noticed that SSL is now defaulted to disabled. Let's enable by default again.

Also, i'm assuming the other feedback items have not made it to the recent branch yet yeah?

[lovincyrus]

Also, i'm assuming the other feedback items have not made it to the recent branch yet yeah?

That's right