Skip to content

fix: ClickHouse form follow-ups from #8028 review#8092

Merged
Zaimwa9 merged 4 commits into
mainfrom
fix/clickhouse-form-follow-ups
Jul 24, 2026
Merged

fix: ClickHouse form follow-ups from #8028 review#8092
Zaimwa9 merged 4 commits into
mainfrom
fix/clickhouse-form-follow-ups

Conversation

@Zaimwa9

@Zaimwa9 Zaimwa9 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Thanks for submitting a PR! Please check the boxes below:

  • I have read the Contributing Guide.
  • I have added information to docs/ if required so people know about the feature.
  • I have filled in the "Changes" section below.
  • I have filled in the "How did you test this code" section below.

Changes

Follow-ups to non-blocking review comments on #8028:

  • Trim whitespace from pasted values before validating and building the payload (password left intact).
  • Inline error under the port field when it's not between 1 and 65535.
  • A failed test no longer fires a danger toast on top of the warning banner.
  • autocomplete="new-password" on the password input.

How did you test this code?

Unit tests (npm run test:unit -- --testPathPatterns=warehouse-tab), lint, typecheck. Manually: pasted values with trailing whitespace, invalid port, failed test.

@vercel

vercel Bot commented Jul 24, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
flagsmith-frontend-preview Ready Ready Preview, Comment Jul 24, 2026 12:43pm
flagsmith-frontend-staging Ready Ready Preview, Comment Jul 24, 2026 12:43pm
1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Ignored Ignored Preview Jul 24, 2026 12:43pm

Request Review

@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 89bfc1c8-e92e-485d-b4f5-b715ff8d5053

📥 Commits

Reviewing files that changed from the base of the PR and between 43faf52 and ab5e2ce.

📒 Files selected for processing (1)
  • frontend/web/components/pages/environment-settings/tabs/warehouse-tab/ClickHouseConfigForm.tsx

📝 Walkthrough

Walkthrough

ClickHouse configuration handling now trims relevant fields, validates ports after trimming, normalises payload values, and ignores equivalent formatting when checking dirtiness. The form displays accessible invalid-port feedback, records connection-test failures as errored without the previous failure toast, and specifies new-password autocomplete for the password field. Tests cover whitespace handling for ports, hosts, payload fields, and passwords.

Estimated code review effort: 2 (Simple) | ~10 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the front-end Issue related to the React Front End Dashboard label Jul 24, 2026
@Zaimwa9
Zaimwa9 marked this pull request as ready for review July 24, 2026 08:45
@Zaimwa9
Zaimwa9 requested a review from a team as a code owner July 24, 2026 08:45
@Zaimwa9
Zaimwa9 requested review from kyle-ssg and removed request for a team July 24, 2026 08:45
@github-actions github-actions Bot added the fix label Jul 24, 2026
@Zaimwa9

Zaimwa9 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

@themis-blindfold review

@github-actions github-actions Bot added fix and removed fix labels Jul 24, 2026
@github-actions

github-actions Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Docker builds report

Image Build Status Security report
ghcr.io/flagsmith/flagsmith-api-test:pr-8092 Finished ✅ Skipped
ghcr.io/flagsmith/flagsmith-e2e:pr-8092 Finished ✅ Skipped
ghcr.io/flagsmith/flagsmith-api:pr-8092 Finished ✅ Results
ghcr.io/flagsmith/flagsmith:pr-8092 Finished ✅ Results
ghcr.io/flagsmith/flagsmith-private-cloud:pr-8092 Finished ✅ Results
ghcr.io/flagsmith/flagsmith-frontend:pr-8092 Finished ✅ Results

@themis-blindfold

Copy link
Copy Markdown
Contributor

⚖️ Themis judgement: ✅ Ship it

Tidy follow-up to #8028 addressing four review comments: whitespace trimming on pasted values (password intentionally left intact), inline port-range validation, removal of redundant danger toasts on test failure, and autocomplete="new-password" on the password input. The trimming is applied consistently in validation (isClickHouseFormValid, canTestClickHouseConnection), dirty-checking (isClickHouseConfigDirty), and payload construction (buildClickHousePayload). Tests cover the new behaviour well.

