feat(ui): <ConfigureSSO /> POC#8491
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
LauraBeatris
force-pushed
the
laura/poc-e2e
branch
from
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: |
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (16)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR implements a complete multi-step SSO configuration wizard for enterprise users. It updates the backend API contract for enterprise connection payloads to use flat prefixed field names (saml_, oidc_) instead of nested structures. The UI introduces a six-step wizard: provider selection, conditional email provision, email domain verification via OTP, app configuration with IdP metadata validation, SSO testing via polling, and final confirmation with enable/disable controls. State flows through a ConfigureSSOContext that manages selected provider, enterprise connection data, and CRUD/revalidation handlers. Each step handles its own error state and uses shared helpers like handleError for consistency. Localization types and strings support the new email verification step. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![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
🤖 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.
0e44641 to
8a5cdf2
Compare
|
!snapshot |
LauraBeatris
force-pushed
the
laura/poc-e2e
branch
from
8a5cdf2 to
fc4519f
Compare
1 participant
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