refactor(react): sso domain tab hook architecture separation - #346
refactor(react): sso domain tab hook architecture separation#346harishsundar-okta wants to merge 9 commits into
Conversation
harishsundar-okta
added
the
refactor
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #346 +/- ##
==========================================
- Coverage 90.89% 90.88% -0.01%
==========================================
Files 239 240 +1
Lines 17696 17736 +40
Branches 1995 2471 +476
==========================================
+ Hits 16084 16119 +35
- Misses 1612 1617 +5 ☔ 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.
9f5f983 to
d40bd8f
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:
📝 WalkthroughWalkthroughIntroduces ChangesSSO domain service extraction
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Component
participant useSsoDomainTab
participant useSsoDomainTabService
participant CoreClient
participant QueryCache
Component->>useSsoDomainTab: verify domain
useSsoDomainTab->>useSsoDomainTabService: verifyDomain(domain)
useSsoDomainTabService->>CoreClient: verify domain
CoreClient-->>useSsoDomainTabService: verification status
useSsoDomainTabService->>QueryCache: update verified domain
useSsoDomainTabService-->>useSsoDomainTab: verification result
useSsoDomainTab->>useSsoDomainTabService: associateToProvider(domain)
useSsoDomainTabService->>CoreClient: associate IdP domain
useSsoDomainTabService->>QueryCache: invalidate provider detail
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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
eb3304f to
2d6eb4c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/react/src/hooks/my-organization/__tests__/use-sso-domain-tab.test.ts (2)
80-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winName nested condition groups with
when....Rename these groups to condition descriptions such as
when initializingandwhen verifying a domain.As per coding guidelines, “name condition groups with
when....”Also applies to: 134-134, 273-273, 337-337
🤖 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/__tests__/use-sso-domain-tab.test.ts` at line 80, Rename the nested condition groups in the initialization, domain-verification, and other referenced test sections to begin with “when”, using descriptive names such as “when initializing” and “when verifying a domain”; update all four affected describe groups while preserving their test contents and behavior.Source: Coding guidelines
72-78: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRestore service-delegation coverage.
This helper fixes one input set, and the service mock discards all arguments. The suite cannot detect dropped
provider,domains,customMessages,pageSize, orfromTokenvalues. A cursor-forwarding regression would return the wrong page without failing these tests.Expose the service mock as a spy. Assert the initial call and a next-page call.
🤖 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/__tests__/use-sso-domain-tab.test.ts` around lines 72 - 78, Update renderUseSsoDomainTab and the service mock setup in the useSsoDomainTab tests to preserve and expose call arguments through a spy. Add assertions covering the initial request and a next-page request, verifying provider, domains, customMessages, pageSize, and fromToken—including cursor forwarding—so dropped values or incorrect pagination fail the suite.
🤖 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/__tests__/use-sso-domain-tab.test.ts`:
- Around line 135-149: Update the success tests around handleVerify and
handleDelete to open their respective modals before invoking the handlers,
rather than relying on the initial false state. Then retain the assertions
verifying each successful action closes the already-open modal, including the
existing verification, toast, and provider-association checks.
- Around line 14-49: Make the vi.mock factories in the use-sso-domain-tab test
capture only mock functions created through vi.hoisted, avoiding direct
references to top-level mockDomain and other ordinary bindings. Initialize the
service mock’s return values in beforeEach after mockDomain, mockVerifiedDomain,
and mockProvider are created, while preserving the existing service behavior and
error-handler mock.
---
Nitpick comments:
In
`@packages/react/src/hooks/my-organization/__tests__/use-sso-domain-tab.test.ts`:
- Line 80: Rename the nested condition groups in the initialization,
domain-verification, and other referenced test sections to begin with “when”,
using descriptive names such as “when initializing” and “when verifying a
domain”; update all four affected describe groups while preserving their test
contents and behavior.
- Around line 72-78: Update renderUseSsoDomainTab and the service mock setup in
the useSsoDomainTab tests to preserve and expose call arguments through a spy.
Add assertions covering the initial request and a next-page request, verifying
provider, domains, customMessages, pageSize, and fromToken—including cursor
forwarding—so dropped values or incorrect pagination fail the suite.
🪄 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: 2efd214f-36ca-4f55-a924-b7de69e9a0ff
📒 Files selected for processing (1)
packages/react/src/hooks/my-organization/__tests__/use-sso-domain-tab.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/react/src/hooks/my-organization/shared/services/__tests__/use-sso-domain-tab-service.test.ts (2)
288-296: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse condition-oriented test group names.
Rename these
describeblocks to start withwhen. Rename eachit('should ...')case to describe the action directly.As per coding guidelines: "Use Vitest unit tests, organize cases with
describeandit, name condition groups withwhen..., and describe the action in theitname."Also applies to: 299-314, 316-343
🤖 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 288 - 296, Rename the custom-messages test group and the additional affected describe blocks to condition-oriented names beginning with “when”. Update each associated it case to describe the tested action directly instead of using “should …”, while preserving the existing test setup and assertions in the relevant test suite.Source: Coding guidelines
358-361: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a typed provider fixture without a double assertion.
as unknown as typeof mockProviderhides fixture incompatibilities. Type a provider fixture with an optionalid, then setid: undefineddirectly.As per coding guidelines: "Avoid type assertions (
as) unless there is no other option."🤖 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 358 - 361, Update the provider fixture in the “should return domain without calling API when provider has no id” test to use a properly typed provider shape with an optional id, allowing id: undefined directly. Remove the `as unknown as typeof mockProvider` double assertion and preserve the existing renderService behavior.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.
Inline comments:
In
`@packages/react/src/hooks/my-organization/shared/services/__tests__/use-sso-domain-tab-service.test.ts`:
- Around line 288-296: Update the “should pass custom messages to translator”
test to mock a domain-list query failure, then assert that handleError is called
with “Custom error message” from customMessages.general_error. Replace the
successful domainsList assertion with the error-path expectation while
preserving the existing renderService setup.
---
Nitpick comments:
In
`@packages/react/src/hooks/my-organization/shared/services/__tests__/use-sso-domain-tab-service.test.ts`:
- Around line 288-296: Rename the custom-messages test group and the additional
affected describe blocks to condition-oriented names beginning with “when”.
Update each associated it case to describe the tested action directly instead of
using “should …”, while preserving the existing test setup and assertions in the
relevant test suite.
- Around line 358-361: Update the provider fixture in the “should return domain
without calling API when provider has no id” test to use a properly typed
provider shape with an optional id, allowing id: undefined directly. Remove the
`as unknown as typeof mockProvider` double assertion and preserve the existing
renderService behavior.
🪄 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: 37cce8a6-4b5d-4eef-a55f-e6bf78504c3f
📒 Files selected for processing (4)
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/tests/utils/__mocks__/my-organization/idp-management/sso-domain.mocks.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react/src/hooks/my-organization/tests/use-sso-domain-tab.test.ts
| describe('custom messages', () => { | ||
| it('should pass custom messages to translator', async () => { | ||
| const customMessages = { general_error: 'Custom error message' }; | ||
| const { result } = await renderService('idp-1', { | ||
| customMessages, | ||
| }); | ||
|
|
||
| expect(result.current.domainsList).toEqual([mockDomain]); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exercise the custom-message error path.
This test supplies general_error but only verifies a successful domain load. The service uses this message only after a query error. Mock a list failure and assert that handleError receives Custom error message.
🤖 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 288 - 296, Update the “should pass custom messages to translator”
test to mock a domain-list query failure, then assert that handleError is called
with “Custom error message” from customMessages.general_error. Replace the
successful domainsList assertion with the error-path expectation while
preserving the existing renderService setup.
| return data as unknown as Domain; | ||
| } | ||
|
|
||
| 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); |
There was a problem hiding this comment.
probaly good to check why we need this typecast here CreateOrganizationDomainRequestContent
There was a problem hiding this comment.
createAction is typed as ComponentAction so onAfter receives the full created Domain. The onBefore fires pre-creation when only the request content is available, so the cast bridges that. Changing the type would break onAfter consumers.
There was a problem hiding this comment.
can you check if Domain is UI defined type? if so any specific difference in CreateOrganizationDomainRequestContent ? because I think CreateOrganizationDomainRequestContent we would be deriving from SDK
36986e3 to
088be12
Compare
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