🎯 Correctness 5/5
🧪 Test coverage 4/5
📐 Code quality 5/5
🚀 Product impact 2/5

CI: unit tests, lint, and typecheck passed. E2E runs were still in progress; build and static-analysis checks all succeeded.

📝 Walkthrough
  • clickhouseConfig.tsisValidPort trims before validating; isClickHouseFormValid, canTestClickHouseConnection, isClickHouseConfigDirty, and buildClickHousePayload all trim string fields (except password) before comparing or emitting.
  • ClickHouseConfigForm.tsx — inline port-range error below the port field; danger toasts removed from the test-connection failure paths (the warning banner already surfaces the detail); autocomplete='new-password' added to the password <Input>.
  • clickhouseConfig.test.ts — new cases for whitespace-trimmed port validation, whitespace-only host rejection, and a full round-trip trim test in buildClickHousePayload.
🧪 How to verify
  1. Paste a host value with leading/trailing whitespace; confirm the form accepts it and the payload trims it (inspect network request on save).
  2. Type 0, 65536, or abc in the port field; confirm the inline error appears and the form cannot be submitted.
  3. Trigger a connection-test failure; confirm no danger toast appears — only the inline warning banner.
  4. Inspect the password <input> element in the browser; confirm autocomplete="new-password" is present.
  5. Run npm run test:unit -- --testPathPatterns=warehouse-tab and confirm all tests pass.

Automate: the existing unit tests cover trimming, port validation, and payload shape; no additional automated checks needed beyond E2E coverage of the warehouse tab.

Product take: Minor UX polish — prevents accidental whitespace in connection details from causing confusing failures, and stops duplicate error feedback on a failed test. Nice quality-of-life improvement but not a capability change.

🧭 Assumptions & unverified claims

No unverified assumptions or claims.

A clean diff that trims the fat (and the whitespace) — exactly what follow-ups should look like. · reviewed at 56d8b6a

@github-actions

github-actions Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor
✅ private-cloud · depot-ubuntu-latest-16 — run #18705 (attempt 1)

Playwright Test Results (private-cloud - depot-ubuntu-latest-16)

passed  3 passed

Details

stats  3 tests across 3 suites
duration  35.7 seconds
commit  ab5e2ce
info  🔄 Run: #18705 (attempt 1)

🗂️ Previous results
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #18705 (attempt 1)

Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)

passed  1 passed

Details

stats  1 test across 1 suite
duration  1 minute, 4 seconds
commit  ab5e2ce
info  🔄 Run: #18705 (attempt 1)

✅ oss · depot-ubuntu-latest-16 — run #18705 (attempt 1)

Playwright Test Results (oss - depot-ubuntu-latest-16)

passed  1 passed

Details

stats  1 test across 1 suite
duration  44.4 seconds
commit  ab5e2ce
info  🔄 Run: #18705 (attempt 1)

✅ oss · depot-ubuntu-latest-arm-16 — run #18705 (attempt 1)

Playwright Test Results (oss - depot-ubuntu-latest-arm-16)

passed  1 passed

Details

stats  1 test across 1 suite
duration  40.3 seconds
commit  ab5e2ce
info  🔄 Run: #18705 (attempt 1)

✅ private-cloud · depot-ubuntu-latest-arm-16 — run #18704 (attempt 1)

Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)

passed  2 passed

Details

stats  2 tests across 2 suites
duration  1 minute, 1 second
commit  43faf52
info  🔄 Run: #18704 (attempt 1)

✅ private-cloud · depot-ubuntu-latest-16 — run #18704 (attempt 1)

Playwright Test Results (private-cloud - depot-ubuntu-latest-16)

passed  1 passed

Details

stats  1 test across 1 suite
duration  34.3 seconds
commit  43faf52
info  🔄 Run: #18704 (attempt 1)

✅ oss · depot-ubuntu-latest-16 — run #18704 (attempt 1)

Playwright Test Results (oss - depot-ubuntu-latest-16)

passed  1 passed

Details

stats  1 test across 1 suite
duration  43.3 seconds
commit  43faf52
info  🔄 Run: #18704 (attempt 1)

✅ oss · depot-ubuntu-latest-arm-16 — run #18704 (attempt 1)

Playwright Test Results (oss - depot-ubuntu-latest-arm-16)

passed  1 passed

Details

stats  1 test across 1 suite
duration  40.5 seconds
commit  43faf52
info  🔄 Run: #18704 (attempt 1)

✅ private-cloud · depot-ubuntu-latest-16 — run #18693 (attempt 1)

Playwright Test Results (private-cloud - depot-ubuntu-latest-16)

passed  2 passed

Details

stats  2 tests across 2 suites
duration  57.3 seconds
commit  33234e1
info  🔄 Run: #18693 (attempt 1)

✅ private-cloud · depot-ubuntu-latest-arm-16 — run #18693 (attempt 1)

Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)

passed  2 passed

Details

stats  2 tests across 2 suites
duration  1 minute, 3 seconds
commit  33234e1
info  🔄 Run: #18693 (attempt 1)

✅ oss · depot-ubuntu-latest-arm-16 — run #18693 (attempt 1)

Playwright Test Results (oss - depot-ubuntu-latest-arm-16)

passed  2 passed

Details

stats  2 tests across 2 suites
duration  46 seconds
commit  33234e1
info  🔄 Run: #18693 (attempt 1)

✅ oss · depot-ubuntu-latest-16 — run #18693 (attempt 1)

Playwright Test Results (oss - depot-ubuntu-latest-16)

passed  1 passed

Details

stats  1 test across 1 suite
duration  33.2 seconds
commit  33234e1
info  🔄 Run: #18693 (attempt 1)

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 06fcfd4b-3c7e-450e-b31b-827090bacf5d

📥 Commits

Reviewing files that changed from the base of the PR and between 30bf03a and 56d8b6a.

📒 Files selected for processing (3)
  • frontend/web/components/pages/environment-settings/tabs/warehouse-tab/ClickHouseConfigForm.tsx
  • frontend/web/components/pages/environment-settings/tabs/warehouse-tab/__tests__/clickhouseConfig.test.ts
  • frontend/web/components/pages/environment-settings/tabs/warehouse-tab/clickhouseConfig.ts

@github-actions

github-actions Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Visual Regression

19 screenshots compared. See report for details.
View full report

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
frontend/web/components/pages/environment-settings/tabs/warehouse-tab/__tests__/clickhouseConfig.test.ts (1)

27-30: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Cover invalid boundaries after trimming.

This test proves that ' 9440 ' is accepted, but add cases such as ' 0 ' and ' 65536 ' to ensure trimming does not bypass the range checks.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 7fabd2ff-9263-47ab-943b-9fb7583ca9c4

📥 Commits

Reviewing files that changed from the base of the PR and between 56d8b6a and 33234e1.

📒 Files selected for processing (3)
  • frontend/web/components/pages/environment-settings/tabs/warehouse-tab/ClickHouseConfigForm.tsx
  • frontend/web/components/pages/environment-settings/tabs/warehouse-tab/__tests__/clickhouseConfig.test.ts
  • frontend/web/components/pages/environment-settings/tabs/warehouse-tab/clickhouseConfig.ts

@github-actions github-actions Bot added fix and removed fix labels Jul 24, 2026
talissoncosta
talissoncosta previously approved these changes Jul 24, 2026

@talissoncosta talissoncosta left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks great!

@Zaimwa9
Zaimwa9 merged commit 80e50a2 into main Jul 24, 2026
29 checks passed
@Zaimwa9
Zaimwa9 deleted the fix/clickhouse-form-follow-ups branch July 24, 2026 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix front-end Issue related to the React Front End Dashboard

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants