feat(ui): <ConfigureSSO /> POC#8491
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 0e44641 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
b6b82d3 to
0e44641
Compare
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
📝 WalkthroughWalkthroughThis pull request adds a comprehensive multi-step SSO configuration wizard to the Clerk UI library. The changes introduce a new ConfigureSSO flow with seven steps (provider selection, email verification, app configuration, testing, and confirmation), complete with context-based state management, wizard routing, and localization support. The PR also includes API payload restructuring to flatten SAML/OIDC enterprise connection fields, a runtime guard preventing SSO configuration without an authenticated user, and extended theming support for the new wizard UI components. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes DetailsRationale: This is a substantial feature addition spanning 26+ files with high logic density and heterogeneous changes. The wizard implementation contains complex routing and navigation logic (554 lines), the step components feature varied patterns (email verification with OTP, URL polling for test runs, clipboard handling), the context system manages stateful flow with nested wizard delegation, and the API payload transformation requires careful verification of field mapping semantics. The changes affect multiple packages with integration points across clerk-js (guards), shared (types/warnings), localizations, and ui (components, contexts, appearance, flow metadata). While some steps follow repetitive patterns, each requires distinct reasoning regarding error handling, state synchronization, and wizard integration. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ui/src/components/ConfigureSSO/ConfigureSSO.tsx`:
- Around line 148-179: The wizard incorrectly skips the ProvideEmail step when
any user.emailAddresses exist; update ConfigureSSOWizard flow to require/select
a domain-specific email instead: replace the hasEmailAddress boolean with logic
that checks for an email matching the target/work domain (or introduce explicit
state like selectedEmail / targetEmail) and only skip rendering the ProvideEmail
step if a matching domain email is found; ensure ProvideEmail sets selectedEmail
and pass that selectedEmail (instead of reading user.emailAddresses or primary)
into VerifyDomainStep so VerifyDomainStep verifies the chosen target email for
the enterprise connection.
In `@packages/ui/src/components/ConfigureSSO/steps/ConfigureCreateAppStep.tsx`:
- Around line 42-60: The auto-create path can leave
hasAttemptedCreateRef.current true on failure causing an unrecoverable spinner;
update the effect around createEnterpriseConnection (which uses
hasAttemptedCreateRef, setIsCreating, card.setError) so that on rejection you
clear/reset hasAttemptedCreateRef.current (or set a local failed flag) and set
an error state via card.setError (or another piece of state) to surface a retry
UI; ensure the render branch that currently checks !enterpriseConnection shows
an error/retry when hasAttemptedCreateRef indicates a failed attempt (instead of
always rendering the spinner) and keep setIsCreating(false) on both success and
failure.
In `@packages/ui/src/components/ConfigureSSO/steps/TestConfigurationStep.tsx`:
- Around line 37-39: The current code launches a test via
user.createEnterpriseConnectionTestRun and then polls for any successful run,
which allows old runs to incorrectly mark the step complete and cannot detect a
specific run failure; capture the run id returned by
createEnterpriseConnectionTestRun (store it alongside setTestUrl), then change
the polling logic (the code that currently calls
getEnterpriseConnectionTestRuns) to query the specific run by id (or filter by
runId) and inspect that run's terminal status, proceeding only when that run's
status is "success" and treating "failure" as a terminal error (stop spinner and
surface an error) so that only the newly created run controls completion of
TestConfigurationStep.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 4ba31d44-1f6e-4de6-b1a7-150bfc68c86f
⛔ Files ignored due to path filters (1)
packages/ui/src/icons/duotone-at-symbol.svgis excluded by!**/*.svg
📒 Files selected for processing (25)
.changeset/lucky-tables-learn.mdpackages/clerk-js/src/core/clerk.tspackages/clerk-js/src/core/resources/User.tspackages/clerk-js/src/core/resources/__tests__/User.test.tspackages/localizations/src/en-US.tspackages/shared/src/internal/clerk-js/warnings.tspackages/shared/src/types/localization.tspackages/ui/src/components/ConfigureSSO/ConfigureSSO.tsxpackages/ui/src/components/ConfigureSSO/ConfigureSSOContext.tsxpackages/ui/src/components/ConfigureSSO/steps/ConfigureCreateAppStep.tsxpackages/ui/src/components/ConfigureSSO/steps/ConfirmationStep.tsxpackages/ui/src/components/ConfigureSSO/steps/ProvideEmailStep.tsxpackages/ui/src/components/ConfigureSSO/steps/SelectProviderStep.tsxpackages/ui/src/components/ConfigureSSO/steps/StepLayout.tsxpackages/ui/src/components/ConfigureSSO/steps/TestConfigurationStep.tsxpackages/ui/src/components/ConfigureSSO/steps/VerifyDomainStep.tsxpackages/ui/src/components/ConfigureSSO/steps/index.tspackages/ui/src/components/ConfigureSSO/wizard/ConfigureSSOWizard.tsxpackages/ui/src/components/ConfigureSSO/wizard/ConfigureSSOWizardContext.tsxpackages/ui/src/components/ConfigureSSO/wizard/index.tspackages/ui/src/components/ConfigureSSO/wizard/types.tspackages/ui/src/customizables/elementDescriptors.tspackages/ui/src/elements/contexts/index.tsxpackages/ui/src/icons/index.tspackages/ui/src/internal/appearance.ts
| const hasEmailAddress = Boolean(user?.emailAddresses?.length); | ||
|
|
||
| return ( | ||
| <ConfigureSSOWizard> | ||
| <ConfigureSSOWizard.Step | ||
| id='select-provider' | ||
| path='select-provider' | ||
| label='Select provider' | ||
| > | ||
| <SelectProviderStep /> | ||
| </ConfigureSSOWizard.Step> | ||
| <ConfigureSSOWizard.Step | ||
| id='verify-email-domain' | ||
| path='verify-email-domain' | ||
| label='Verify domain' | ||
| > | ||
| <ConfigureSSOWizard> | ||
| {!hasEmailAddress && ( | ||
| <ConfigureSSOWizard.Step | ||
| id='provide-email' | ||
| path='provide-email' | ||
| > | ||
| <ProvideEmail /> | ||
| </ConfigureSSOWizard.Step> | ||
| )} | ||
| <ConfigureSSOWizard.Step | ||
| id='verify-domain' | ||
| path='verify-domain' | ||
| > | ||
| <VerifyDomainStep /> | ||
| </ConfigureSSOWizard.Step> | ||
| </ConfigureSSOWizard> |
There was a problem hiding this comment.
Don't skip the add-email path just because the user has some email address.
hasEmailAddress only checks user.emailAddresses.length, so users who are already signed in with a personal address bypass ProvideEmail. VerifyDomainStep then reuses the current primary/unverified address, which means this flow can't collect and verify the work-domain email required for the enterprise connection. That breaks the end-to-end Configure SSO path for a common account shape; the wizard needs to track/select an email for the target domain rather than treating “any email exists” as sufficient.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/components/ConfigureSSO/ConfigureSSO.tsx` around lines 148 -
179, The wizard incorrectly skips the ProvideEmail step when any
user.emailAddresses exist; update ConfigureSSOWizard flow to require/select a
domain-specific email instead: replace the hasEmailAddress boolean with logic
that checks for an email matching the target/work domain (or introduce explicit
state like selectedEmail / targetEmail) and only skip rendering the ProvideEmail
step if a matching domain email is found; ensure ProvideEmail sets selectedEmail
and pass that selectedEmail (instead of reading user.emailAddresses or primary)
into VerifyDomainStep so VerifyDomainStep verifies the chosen target email for
the enterprise connection.
| React.useEffect(() => { | ||
| if (enterpriseConnection || !selectedProvider || !emailDomain || hasAttemptedCreateRef.current) { | ||
| return; | ||
| } | ||
|
|
||
| hasAttemptedCreateRef.current = true; | ||
| setIsCreating(true); | ||
| card.setError(undefined); | ||
|
|
||
| createEnterpriseConnection({ | ||
| provider: selectedProvider, | ||
| name: buildConnectionName(selectedProvider, emailDomain), | ||
| }) | ||
| .catch(err => handleError(err as Error, [], card.setError)) | ||
| .finally(() => setIsCreating(false)); | ||
| // `card` is intentionally omitted: `useCardState()` returns a new | ||
| // object every render, which would refire this effect indefinitely | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [enterpriseConnection, selectedProvider, emailDomain, createEnterpriseConnection]); |
There was a problem hiding this comment.
Make the auto-create failure path recoverable.
If createEnterpriseConnection() fails here, hasAttemptedCreateRef stays true while enterpriseConnection stays undefined, so the branch at Line 108 keeps rendering the spinner forever and the user has no retry path. The same dead-end happens if this step is reached without the prerequisites needed to start creation. Please render an error/retry state when creation cannot proceed, instead of treating !enterpriseConnection as "still loading".
Also applies to: 108-119
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/components/ConfigureSSO/steps/ConfigureCreateAppStep.tsx`
around lines 42 - 60, The auto-create path can leave
hasAttemptedCreateRef.current true on failure causing an unrecoverable spinner;
update the effect around createEnterpriseConnection (which uses
hasAttemptedCreateRef, setIsCreating, card.setError) so that on rejection you
clear/reset hasAttemptedCreateRef.current (or set a local failed flag) and set
an error state via card.setError (or another piece of state) to surface a retry
UI; ensure the render branch that currently checks !enterpriseConnection shows
an error/retry when hasAttemptedCreateRef indicates a failed attempt (instead of
always rendering the spinner) and keep setIsCreating(false) on both success and
failure.
| user | ||
| .createEnterpriseConnectionTestRun(enterpriseConnection.id) | ||
| .then(({ url }) => setTestUrl(url)) |
There was a problem hiding this comment.
Track the specific test run instead of polling for any prior success.
This step marks the check as complete as soon as getEnterpriseConnectionTestRuns(..., { status: ['success'] }) returns any row, so an older successful run on the same connection will enable Continue immediately even if the URL created here was never used. The inverse case is broken too: a newly failed run is indistinguishable from “still waiting”, so the user can sit on the spinner forever after a bad test. Please tie the poll to the run started at Line 38 and inspect that run’s terminal status before enabling progression.
Also applies to: 59-67
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/components/ConfigureSSO/steps/TestConfigurationStep.tsx`
around lines 37 - 39, The current code launches a test via
user.createEnterpriseConnectionTestRun and then polls for any successful run,
which allows old runs to incorrectly mark the step complete and cannot detect a
specific run failure; capture the run id returned by
createEnterpriseConnectionTestRun (store it alongside setTestUrl), then change
the polling logic (the code that currently calls
getEnterpriseConnectionTestRuns) to query the specific run by id (or filter by
runId) and inspect that run's terminal status, proceeding only when that run's
status is "success" and treating "failure" as a terminal error (stop spinner and
surface an error) so that only the newly created run controls completion of
TestConfigurationStep.
Description
<ConfigureSSO />, it contains a rough UI/UX but allows the end-user to create a connection e2eChecklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change