Skip to content

Conversation

@royendo
Copy link
Contributor

@royendo royendo commented Feb 3, 2026

as requested here:
https://rilldata.slack.com/archives/C07710F6932/p1769619124383569
Errors:
https://www.loom.com/share/2306ff33429b442d9a6d9ef618dd24d4

Screenshot 2026-02-03 at 17 30 54 Screenshot 2026-02-03 at 17 32 55

changed design after discussion with EricO and EricG

Checklist:

  • Covered by tests
  • Ran it and it works as intended
  • Reviewed the diff before requesting a review
  • Checked for unhandled edge cases
  • 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
  • I'm proud of this work!

@ericokuma
Copy link
Contributor

ericokuma commented Feb 4, 2026

UQXA:

  • If all Playground ClickHouse fields are disabled, then maybe it makes sense to not show the form and just have the appropriate parameters appear in the YAML preview? (similar to rill-managed)?
  • Probably more of a @briangregoryholmes, but these fields should be white. I think he's addressing it in this branch: feedback #8724
Screenshot 2026-02-03 at 6 54 48 PM

Copy link
Contributor

@ericokuma ericokuma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above

The core logic is already tested by unit tests in code-utils.spec.ts - specifically replaceOlapConnectorInYAML. The e2e test is just testing the UI flow that triggers this logic.

Since the unit tests already cover the YAML manipulation, and the integration point is simply the OLAP_ENGINES.includes() check in submitAddDataForm.ts:389, we can safely remove the flaky e2e test.
@royendo royendo requested a review from ericokuma February 4, 2026 21:34
Copy link
Contributor Author

@royendo royendo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final Review Summary

All Previous Issues: ✅ Resolved

Issue Resolution
TypeScript error (Writable vs FormStore) Fixed with custom FormStore type matching superforms interface
Missing unit tests for isDisabledForValues() Added 16 comprehensive tests in schema-utils.spec.ts
Unused TemplateSelector import Removed from JSONSchemaFormRenderer (component kept for future use)
Form binding issue ($form vs store) Fixed by passing formStore={form} instead of form={$form}
Nested tab fields not clearing on type change Enhanced handleSelectChange to clear x-tab-group fields

Code Quality

  • Type safety: Custom FormStore type correctly models superforms interface
  • Test coverage: Unit tests cover isDisabledForValues, isVisibleForValues, and getRequiredFieldsForValues
  • Clean code: Unused imports removed, proper component structure maintained
  • Bug fixes: Form binding and field clearing work correctly

Minor Suggestions (Non-Blocking)

  1. Consider adding JSDoc comment to isDisabledForValues() function
  2. ConnectionTypeSelector has hardcoded icon/color mappings - could be schema-driven in future

Verdict: APPROVE

Ready for merge once CI passes. The implementation successfully consolidates ClickHouse connection options into a unified schema with proper conditional rendering and form state management.

Copy link
Contributor

@ericpgreen2 ericpgreen2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1. "x-display": "connection-type" is domain-specific, not a form element type.

The other x-display values (radio, select, tabs, textarea, file) describe how a field renders as a form element. connection-type describes what the field means, which is a different axis. In the renderer, the connection-type and select branches are structurally identical — the only difference is which component gets instantiated (ConnectionTypeSelector vs Select). This could be modeled as "x-display": "select" with an additional property (e.g. "x-select-style": "rich" or "x-select-variant": "card") to control the visual treatment, keeping the display vocabulary generic and the renderer logic DRY.

2. Duplicated enum-rendering logic across GroupedFieldsRenderer and JSONSchemaFormRenderer.

isEnumWithDisplay, isRadioEnum, isTabsEnum, isSelectEnum, tabOptions, selectOptions, and radioOptions are duplicated between the two components. These are pure functions on JSONSchemaField and belong in schema-utils.ts. Any bug fix or new display type would need to be updated in two places.

3. {@html description} in InformationalField.svelte without sanitization.

This was changed to render the playground link in clickhouse.ts. Today the values only come from hardcoded schema files, so the practical risk is low. But InformationalField is a shared component — any future schema that accepts user-provided descriptions would inherit this XSS surface. Consider sanitizing with DOMPurify, or restructuring the playground description to avoid raw HTML (e.g. an "x-link" schema property that the component renders as a typed <a> tag).

4. Port default should vary by deployment type.

The default port changed from 9000 to 8443, which makes sense for ClickHouse Cloud but less so for self-managed instances where 9000 (native TCP) is common. The allOf conditionals already support default values — this is the standard JSON Schema (draft-07) mechanism for conditional defaults. The fix is two parts:

Schema: Add port: { default: "8443" } to the cloud/parameters then block and port: { default: "9000" } to the self-managed/parameters then block.

Renderer: handleSelectChange currently resets fields to their base default (the static value on the field definition), which means the conditional defaults from allOf are never consulted. After setting $form[key] = newValue, it should call getConditionalValues(schema, $form) and use those values as reset targets, falling back to the base default when no conditional default applies. This is a small, targeted change that uses the existing infrastructure.

5. Error clearing regression in AddDataForm.svelte.

The old $: if ($paramsTainted) paramsError = null cleared errors on any form edit. The new logic only clears when deployment_type changes. This means if a user gets a connection error, corrects the host, and resubmits, the stale error persists. Was this intentional?

6. Dead code: TemplateSelector.svelte, FormTemplate type, "x-templates" schema property.

These are defined but never imported or used. If the template selector feature was dropped in favor of the deployment-type dropdown approach, they should be removed from the PR.

7. ConnectionTypeSelector hardcodes ClickHouse-specific icons and colors.

The component lives in the generic features/templates/ directory but has hardcoded mappings for cloud, playground, self-managed, and rill-managed. Meanwhile, the x-enum-icons schema extension was added to types.ts but the ClickHouse schema doesn't use it — ConnectionTypeSelector bypasses that mechanism entirely. If the plan is for this component to be reusable, icons and colors should come from schema metadata or props.

8. Removed Playwright test for rill-managed ClickHouse (rill-yaml.spec.ts).

The test that verified "Should set default olap_connector to clickhouse for Rill-managed ClickHouse" was deleted. The rill-managed path still exists as a dropdown option — does this need a replacement test that exercises the new flow?

9. force: true in Playwright tests (save-anyway.spec.ts).

If the submit button is outside the viewport at 1920x1080, that suggests a layout issue. scrollIntoViewIfNeeded() before clicking would be a safer alternative than force: true, which bypasses actionability checks entirely.


Developed in collaboration with Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants