feat(ui): <ConfigureSSO /> POC#8491
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 8a5cdf2 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: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a multi-step Configure SSO wizard and supporting infrastructure to the UI package: a declarative ConfigureSSOWizard with context/registry, seven step components (select provider, verify domain, provide email, configure create-app, map attributes, test, confirmation), a ConfigureSSO flow provider/context, new appearance keys and localization for email verification, a runtime guard preventing mounting when no user exists, and a flattened SAML/OIDC enterprise-connection request body implementation in clerk-js. Also updates FlowMetadata parts, icons exports, and tests to match the new enterprise connection payload shape. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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. ✨ Finishing Touches⚔️ Resolve merge conflicts
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.
0e44641 to
8a5cdf2
Compare
|
!snapshot |
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