refactor(react): sso domain tab hook architecture separation#346
refactor(react): sso domain tab hook architecture separation#346harishsundar-okta wants to merge 6 commits into
Conversation
harishsundar-okta
added
the
refactor
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #346 +/- ##
==========================================
+ Coverage 88.73% 88.77% +0.03%
==========================================
Files 203 204 +1
Lines 17351 17411 +60
Branches 1816 1803 -13
==========================================
+ Hits 15397 15456 +59
- Misses 1954 1955 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
| // ===== Mock packages ===== | ||
|
|
||
| const { initMockCoreClient } = mockCore(); | ||
|
|
||
| // Test data |
There was a problem hiding this comment.
can we remove these comments?
There was a problem hiding this comment.
Removed all comments
There was a problem hiding this comment.
still we have these comments
There was a problem hiding this comment.
Removed the remaining comments
9f5f983 to
d40bd8f
Compare
|
Warning Review limit reached
Next review available in: 56 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughIntroduces a new ChangesSSO domain service extraction
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Component
participant useSsoDomainTabService
participant CoreClient
participant QueryCache
Component->>useSsoDomainTabService: verifyDomain(domain)
useSsoDomainTabService->>CoreClient: verify endpoint call
CoreClient-->>useSsoDomainTabService: status: verified
useSsoDomainTabService->>QueryCache: setQueryData(updated domain)
useSsoDomainTabService-->>Component: isVerified true
Component->>useSsoDomainTabService: associateToProvider(domain)
useSsoDomainTabService->>CoreClient: IdP domain association call
useSsoDomainTabService->>QueryCache: invalidate IdP detail query
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
♻️ Duplicate comments (1)
packages/react/src/hooks/my-organization/shared/services/use-sso-domain-tab-service.ts (1)
181-196: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPreviously "fixed"
provider.idcheck appears reverted.A prior review thread on this file flagged that
provider.idis optional and recommended checkingif (!provider?.id)to safely drop the!at Line 196. The author noted this was fixed, but the current code still showsif (!provider)(Line 183) andprovider.id!(Line 196) — the original issue persists.🛡️ Proposed fix
const deleteFromProviderMutation = useMutation({ mutationFn: async (domain: Domain) => { - if (!provider) { + if (!provider?.id) { return domain; } ... await coreClient! .getMyOrganizationApiClient() - .organization.identityProviders.domains.delete(provider.id!, domain.domain); + .organization.identityProviders.domains.delete(provider.id, domain.domain);🤖 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/react/src/hooks/my-organization/shared/services/use-sso-domain-tab-service.ts` around lines 181 - 196, The provider ID safety fix is still incomplete in deleteFromProviderMutation. Update the guard in use-sso-domain-tab-service so it checks provider?.id before proceeding, not just provider, and remove the non-null assertion when calling organization.identityProviders.domains.delete. Keep the existing onBefore flow intact, but ensure the mutation returns early whenever provider or provider.id is missing.
🧹 Nitpick comments (2)
packages/react/src/hooks/my-organization/shared/services/__tests__/use-sso-domain-tab-service.test.ts (2)
40-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant initial
mockHandleErrorassignment.Line 40's
vi.fn()assignment is immediately overwritten by the destructure at Line 72; the first assignment is dead code.Also applies to: 65-73
🤖 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/react/src/hooks/my-organization/shared/services/__tests__/use-sso-domain-tab-service.test.ts` at line 40, Remove the redundant initial assignment of mockHandleError in the use-sso-domain-tab-service test setup, since it is immediately replaced by the later destructured value. Update the test initialization around mockHandleError so there is only one source of truth, and ensure any related setup in the useSsoDomainTabService test block uses the destructured vi.fn() instance consistently.
292-304: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test coverage for mutations under null
coreClient.Only the listing query is tested under a null
coreClient(Line 293-303). Once the guard gap increate/verify/associate/deleteFromProvidermutations (flagged inuse-sso-domain-tab-service.ts) is fixed, add coverage here to lock in the expected behavior and close the Codecov-reported coverage gap for this file.🤖 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/react/src/hooks/my-organization/shared/services/__tests__/use-sso-domain-tab-service.test.ts` around lines 292 - 304, Add test coverage in useSsoDomainTabService for null coreClient across the mutation paths, not just the list query. Extend the existing edge-case suite in use-sso-domain-tab-service.test.ts to verify the create, verify, associate, and deleteFromProvider actions from useSsoDomainTabService do not call organization.domains when useCoreClient returns null and instead fail gracefully in the same way as the existing missing-client test. Focus on the mutation methods and the existing setupMockUseCoreClientNull helper so the expected guarded behavior is locked in.
🤖 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/react/src/hooks/my-organization/shared/services/use-sso-domain-tab-service.ts`:
- Around line 76-97: The mutation handlers in use-sso-domain-tab-service.ts are
inconsistent in how they handle a null coreClient, since deleteDomainMutation
already guards but createDomainMutation, verifyDomainMutation,
associateToProviderMutation, and deleteFromProviderMutation still use
coreClient! directly. Add the same null-check/failure path used by
deleteDomainMutation to each of those mutationFn blocks before calling
getMyOrganizationApiClient(), and ensure the error is surfaced gracefully
instead of causing a runtime TypeError.
- Around line 99-126: The verify flow in use-sso-domain-tab-service is passing
stale data to verifyAction.onAfter. Update the mutationFn in
verifyDomainMutation so onAfter receives the post-verification result from
updatedDomain instead of the original domain, and keep the updatedDomain value
available for any downstream consumers that need the verified status.
- Around line 76-92: `createAction.onBefore` is typed for `Domain`, but
`useSsoDomainTabService` invokes it with
`CreateOrganizationDomainRequestContent`; update the shared hook and prop types
so `SsoDomainsTabEditProps.createAction` accepts the create request payload
shape instead of `Domain`. Align the `useMutation` callback in
`use-sso-domain-tab-service` and the related `createAction` definitions to the
request-content type so the pre-submit hook only sees fields available during
creation.
---
Duplicate comments:
In
`@packages/react/src/hooks/my-organization/shared/services/use-sso-domain-tab-service.ts`:
- Around line 181-196: The provider ID safety fix is still incomplete in
deleteFromProviderMutation. Update the guard in use-sso-domain-tab-service so it
checks provider?.id before proceeding, not just provider, and remove the
non-null assertion when calling organization.identityProviders.domains.delete.
Keep the existing onBefore flow intact, but ensure the mutation returns early
whenever provider or provider.id is missing.
---
Nitpick comments:
In
`@packages/react/src/hooks/my-organization/shared/services/__tests__/use-sso-domain-tab-service.test.ts`:
- Line 40: Remove the redundant initial assignment of mockHandleError in the
use-sso-domain-tab-service test setup, since it is immediately replaced by the
later destructured value. Update the test initialization around mockHandleError
so there is only one source of truth, and ensure any related setup in the
useSsoDomainTabService test block uses the destructured vi.fn() instance
consistently.
- Around line 292-304: Add test coverage in useSsoDomainTabService for null
coreClient across the mutation paths, not just the list query. Extend the
existing edge-case suite in use-sso-domain-tab-service.test.ts to verify the
create, verify, associate, and deleteFromProvider actions from
useSsoDomainTabService do not call organization.domains when useCoreClient
returns null and instead fail gracefully in the same way as the existing
missing-client test. Focus on the mutation methods and the existing
setupMockUseCoreClientNull helper so the expected guarded behavior is locked in.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3824ee3c-5207-4e4c-9c4f-10a68140e22e
📒 Files selected for processing (5)
packages/react/src/hooks/my-organization/__tests__/use-sso-domain-tab.test.tspackages/react/src/hooks/my-organization/shared/services/__tests__/use-sso-domain-tab-service.test.tspackages/react/src/hooks/my-organization/shared/services/use-sso-domain-tab-service.tspackages/react/src/hooks/my-organization/use-sso-domain-tab.tspackages/react/src/types/my-organization/idp-management/sso-domain/sso-domain-tab-types.ts
| const createDomainMutation = useMutation({ | ||
| mutationFn: async (data: CreateOrganizationDomainRequestContent) => { | ||
| if (domains?.createAction?.onBefore) { | ||
| const canProceed = domains.createAction.onBefore(data as Domain); | ||
| if (!canProceed) { | ||
| throw new BusinessError({ message: t('domain_create.on_before') }); | ||
| } | ||
| } | ||
|
|
||
| const result: Domain = await coreClient! | ||
| .getMyOrganizationApiClient() | ||
| .organization.domains.create(data); | ||
|
|
||
| domains?.createAction?.onAfter?.(result); | ||
|
|
||
| return result; | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the definition of createAction.onBefore to check its expected parameter type
rg -n -B3 -A8 'createAction' --type=ts packages/react/src/typesRepository: auth0/auth0-ui-components
Length of output: 15911
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== ComponentAction definition ==\n'
rg -n -B3 -A12 'export interface ComponentAction|type ComponentAction' packages/react/src -g '*.ts' -g '*.tsx'
printf '\n== SSO domain tab types ==\n'
sed -n '1,120p' packages/react/src/types/my-organization/idp-management/sso-domain/sso-domain-tab-types.ts
printf '\n== use-sso-domain-tab-service ==\n'
sed -n '1,180p' packages/react/src/hooks/my-organization/shared/services/use-sso-domain-tab-service.ts
printf '\n== Search onBefore usages for domains ==\n'
rg -n 'onBefore\(' packages/react/src -g '*.ts' -g '*.tsx'Repository: auth0/auth0-ui-components
Length of output: 18603
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== ComponentAction-related type comments/usages ==\n'
rg -n -B2 -A8 'ComponentAction<' packages/react/src/types packages/react/src/hooks -g '*.ts' -g '*.tsx'
printf '\n== domain-table service create path ==\n'
sed -n '1,170p' packages/react/src/hooks/my-organization/shared/services/use-domain-table-service.ts
printf '\n== domain-table types ==\n'
sed -n '1,120p' packages/react/src/types/my-organization/domain-management/domain-table-types.tsRepository: auth0/auth0-ui-components
Length of output: 45556
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== use-domain-table-service create path ==\n'
sed -n '1,140p' packages/react/src/hooks/my-organization/shared/services/use-domain-table-service.ts
printf '\n== domain-table type definitions ==\n'
sed -n '1,100p' packages/react/src/types/my-organization/domain-management/domain-table-types.ts
printf '\n== provider create pattern for comparison ==\n'
sed -n '110,160p' packages/react/src/hooks/my-organization/shared/services/use-sso-provider-create-service.tsRepository: auth0/auth0-ui-components
Length of output: 9841
Type createAction.onBefore to accept the create payload, not Domain. useSsoDomainTabService calls it with CreateOrganizationDomainRequestContent, so the current Domain signature advertises fields that are unavailable at this point. Update SsoDomainsTabEditProps.createAction (and the shared hook types) to the request-content shape.
🤖 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/react/src/hooks/my-organization/shared/services/use-sso-domain-tab-service.ts`
around lines 76 - 92, `createAction.onBefore` is typed for `Domain`, but
`useSsoDomainTabService` invokes it with
`CreateOrganizationDomainRequestContent`; update the shared hook and prop types
so `SsoDomainsTabEditProps.createAction` accepts the create request payload
shape instead of `Domain`. Align the `useMutation` callback in
`use-sso-domain-tab-service` and the related `createAction` definitions to the
request-content type so the pre-submit hook only sees fields available during
creation.
| const createDomainMutation = useMutation({ | ||
| mutationFn: async (data: CreateOrganizationDomainRequestContent) => { | ||
| if (domains?.createAction?.onBefore) { | ||
| const canProceed = domains.createAction.onBefore(data as Domain); | ||
| if (!canProceed) { | ||
| throw new BusinessError({ message: t('domain_create.on_before') }); | ||
| } | ||
| } | ||
|
|
||
| const result: Domain = await coreClient! | ||
| .getMyOrganizationApiClient() | ||
| .organization.domains.create(data); | ||
|
|
||
| domains?.createAction?.onAfter?.(result); | ||
|
|
||
| return result; | ||
| }, | ||
| onSuccess: () => { | ||
| queryClient.invalidateQueries({ queryKey: ssoDomainQueryKeys.list(idpId) }); | ||
| queryClient.invalidateQueries({ queryKey: ssoProviderQueryKeys.detail(idpId) }); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Inconsistent coreClient null-guard across mutations.
deleteDomainMutation (Line 130) guards against a null coreClient, but createDomainMutation, verifyDomainMutation, associateToProviderMutation, and deleteFromProviderMutation all dereference coreClient! unguarded (Lines 85, 108, 164, 194). If coreClient is ever null when these fire, they'll throw a runtime TypeError instead of failing gracefully like deleteDomainMutation. This also matches the Codecov-reported coverage gap for this file — none of these guard paths are exercised by tests.
🛡️ Proposed fix (example for createDomainMutation)
const createDomainMutation = useMutation({
mutationFn: async (data: CreateOrganizationDomainRequestContent) => {
+ if (!coreClient) {
+ throw new Error('Core client is not available');
+ }
if (domains?.createAction?.onBefore) {
const canProceed = domains.createAction.onBefore(data as Domain);
if (!canProceed) {
throw new BusinessError({ message: t('domain_create.on_before') });
}
}
- const result: Domain = await coreClient!
+ const result: Domain = await coreClient
.getMyOrganizationApiClient()
.organization.domains.create(data);Also applies to: 99-126, 155-179, 181-207
🤖 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/react/src/hooks/my-organization/shared/services/use-sso-domain-tab-service.ts`
around lines 76 - 97, The mutation handlers in use-sso-domain-tab-service.ts are
inconsistent in how they handle a null coreClient, since deleteDomainMutation
already guards but createDomainMutation, verifyDomainMutation,
associateToProviderMutation, and deleteFromProviderMutation still use
coreClient! directly. Add the same null-check/failure path used by
deleteDomainMutation to each of those mutationFn blocks before calling
getMyOrganizationApiClient(), and ensure the error is surfaced gracefully
instead of causing a runtime TypeError.
| const verifyDomainMutation = useMutation({ | ||
| mutationFn: async (domain: Domain) => { | ||
| if (domains?.verifyAction?.onBefore) { | ||
| const canProceed = domains.verifyAction.onBefore(domain); | ||
| if (!canProceed) { | ||
| throw new BusinessError({ message: t('domain_verify.on_before') }); | ||
| } | ||
| } | ||
|
|
||
| const updatedDomain = await coreClient! | ||
| .getMyOrganizationApiClient() | ||
| .organization.domains.verify.create(domain.id); | ||
|
|
||
| if (domains?.verifyAction?.onAfter) { | ||
| await domains.verifyAction.onAfter(domain); | ||
| } | ||
|
|
||
| return { updatedDomain, isVerified: updatedDomain.status === 'verified' }; | ||
| }, | ||
| onSuccess: ({ updatedDomain, isVerified }, domain) => { | ||
| if (isVerified) { | ||
| queryClient.setQueryData<Domain[]>(ssoDomainQueryKeys.list(idpId), (oldDomains) => { | ||
| if (!oldDomains) return oldDomains; | ||
| return oldDomains.map((d) => (d.id === domain.id ? { ...d, ...updatedDomain } : d)); | ||
| }); | ||
| } | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
verifyAction.onAfter receives the stale pre-verification domain.
updatedDomain (Line 108) reflects the post-verification state (e.g., new status), but onAfter (Line 112-114) is invoked with the original domain argument instead. Consumers relying on onAfter to know the verification result get outdated data, even though updatedDomain is already available in scope.
🐛 Proposed fix
const updatedDomain = await coreClient!
.getMyOrganizationApiClient()
.organization.domains.verify.create(domain.id);
if (domains?.verifyAction?.onAfter) {
- await domains.verifyAction.onAfter(domain);
+ await domains.verifyAction.onAfter(updatedDomain);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const verifyDomainMutation = useMutation({ | |
| mutationFn: async (domain: Domain) => { | |
| if (domains?.verifyAction?.onBefore) { | |
| const canProceed = domains.verifyAction.onBefore(domain); | |
| if (!canProceed) { | |
| throw new BusinessError({ message: t('domain_verify.on_before') }); | |
| } | |
| } | |
| const updatedDomain = await coreClient! | |
| .getMyOrganizationApiClient() | |
| .organization.domains.verify.create(domain.id); | |
| if (domains?.verifyAction?.onAfter) { | |
| await domains.verifyAction.onAfter(domain); | |
| } | |
| return { updatedDomain, isVerified: updatedDomain.status === 'verified' }; | |
| }, | |
| onSuccess: ({ updatedDomain, isVerified }, domain) => { | |
| if (isVerified) { | |
| queryClient.setQueryData<Domain[]>(ssoDomainQueryKeys.list(idpId), (oldDomains) => { | |
| if (!oldDomains) return oldDomains; | |
| return oldDomains.map((d) => (d.id === domain.id ? { ...d, ...updatedDomain } : d)); | |
| }); | |
| } | |
| }, | |
| }); | |
| const verifyDomainMutation = useMutation({ | |
| mutationFn: async (domain: Domain) => { | |
| if (domains?.verifyAction?.onBefore) { | |
| const canProceed = domains.verifyAction.onBefore(domain); | |
| if (!canProceed) { | |
| throw new BusinessError({ message: t('domain_verify.on_before') }); | |
| } | |
| } | |
| const updatedDomain = await coreClient! | |
| .getMyOrganizationApiClient() | |
| .organization.domains.verify.create(domain.id); | |
| if (domains?.verifyAction?.onAfter) { | |
| await domains.verifyAction.onAfter(updatedDomain); | |
| } | |
| return { updatedDomain, isVerified: updatedDomain.status === 'verified' }; | |
| }, | |
| onSuccess: ({ updatedDomain, isVerified }, domain) => { | |
| if (isVerified) { | |
| queryClient.setQueryData<Domain[]>(ssoDomainQueryKeys.list(idpId), (oldDomains) => { | |
| if (!oldDomains) return oldDomains; | |
| return oldDomains.map((d) => (d.id === domain.id ? { ...d, ...updatedDomain } : d)); | |
| }); | |
| } | |
| }, | |
| }); |
🤖 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/react/src/hooks/my-organization/shared/services/use-sso-domain-tab-service.ts`
around lines 99 - 126, The verify flow in use-sso-domain-tab-service is passing
stale data to verifyAction.onAfter. Update the mutationFn in
verifyDomainMutation so onAfter receives the post-verification result from
updatedDomain instead of the original domain, and keep the updatedDomain value
available for any downstream consumers that need the verified status.
3 participants
Summary
Refactors the
useSsoDomainTabhook to follow the hook architecture pattern — two hooks internally, only one exposed publicly.Why
The SSO domain tab hook previously combined data operations (TanStack Query, API calls, cache management) with UI orchestration (modals, toasts, event handlers) in a single hook. This refactor separates concerns to match the established pattern from the SSO provider table and domain table implementations.
What
use-sso-domain-tab.tsto a new internal service hook atshared/services/use-sso-domain-tab-service.tsuse-sso-domain-tab.tsto consume the service hook internally, handling only UI state (modals, selections, toasts, error handling)ssoDomainQueryKeysto core for consistent cache key managementPackages
packages/corepackages/reactexamplesReferences
Testing
How can this be verified? Note anything intentionally not covered by tests and why.
Checklist
Contributing
Summary by CodeRabbit