-
Notifications
You must be signed in to change notification settings - Fork 164
feat: clickhouse play template #8753
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
UQXA:
|
ericokuma
left a comment
There was a problem hiding this 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.
…ncreased height consolidate the JSONSchemaRender, decrease the form height, Salesforce doesnt need TALL
royendo
left a comment
There was a problem hiding this 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
FormStoretype correctly models superforms interface - Test coverage: Unit tests cover
isDisabledForValues,isVisibleForValues, andgetRequiredFieldsForValues - Clean code: Unused imports removed, proper component structure maintained
- Bug fixes: Form binding and field clearing work correctly
Minor Suggestions (Non-Blocking)
- Consider adding JSDoc comment to
isDisabledForValues()function ConnectionTypeSelectorhas 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.
ericpgreen2
left a comment
There was a problem hiding this 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

as requested here:
https://rilldata.slack.com/archives/C07710F6932/p1769619124383569
Errors:
https://www.loom.com/share/2306ff33429b442d9a6d9ef618dd24d4
changed design after discussion with EricO and EricG
Checklist: