Skip to content

fix(platform): seed enterprise sso form select/switch with defined defaults (#2095)#2099

Open
larryro wants to merge 1 commit into
mainfrom
tale/x57c1t3gnx58pvy8my5mt1qyt5898gma
Open

fix(platform): seed enterprise sso form select/switch with defined defaults (#2095)#2099
larryro wants to merge 1 commit into
mainfrom
tale/x57c1t3gnx58pvy8my5mt1qyt5898gma

Conversation

@larryro

@larryro larryro commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes #2095 — the Enterprise SSO settings form logged five React/Radix uncontrolled→controlled warnings on load.

While the parent config query is loading, the seeded data is undefined, so each Controller's field.value is undefined. The Radix Select and Switch controls therefore mounted uncontrolled, then flipped to controlled once the config hydrated — Radix's useControllableState emits a console.warn on that transition (2 Selects + 3 Switches = 5 warnings).

Fix

Provide a defined fallback at each render point so the controls are controlled from the first render:

  • Selects (protocol, defaultRole): value={field.value ?? ''}
  • Switches (pkce, autoRole, autoTeam): checked={field.value ?? false}

editor.isLoading (which drives the disabled/loading state) still derives from data === undefined, so the loading behaviour is unchanged — only the per-field control values gain a defined default.

Test

Added a regression test that mounts the form with config={undefined}, resolves it, and asserts no uncontrolled→controlled warning is logged (spying on console.warn, which is where Radix reports it). Verified it fails without the fix and passes with it.

Verification

  • vitest --project client (form test file): 9 passed
  • oxlint --type-aware: 0 warnings, 0 errors
  • tsc --noEmit: clean

@coderabbitai

coderabbitai Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@larryro, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 33 minutes and 5 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 27762a5d-4498-4c3d-aab2-469b9a3d4bbc

📥 Commits

Reviewing files that changed from the base of the PR and between 7b961c4 and bea97d4.

📒 Files selected for processing (2)
  • services/platform/app/features/settings/enterprise-sso/components/enterprise-sso-form.test.tsx
  • services/platform/app/features/settings/enterprise-sso/components/enterprise-sso-form.tsx
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch tale/x57c1t3gnx58pvy8my5mt1qyt5898gma

Warning

Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption.


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.

@larryro

larryro commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator Author

Desk Review — READY TO MERGE ✅

Reviewed the full implementation on branch tale/x57c1t3gnx58pvy8my5mt1qyt5898gma against issue #2095 (uncontrolled→controlled warnings on the Enterprise SSO form).

CI

All checks green for head bea97d497d89ccb92dfef4650214db9be7a6b1cf — no failing, no pending (Unit, UI, Type check, Lint, Format, all 16 Playwright shards, Storybook, security scans). gh pr checks confirmed.

Tests

bunx vitest --run --project client …/enterprise-sso-form.test.tsx9 passed. I additionally verified the new regression test is a genuine guard, not a tautology: reverting the 5 fallbacks locally makes it fail (warned=true), and restoring them makes it pass. Confirmed Radix useControllableState emits the warning via console.warn with "changing from uncontrolled to controlled", which the test's spy + /uncontrolled.*controlled/i regex correctly target.

Correctness & completeness

  • All 5 reported warnings are addressed: protocol Select + defaultRole Select (value={field.value ?? ''}), and pkce / autoRole / autoTeam Switches (checked={field.value ?? false}). No other controlled Radix control in the form is left unfixed; the Inputs/Textarea use register() (uncontrolled, no warning).
  • The ''/false placeholders apply only during the loading window (data undefined). Once config resolves, every field hydrates to a defined, valid value, so no saved value changes and the '' fallback can never be submitted — the onValueChange narrowStringUnion guards plus the zod enum on protocol/defaultRole both reject empty string.
  • No behavioral change across OIDC / oauth2 / generic-oidc / SAML / unconfigured branches; pure render-time defaults, no lifecycle/resource impact.

Elegance

The field.value ?? '' / ?? false pattern matches the repo's existing convention (e.g. organization-settings.tsx). Comments are clear; minor near-duplication is acceptable.

Non-blocking observation (pre-existing, out of scope)

roleOptions omits 'disabled' while the zod enum + data seeding allow defaultRole: 'disabled', so a stored 'disabled' role would have no matching Select option. This predates this PR and is unrelated to #2095 — not a blocker here; worth a separate follow-up.

Verdict: READY TO MERGE.

@larryro

larryro commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator Author

Desk Review — #2095 (uncontrolled→controlled warnings on Enterprise SSO form)

Verdict: READY TO MERGE.

Reviewed branch tale/x57c1t3gnx58pvy8my5mt1qyt5898gma (commit bea97d497) in two rounds (5 parallel review subagents across correctness/robustness/elegance/tests/boundaries, then independent confirmation).

CI

All required checks green (gh pr checks exit 0): Build platform, Unit, UI, Storybook, Type check, Lint, Format, Knip, all 16 Playwright (platform) shards, Playwright (web/docs), Smoke test, Opengrep, Trivy/Scan. Only skipping entries are fork-PR / web-container jobs (expected, not failures).

Tests

Ran the project's own UI suite for the changed file:
bunx vitest --run --config vitest.ui.config.ts app/features/settings/enterprise-sso/components/enterprise-sso-form.test.tsx9 passed (9).

The new regression test (does not log uncontrolled→controlled warnings when config resolves after load) was independently validated as meaningful:

  • Reverting all 5 fallbacks → test fails (expected true to be false).
  • Reverting only the autoTeam Switch fallback (line 794) → test still fails. This confirms the global console.warn spy covers every one of the 5 controls in the single config: undefined → connectedOidc transition, so pkce/autoRole/autoTeam are all genuinely exercised even without per-control assertions.

Findings

  • Correctness — pass. The diff seeds exactly the 5 reported controls: 2 <Select value={field.value ?? ''}> (protocol :565, defaultRole :760) and 3 <Switch checked={field.value ?? false}> (pkce :678, autoRole :781, autoTeam :794). '' is never a valid enum option for either Select, and the onValueChange handlers run narrowStringUnion(...) which drops '', so the placeholder can't be committed. The transient pkce=false (real seeded default is true) is cosmetic only — the <fieldset disabled={!canEdit || editor.isLoading}> (line 541) keeps the form non-interactive while data is undefined, and form.reset(data) re-seeds real values on hydration before the user can act.
  • Completeness — pass. Enumerated every controlled input in the file; only these 5 Select/Switch controls take field.value while loading. The other value= sites (709/714/728 read-only copy fields already ?? ''; 873 guarded by config?.scim.baseUrl &&) are pre-existing display fields, not in scope. No 6th unfixed control.
  • Robustness — pass. No new re-render churn, lifecycle, or ordering hazard; the disabled-during-load gate prevents any bad-state window.
  • Boundaries/security — pass. Zod superRefine validation is untouched; ''/false can't bypass enum or required-field checks, and nothing persists a transient default (form disabled until config resolves). The unrelated protocol-switch validation gap is not regressed.
  • Elegance — pass. Inline ?? '' / ?? false matches the repo's established convention (e.g. organization-settings.tsx, branding-form.tsx, password-policy-editor.tsx). Minimal, focused, accurate comments.

The fix fully resolves the reported issue (all 5 warnings eliminated) with no blocking issues found.

READY TO MERGE.

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.

Bug: Enterprise SSO settings form logs React uncontrolled→controlled warnings (Select + Switch initialized undefined)

1 participant