feat: ClickHouse database setup SQL on the warehouse card and docs - #8112
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds customer-managed ClickHouse support for experiment warehouses, including generated database/table DDL, updated database defaults, setup documentation, and tests. Adds expandable SQL setup help with syntax highlighting and clipboard copying to the ClickHouse configuration form and connection card. Estimated code review effort: 3 (Moderate) | ~20 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 |
|
@themis-blindfold review |
⚖️ Themis judgement: 🟠 Fix before mergeA small, well-shaped UI + docs addition that copies the existing
🟠 Majors
🧹 Nits
📝 Walkthrough
🧪 How to verify
Product take: Removes a genuine onboarding dead end — before this, a BYO-ClickHouse customer had no idea what table Flagsmith expected. Modest in size but high leverage for the experimentation beta, provided the database name in the snippet and the form agree. Big picture: The events schema is now maintained in three places — the TS constant, the docs code block, and whatever component actually writes the table (not in this repo). Nothing links them, so a column added upstream silently leaves customers with a table that no longer matches. The cheapest boundary is one source for the DDL in the repo (generate or include the docs block from the constant) plus a test pinning the database/table name to 🧭 Assumptions & unverified claims
Handing customers a |
Docker builds report
|
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ea600869-bc74-4701-a542-6a60f9f2387c
📒 Files selected for processing (4)
docs/docs/experimentation/connect-a-warehouse.mdfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/WarehouseConnectionCard.tsxfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/WarehouseSetupSqlHelp.tsxfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/clickhouseSetupSql.ts
✅ private-cloud · depot-ubuntu-latest-16 — run #18853 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-arm-16 — run #18853 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
Skipped testsfirefox › tests/onboarding-tests.pw.ts › Onboarding › New user connects via the single-page onboarding flow @oss ✅ oss · depot-ubuntu-latest-arm-16 — run #18853 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-16 — run #18853 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #18847 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
✅ private-cloud · depot-ubuntu-latest-16 — run #18847 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
✅ oss · depot-ubuntu-latest-arm-16 — run #18847 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-16 — run #18847 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #18830 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
✅ private-cloud · depot-ubuntu-latest-16 — run #18830 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
✅ oss · depot-ubuntu-latest-arm-16 — run #18830 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-16 — run #18830 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
|
|
@themis-blindfold review |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
frontend/web/components/pages/environment-settings/tabs/warehouse-tab/clickhouseSetupSql.ts (2)
1-5: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winValidate or safely quote the database identifier.
The database field is only checked for non-emptiness before being interpolated into executable DDL. A crafted value can alter the copied SQL an administrator runs. Restrict it to valid ClickHouse identifiers or quote and escape it centrally; add malicious-input coverage.
29-41: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftStatic onboarding SQL breaks custom database onboarding. The helper hardcodes
flagsmith_exp, while the create form allows a different database after Step 1.
frontend/web/components/pages/environment-settings/tabs/warehouse-tab/clickhouseSetupSql.ts#L29-L41: accept the configured database and generate grants and DDL from it.frontend/web/components/pages/environment-settings/tabs/warehouse-tab/WarehouseSetup.tsx#L160-L167: provide the live database value to the onboarding SQL, rather than calling the static helper.frontend/web/components/pages/environment-settings/tabs/warehouse-tab/WarehouseSetup.tsx (1)
162-165: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the actual SQL placeholders in the instructions.
The script contains
<USER>and<CHANGE_ME_PASSWORD>, not<USERNAME>and<YOUR_SECURED_PASSWORD>.frontend/web/components/pages/environment-settings/tabs/warehouse-tab/WarehouseSqlSnippet.tsx (1)
1-30: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winBoth new SQL components bypass the required component-folder structure.
frontend/web/components/pages/environment-settings/tabs/warehouse-tab/WarehouseSqlSnippet.tsx#L1-L30: move the component into its named folder with barrel and co-located SCSS.frontend/web/components/pages/environment-settings/tabs/warehouse-tab/WarehouseSetupSqlHelp.tsx#L1-L35: move the component into its named folder with barrel and co-located SCSS.As per coding guidelines, “Each new component must live in its own folder with a barrel
index.ts, aComponentName/ComponentName.tsxfile, [and] co-locatedComponentName.scss.”Source: Coding guidelines
frontend/web/components/pages/environment-settings/tabs/warehouse-tab/WarehouseSetupSqlHelp.tsx (1)
17-17: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract the visibility override union into a named type.
Define, for example,
type VisibilityOverride = boolean | null, then use it in the state declaration.As per coding guidelines, “Extract inline union types into named types.”
Source: Coding guidelines
frontend/web/components/pages/environment-settings/tabs/warehouse-tab/warehouseFormUtils.ts (1)
14-15: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe specialised warning path lacks punctuation handling and regression coverage.
frontend/web/components/pages/environment-settings/tabs/warehouse-tab/warehouseFormUtils.ts#L14-L15: append terminal punctuation todetailbefore adding the next sentence.frontend/web/components/pages/environment-settings/tabs/warehouse-tab/__tests__/warehouseFormUtils.test.ts#L13-L24: add a missing-table detail without terminal punctuation and assert a sentence break.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1f0a3f2f-9772-477f-8c94-9d69548cdd63
📒 Files selected for processing (12)
docs/docs/experimentation/connect-a-warehouse.mdfrontend/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.scssfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/WarehouseSetup.tsxfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/WarehouseSetupSqlHelp.tsxfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/WarehouseSqlSnippet.tsxfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/__tests__/clickhouseSetupSql.test.tsfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/__tests__/warehouseFormUtils.test.tsfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/clickhouseConfig.tsfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/clickhouseSetupSql.tsfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/warehouseFormUtils.ts
⚖️ Themis review: 🧹 Ship it, nits insideThe two-step onboarding flow is a real improvement over the previous single form, and the naming is now coherent:
🧹 Nits
⚖️ Acknowledged
📝 Walkthrough
🧪 How to verify
Automate: the docs-parity test already guards the SQL copies; a render test for Product take: Self-serve setup for bring-your-own ClickHouse — the SQL is now one copy button away in the product instead of a support conversation, and the numbered steps make the ordering (provision, then connect) obvious. That is a solid capability improvement for the experimentation beta, above routine polish. The rough edge is the fixed Big picture: The canonical 🧭 Assumptions & unverified claims
A setup script that documents itself, tested against the docs that document it — turtles, but well-stacked ones. · reviewed at 8df5da2 |
talissoncosta
left a comment
There was a problem hiding this comment.
No blockers, only suggestions
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Surfaces the one-off ClickHouse database setup SQL (
eventstable DDL):total_events_receivedis currently always null for ClickHouse connections — a backend follow-up will extend event stats to ClickHouse so the collapse becomes effective.)flagsmith_exp: form default and placeholder updated (backend default to follow in the backend PR).How did you test this code?
Unit tests (including snippet interpolation pinned to the form's default database), lint, typecheck (no new errors against the baseline). Manually: checked the section renders on card and form for ClickHouse only, interpolates the Database field value, copies the SQL, and collapses/expands on click.