feat: clickhouse warehouse setup UI#8028
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughAdds ClickHouse warehouse request and response types, form validation, payload construction, and tests. Introduces feature-flagged ClickHouse selection, creation, editing, and connection testing in warehouse settings. Updates warehouse cards to display status details, host information, and conditional test actions. Moves shared save-button label logic into a utility and restricts polling to Flagsmith connections. Estimated code review effort: 3 (Moderate) | ~25 minutes Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
front-end
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/clickhouse-byo-warehouse-backend #8028 +/- ##
=========================================================================
+ Coverage 98.65% 98.67% +0.02%
=========================================================================
Files 1507 1514 +7
Lines 59866 60930 +1064
=========================================================================
+ Hits 59060 60124 +1064
Misses 806 806 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Zaimwa9
changed the title
Zaimwa9
changed the title
github-actions
Bot
added
feature
Docker builds report
|
|
@themis-blindfold review |
⚖️ Themis judgement: ✅ Ship itAdds the ClickHouse warehouse connection type to the frontend, gated behind the The form validation logic and payload builder are extracted into a standalone module with thorough unit tests covering port range validation, required-field checks, edit-mode password handling, and credential separation. The polling guard correctly limits automatic test-connection calls to Flagsmith-type connections while ClickHouse uses manual "Test connection" instead.
📝 Walkthrough
🧪 How to verify
Automate: the unit tests already cover the form logic; a Cypress/E2E test for the full create-edit-test-delete flow would close the gap. Product take: Solid feature addition that puts ClickHouse on equal footing with the Flagsmith warehouse type for BYO-warehouse users. Properly gated behind a flag so it ships dark — low-risk incremental rollout. 🧭 Assumptions & unverified claims
A warehouse that connects on the first try — now that's a feature worth flagging. · reviewed at af485e8 |
af485e8 to
19b31a1
Compare
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
|
Visual Regression19 screenshots compared. See report for details. |
github-actions
Bot
added
feature
|
@themis-blindfold review once again but this time focus on the frontend design patterns to make sure the used components follow the snowflake designs. Let's flag whether some scss or classes are duplicated, or could be better implemented. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frontend/common/types/requests.ts (1)
1031-1046: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract the repeated
configvalue union into a named type.
string | number | booleanis duplicated verbatim in bothcreateWarehouseConnection.configandupdateWarehouseConnection.config. Extract to a shared type, e.g.type WarehouseConfigValue = string | number | boolean, and reuse it in both places.As per coding guidelines, "Extract inline union types into named types (for example, use
type Status = 'A' | 'B'instead of an inline union)."♻️ Proposed fix
+type WarehouseConfigValue = string | number | boolean + createWarehouseConnection: { environmentId: string warehouse_type: string name?: string - config?: Record<string, string | number | boolean> + config?: Record<string, WarehouseConfigValue> credentials?: { password: string } } deleteWarehouseConnection: { environmentId: string; id: number } testWarehouseConnection: { environmentId: string; id: number } updateWarehouseConnection: { environmentId: string id: number name?: string - config?: Record<string, string | number | boolean> + config?: Record<string, WarehouseConfigValue> credentials?: { password: string } }Source: Coding guidelines
frontend/web/components/pages/environment-settings/tabs/warehouse-tab/WarehouseConnectionCard.tsx (2)
92-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated per-type identifier rendering — consider a single lookup.
The
account_identifier(Snowflake) andhost(ClickHouse) blocks are structurally identical, just keyed on different config fields. As more warehouse types are added (BigQuery/Databricks are already teased inWarehouseSetup), this will keep growing. Consider deriving a single "identifier" value fromconnection.warehouse_type/connection.configonce, and rendering it a single time.♻️ Illustrative refactor
-{connection.config && - 'account_identifier' in connection.config && - connection.config.account_identifier && - `: ${connection.config.account_identifier}`} -{connection.config && - 'host' in connection.config && - connection.config.host && - `: ${connection.config.host}`} +{getConnectionIdentifier(connection.config) && + `: ${getConnectionIdentifier(connection.config)}`}
147-157: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPermanently-disabled "Test connection" button for non-ClickHouse connections — see consolidated comment.
onTestConnectionisundefinedfor any connection that isn'tclickhouse(perindex.tsx), yet this button still renders for any!isFlagsmithconnection, including Snowflake. The result is a disabled button with no explanation for existing Snowflake connections.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 730e0b0c-7865-4e6d-b738-7dea9f22117f
📒 Files selected for processing (8)
frontend/common/types/requests.tsfrontend/common/types/responses.tsfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/ClickHouseConfigForm.tsxfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/WarehouseConnectionCard.tsxfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/WarehouseSetup.tsxfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/__tests__/clickhouseConfig.test.tsfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/clickhouseConfig.tsfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/index.tsx
| import React, { FC, useState } from 'react' | ||
| import Button from 'components/base/forms/Button' | ||
| import Input from 'components/base/forms/Input' | ||
| import Switch from 'components/Switch' | ||
| import ErrorMessage from 'components/ErrorMessage' | ||
| import { ClickHouseConfig } from 'common/types/responses' | ||
| import { | ||
| buildClickHousePayload, | ||
| CLICKHOUSE_DEFAULTS, | ||
| ClickHouseFormData, | ||
| ClickHouseFormState, | ||
| isClickHouseFormValid, | ||
| } from './clickhouseConfig' | ||
| import './ConfigForm.scss' |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Move into its own component folder and stop importing ConfigForm.scss directly.
This new component lives directly under warehouse-tab/ and imports ./ConfigForm.scss (Snowflake's stylesheet) rather than having its own co-located stylesheet. That silently couples ClickHouse styling to the Snowflake form file.
As per path instructions, "Each new component must live in its own folder with a barrel index.ts, a ComponentName/ComponentName.tsx file, co-located ComponentName.scss, any subcomponents, and imports should use the component folder path rather than the inner file."
♻️ Proposed structure
ClickHouseConfigForm/
index.ts // export { default } from './ClickHouseConfigForm'
ClickHouseConfigForm.tsx
ClickHouseConfigForm.scss // own styles, or explicitly import the shared partial
Source: Path instructions
|
Review failed after 2 attempt(s) (OutputError). Check the worker logs. |
Zaimwa9
left a comment
Zaimwa9
left a comment
There was a problem hiding this comment.
Verdict: 🧹 Ship it, nits inside — no correctness issues, two nits filed inline:
getButtonLabelduplication — identical helper copied from the Snowflake form; should be shared to avoid a third copy when the next warehouse type arrives.- Missing
idattributes on form buttons — the Snowflake form has them for E2E selectors; the ClickHouse form omits them.
The PR is well-structured overall: clean type separation for credentials vs config, solid unit tests on the pure logic, correct polling scoping, and consistent use of the shared SCSS and BEM conventions.
Recovered Themis review, posted manually (bot post failed on an output-format validation error).
github-actions
Bot
added
feature
c9cb041 to
4b54dc7
Compare
- Only render Test connection button when the handler is provided (backend only supports ClickHouse, not Snowflake) - Add missing id attributes on ClickHouse form buttons for E2E - Extract shared getButtonLabel helper from both config forms - Extract WarehouseConfigValue named type in requests.ts - Extract WarehouseConfigResponse named type in responses.ts
7b7a695 to
aa07e2f
Compare
github-actions
Bot
added
feature
✅ private-cloud · depot-ubuntu-latest-16 — run #18534 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-arm-16 — run #18534 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #18534 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #18534 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
|
1 participant
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Frontend for the
clickhousewarehouse connection type, gated behind theclickhouse_warehouseflag (off by default). Builds on the backend in #8020.status_detailfield: the status tooltip shows the failure reason when the connection is errored, and the test-connection toast reports it instead of a generic message.credentials, read-onlystatus_detail.How did you test this code?
Unit tests for the form validation and payload helpers (
npm run test:unit -- --testPathPatterns=clickhouseConfig),npm run lint,npm run typecheck(no new errors against the baseline).Manually: enable the
clickhouse_warehouseflag, go to Environment Settings → Warehouse, connect a ClickHouse instance, then edit it and use Test connection to see verification results, including failure reasons.