ORGS-1766: Improve self-serve SSO domain verification UX#9183
ORGS-1766: Improve self-serve SSO domain verification UX#9183NicolasLopes7 wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
📝 WalkthroughWalkthroughThe SSO organization domains flow now lets admins retry ownership verification for unverified TXT records. The UI adds loading and error handling, localization types and resources gain two keys, and a test covers the retry action. ChangesSSO domain verification retry
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/electron
@clerk/electron-passkeys
@clerk/eslint-plugin
@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: |
![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/steps/OrganizationDomainsStep.tsx`:
- Around line 645-663: Add a dedicated element descriptor for the new themable
check-now action in the OrganizationDomainsStep domain card, and pass it to the
Button via the elementDescriptor prop. Reuse the project’s established
descriptor naming and localization/context conventions, keeping the existing
handleCheckNow behavior unchanged.
- Around line 100-108: Update handleAttemptOwnershipVerification in
packages/ui/src/components/ConfigureSSO/steps/OrganizationDomainsStep.tsx#L100-L108
to inspect the resolved bulk verification result and pass any returned domain
errors through card.setError, while preserving thrown-error handling. Add
coverage in
packages/ui/src/components/ConfigureSSO/__tests__/ConfigureSSO.test.tsx#L173-L183
by mocking a resolved response containing errors and asserting the user-visible
error state.
- Around line 579-582: Update handleCheckNow and the useOrganizationDomains
polling flow to share an in-flight guard or pause polling during manual
verification, ensuring onAttemptOwnershipVerification cannot run concurrently
from the button and scheduled poll. Preserve the existing loading-state behavior
and resume polling after the manual check completes.
🪄 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 Plus
Run ID: 9e7eed89-4e06-4d35-a498-737fe6ad65f4
📒 Files selected for processing (52)
packages/localizations/src/ar-SA.tspackages/localizations/src/be-BY.tspackages/localizations/src/bg-BG.tspackages/localizations/src/bn-IN.tspackages/localizations/src/ca-ES.tspackages/localizations/src/cs-CZ.tspackages/localizations/src/da-DK.tspackages/localizations/src/de-DE.tspackages/localizations/src/el-GR.tspackages/localizations/src/en-GB.tspackages/localizations/src/en-US.tspackages/localizations/src/es-CR.tspackages/localizations/src/es-ES.tspackages/localizations/src/es-MX.tspackages/localizations/src/es-UY.tspackages/localizations/src/fa-IR.tspackages/localizations/src/fi-FI.tspackages/localizations/src/fr-FR.tspackages/localizations/src/he-IL.tspackages/localizations/src/hi-IN.tspackages/localizations/src/hr-HR.tspackages/localizations/src/hu-HU.tspackages/localizations/src/id-ID.tspackages/localizations/src/is-IS.tspackages/localizations/src/it-IT.tspackages/localizations/src/ja-JP.tspackages/localizations/src/kk-KZ.tspackages/localizations/src/ko-KR.tspackages/localizations/src/mn-MN.tspackages/localizations/src/ms-MY.tspackages/localizations/src/nb-NO.tspackages/localizations/src/nl-BE.tspackages/localizations/src/nl-NL.tspackages/localizations/src/pl-PL.tspackages/localizations/src/pt-BR.tspackages/localizations/src/pt-PT.tspackages/localizations/src/ro-RO.tspackages/localizations/src/ru-RU.tspackages/localizations/src/sk-SK.tspackages/localizations/src/sr-RS.tspackages/localizations/src/sv-SE.tspackages/localizations/src/ta-IN.tspackages/localizations/src/te-IN.tspackages/localizations/src/th-TH.tspackages/localizations/src/tr-TR.tspackages/localizations/src/uk-UA.tspackages/localizations/src/vi-VN.tspackages/localizations/src/zh-CN.tspackages/localizations/src/zh-TW.tspackages/shared/src/types/localization.tspackages/ui/src/components/ConfigureSSO/__tests__/ConfigureSSO.test.tsxpackages/ui/src/components/ConfigureSSO/steps/OrganizationDomainsStep.tsx
| const handleAttemptOwnershipVerification = async (domain: OrganizationDomainResource) => { | ||
| card.setError(undefined); | ||
|
|
||
| try { | ||
| await attemptOwnershipVerification([domain]); | ||
| } catch (err: any) { | ||
| const apiError = getFieldError(err) ?? getGlobalError(err); | ||
| card.setError(apiError); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Handle resolved per-domain verification failures.
The bulk API reports bad domains in its resolved errors array, but Line 104 ignores that result; only thrown failures reach the alert. Surface returned errors in card.error and cover that response shape.
packages/ui/src/components/ConfigureSSO/steps/OrganizationDomainsStep.tsx#L100-L108: inspect the returned bulk result and set a card error when it contains domain errors.packages/ui/src/components/ConfigureSSO/__tests__/ConfigureSSO.test.tsx#L173-L183: mock a resolved response with errors and assert the user-visible error state.
📍 Affects 2 files
packages/ui/src/components/ConfigureSSO/steps/OrganizationDomainsStep.tsx#L100-L108(this comment)packages/ui/src/components/ConfigureSSO/__tests__/ConfigureSSO.test.tsx#L173-L183
🤖 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/OrganizationDomainsStep.tsx`
around lines 100 - 108, Update handleAttemptOwnershipVerification in
packages/ui/src/components/ConfigureSSO/steps/OrganizationDomainsStep.tsx#L100-L108
to inspect the resolved bulk verification result and pass any returned domain
errors through card.setError, while preserving thrown-error handling. Add
coverage in
packages/ui/src/components/ConfigureSSO/__tests__/ConfigureSSO.test.tsx#L173-L183
by mocking a resolved response containing errors and asserting the user-visible
error state.
Source: Coding guidelines
There was a problem hiding this comment.
@NicolasLopes7 is a returned error here theoretically possible? if so I assume we want to examine the return value, extract errors, and set them to the card error
| const handleCheckNow = () => { | ||
| setIsChecking(true); | ||
| void onAttemptOwnershipVerification().finally(() => setIsChecking(false)); | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and inspect the relevant range.
wc -l packages/ui/src/components/ConfigureSSO/steps/OrganizationDomainsStep.tsx
sed -n '520,720p' packages/ui/src/components/ConfigureSSO/steps/OrganizationDomainsStep.tsx
# Find the ownership-verification hook and polling references.
rg -n "onAttemptOwnershipVerification|isChecking|10-second|setIsChecking|poll|verification" packages/ui/src/components/ConfigureSSO -S
# Inspect nearby files that define the hook behavior, if present.
fd -a "OrganizationDomainsStep.tsx|*ConfigureSSO*|*ownership*|*verification*" packages/ui/src/components/ConfigureSSO packages/ui/src -t fRepository: clerk/javascript
Length of output: 11136
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the parent logic around the handler passed to TxtRecord.
sed -n '150,240p' packages/ui/src/components/ConfigureSSO/steps/OrganizationDomainsStep.tsx
sed -n '360,540p' packages/ui/src/components/ConfigureSSO/steps/OrganizationDomainsStep.tsx
# Read the organization enterprise connection hook to understand whether it polls or refetches automatically.
sed -n '1,220p' packages/ui/src/components/ConfigureSSO/hooks/useOrganizationEnterpriseConnection.ts
# Read any helper/hook used to attempt ownership verification, if present.
rg -n "attemptOwnershipVerification|ownershipVerification|prepareOwnershipVerification|checkNow|verifyAgain" packages/ui/src/components/ConfigureSSO/hooks packages/ui/src/components/ConfigureSSO/domain -SRepository: clerk/javascript
Length of output: 20125
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the rest of useOrganizationEnterpriseConnection, especially the domain mutation wiring.
sed -n '220,320p' packages/ui/src/components/ConfigureSSO/hooks/useOrganizationEnterpriseConnection.ts
# Locate any in-repo implementation of the internal domain hook or its revalidate behavior.
rg -n "__internal_useOrganizationDomains|attemptOwnershipVerification|prepareOwnershipVerification|revalidateDomains|onOwnershipVerified" packages -S
# Inspect the type or implementation surface for the domain aggregate if present in repo.
fd -a "organizationDomains*|useOrganizationDomains*|*OrganizationDomains*" packages -t fRepository: clerk/javascript
Length of output: 11925
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the organization-domains hook implementation.
wc -l packages/shared/src/react/hooks/useOrganizationDomains.tsx
sed -n '1,260p' packages/shared/src/react/hooks/useOrganizationDomains.tsx
# Inspect the organization-domain resource methods for attempt/prep semantics.
sed -n '170,220p' packages/shared/src/types/organization.ts
sed -n '230,280p' packages/shared/src/types/organizationDomain.tsRepository: clerk/javascript
Length of output: 13368
Serialize manual checks with the polling loop
handleCheckNow only disables this button; useOrganizationDomains keeps polling attemptOwnershipVerification() every 10s, so a click can overlap with the next scheduled attempt. Share an in-flight guard, or pause polling while a manual check is running, to avoid duplicate requests.
🤖 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/OrganizationDomainsStep.tsx`
around lines 579 - 582, Update handleCheckNow and the useOrganizationDomains
polling flow to share an in-flight guard or pause polling during manual
verification, ensuring onAttemptOwnershipVerification cannot run concurrently
from the button and scheduled poll. Preserve the existing loading-state behavior
and resume polling after the manual check completes.
| <Button | ||
| variant='bordered' | ||
| colorScheme='secondary' | ||
| size='xs' | ||
| isDisabled={isChecking} | ||
| isLoading={isChecking} | ||
| onClick={handleCheckNow} | ||
| sx={t => ({ alignSelf: 'flex-start', gap: t.space.$1x5 })} | ||
| > | ||
| <Icon | ||
| icon={RotateLeftRight} | ||
| size='sm' | ||
| colorScheme='neutral' | ||
| /> | ||
| <Text | ||
| as='span' | ||
| localizationKey={localizationKeys('configureSSO.organizationDomainsStep.domainCard.txtRecord.checkNowButton')} | ||
| /> | ||
| </Button> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add an element descriptor for the new action.
This new themable button cannot be targeted through Appearance customization. Add a dedicated descriptor and pass it via elementDescriptor. As per coding guidelines, “Use element descriptors for all themable elements.”
🤖 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/OrganizationDomainsStep.tsx`
around lines 645 - 663, Add a dedicated element descriptor for the new themable
check-now action in the OrganizationDomainsStep domain card, and pass it to the
Button via the elementDescriptor prop. Reuse the project’s established
descriptor naming and localization/context conventions, keeping the existing
handleCheckNow behavior unchanged.
Source: Coding guidelines
API Changes Report
Summary
@clerk/sharedCurrent version: 4.25.5 Subpath
|
2 participants
Summary
Testing
Linear: ORGS-1766
Summary by CodeRabbit
New Features
Tests