feat(ui): replace the SSO setup confirmation step with an Activate step#8882
feat(ui): replace the SSO setup confirmation step with an Activate step#8882iagodahlem wants to merge 7 commits into
Conversation
… step The SSO wizard now ends on an explicit Activate step after Test: a warning that activating forces SSO sign-in for the verified domain, an Activate SSO action, and a Skip for now exit — both return to the Security overview. Wizard step labels are renamed to Domains, Connection, and Activate.
🦋 Changeset detectedLatest commit: b705adc The changes in this PR will be included in the next version bump. This PR includes changesets to release 21 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe Configure SSO wizard's final step is changed from a static ChangesSSO Wizard: Replace ConfirmationStep with ActivateStep
Sequence Diagram(s)sequenceDiagram
participant User
participant ConfigureSSOWizard
participant ActivateStep
participant CardState
participant ClerkAPI
User->>ConfigureSSOWizard: completes test step (hasSuccessfulTestRun=true)
ConfigureSSOWizard->>ActivateStep: render activate step
ActivateStep-->>User: show title, domain subtitle, Activate/Skip buttons
alt User clicks "Activate SSO"
User->>ActivateStep: click Activate SSO
ActivateStep->>CardState: setLoading(true)
ActivateStep->>ClerkAPI: setConnectionActive(id, true)
ClerkAPI-->>ActivateStep: success
ActivateStep->>ConfigureSSOWizard: onExit()
else User clicks "Skip for now"
User->>ActivateStep: click Skip for now
ActivateStep->>ConfigureSSOWizard: onExit()
else Activation fails
ClerkAPI-->>ActivateStep: ClerkRuntimeError
ActivateStep->>CardState: handleError(err)
ActivateStep-->>User: show error alert
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@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: |
iagodahlem
force-pushed
the
iago/orgs-1629-activate-step
branch
from
1652bcb to
877089e
Compare
Replace the `size='lg'` prop on the Activate step shield-check icon with an `sx` width and height set to `t.sizes.$8`, matching the design specification.
API Changes Report
Summary
@clerk/sharedCurrent version: 4.18.0 Subpath
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/ui/src/components/ConfigureSSO/steps/index.ts (1)
1-1: ⚡ Quick winAvoid expanding the
steps/index.tsbarrel.This adds another re-export to an
index.tsbarrel; prefer direct imports (e.g.,./ActivateStep) in callers to keep dependency edges explicit and reduce circular-dependency risk.As per coding guidelines,
**/index.ts: “Avoid barrel files (index.ts re-exports) as they can cause circular dependencies.”🤖 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/index.ts` at line 1, The export statement in steps/index.ts is a barrel re-export that violates coding guidelines and increases circular-dependency risk. Remove the `export { ActivateStep } from './ActivateStep'` statement from the index.ts file and update all callers to use direct imports instead, importing ActivateStep directly from `./ActivateStep` rather than from the barrel file.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@packages/ui/src/components/ConfigureSSO/steps/index.ts`:
- Line 1: The export statement in steps/index.ts is a barrel re-export that
violates coding guidelines and increases circular-dependency risk. Remove the
`export { ActivateStep } from './ActivateStep'` statement from the index.ts file
and update all callers to use direct imports instead, importing ActivateStep
directly from `./ActivateStep` rather than from the barrel file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 4f1edacc-05b0-4cb1-9b61-1eeb1517222c
⛔ Files ignored due to path filters (1)
packages/ui/src/icons/duotone-shield-check.svgis excluded by!**/*.svg
📒 Files selected for processing (16)
.changeset/moody-carrots-make.mdpackages/localizations/src/en-US.tspackages/shared/src/types/localization.tspackages/ui/src/components/ConfigureSSO/ConfigureSSOWizard.tsxpackages/ui/src/components/ConfigureSSO/__tests__/ConfigureSSO.navigation.test.tsxpackages/ui/src/components/ConfigureSSO/__tests__/ConfigureSSO.test.tsxpackages/ui/src/components/ConfigureSSO/elements/Step.tsxpackages/ui/src/components/ConfigureSSO/steps/ActivateStep.tsxpackages/ui/src/components/ConfigureSSO/steps/ConfirmationStep.tsxpackages/ui/src/components/ConfigureSSO/steps/__tests__/ActivateStep.test.tsxpackages/ui/src/components/ConfigureSSO/steps/index.tspackages/ui/src/components/ConfigureSSO/types.tspackages/ui/src/customizables/elementDescriptors.tspackages/ui/src/elements/contexts/index.tsxpackages/ui/src/icons/index.tspackages/ui/src/internal/appearance.ts
💤 Files with no reviewable changes (1)
- packages/ui/src/components/ConfigureSSO/steps/ConfirmationStep.tsx
| > | ||
| <Col | ||
| align='center' | ||
| sx={t => ({ textAlign: 'center', maxWidth: '332px', gap: t.space.$3x5 })} |
There was a problem hiding this comment.
I don't see references on the codebase of relying on px as unit for maxWidth, perhaps we should rely on sizes from the theme variable or REM
LauraBeatris
left a comment
LauraBeatris
left a comment
There was a problem hiding this comment.
We just need to follow up with product design to get the UI prototype updated for when the user lands on this screen with an already enabled connection
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui/src/components/ConfigureSSO/steps/ActivateStep.tsx (1)
33-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate activation result before exiting the flow.
At Line 33,
setConnectionActivecan resolve toundefinedper its contract, but Line 34 still callsonExit?.(). That can navigate away without a confirmed activation.Suggested fix
- await setConnectionActive(enterpriseConnection.id, true); - onExit?.(); + const updatedConnection = await setConnectionActive(enterpriseConnection.id, true); + if (!updatedConnection) { + throw new Error('Failed to activate SSO connection.'); + } + onExit?.();🤖 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/ActivateStep.tsx` around lines 33 - 35, The code calls onExit?.() on line 34 without validating that setConnectionActive actually succeeded. Since setConnectionActive can resolve to undefined per its contract, the flow may navigate away without confirming activation. Capture the return value of the setConnectionActive call for enterpriseConnection.id and only invoke onExit?.() if the result confirms successful activation, preventing navigation when activation fails or returns undefined.
🧹 Nitpick comments (1)
packages/ui/src/components/ConfigureSSO/steps/__tests__/ActivateStep.test.tsx (1)
81-91: ⚡ Quick winAdd a regression test for nullable activation results.
Please add a case where
setConnectionActiveresolvesundefinedand assertonExitis not called. This protects the nullable mutation contract edge case.🤖 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/__tests__/ActivateStep.test.tsx` around lines 81 - 91, Add a new test case in the ActivateStep.test.tsx file that tests the scenario where setConnectionActive resolves to undefined instead of a valid response object. In this new test, mock setConnectionActive to resolve with undefined, render the step, click the "Activate SSO" button, and then assert that onExit is not called (verify it was called zero times). This ensures the component properly handles the nullable return value from setConnectionActive and does not incorrectly exit when the activation result is undefined.
🤖 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.
Outside diff comments:
In `@packages/ui/src/components/ConfigureSSO/steps/ActivateStep.tsx`:
- Around line 33-35: The code calls onExit?.() on line 34 without validating
that setConnectionActive actually succeeded. Since setConnectionActive can
resolve to undefined per its contract, the flow may navigate away without
confirming activation. Capture the return value of the setConnectionActive call
for enterpriseConnection.id and only invoke onExit?.() if the result confirms
successful activation, preventing navigation when activation fails or returns
undefined.
---
Nitpick comments:
In
`@packages/ui/src/components/ConfigureSSO/steps/__tests__/ActivateStep.test.tsx`:
- Around line 81-91: Add a new test case in the ActivateStep.test.tsx file that
tests the scenario where setConnectionActive resolves to undefined instead of a
valid response object. In this new test, mock setConnectionActive to resolve
with undefined, render the step, click the "Activate SSO" button, and then
assert that onExit is not called (verify it was called zero times). This ensures
the component properly handles the nullable return value from
setConnectionActive and does not incorrectly exit when the activation result is
undefined.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: ab01f162-2729-48f3-bfaf-23c9aa75c543
📒 Files selected for processing (6)
packages/localizations/src/en-US.tspackages/shared/src/types/localization.tspackages/ui/src/components/ConfigureSSO/__tests__/ConfigureSSO.navigation.test.tsxpackages/ui/src/components/ConfigureSSO/__tests__/ConfigureSSO.test.tsxpackages/ui/src/components/ConfigureSSO/steps/ActivateStep.tsxpackages/ui/src/components/ConfigureSSO/steps/__tests__/ActivateStep.test.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/localizations/src/en-US.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/shared/src/types/localization.ts
2 participants
Summary
Replaces the final step of the self-serve SSO setup wizard — previously a static "Confirmation" summary — with an explicit Activate step, so configuring a connection and turning it on become distinct, intentional actions.
After the Test step, the user lands on Activate:
Also renames the wizard step labels to Domains → Connection → Test → Activate.
Follows #8864 (Security ↔ wizard navigation).
Test plan
ConfigureSSO+OrganizationSecurityPageunit suites passSummary by CodeRabbit