OSAC-3382: add identity provider list page - #117
Conversation
|
@rawagner: This pull request references OSAC-3382 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Warning Review limit reached
Next review available in: 34 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
WalkthroughAdds identity provider listing, localized status and search UI, mock API support, and tenant role routing to ChangesIdentity provider management
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TenantUser
participant AppShell
participant IdentityProviderListPage
participant useIdentityProviders
participant IdentityProvidersAPI
TenantUser->>AppShell: open /tenant/identity-provider
AppShell->>IdentityProviderListPage: render page
IdentityProviderListPage->>useIdentityProviders: request providers
useIdentityProviders->>IdentityProvidersAPI: list providers
IdentityProvidersAPI-->>useIdentityProviders: return provider items
useIdentityProviders-->>IdentityProviderListPage: provide items
IdentityProviderListPage-->>TenantUser: render searchable provider table
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
9175458 to
09db0a3
Compare
| description={t('Manage identity providers for your tenant.')} | ||
| error={error} | ||
| actions={ | ||
| <Button variant="primary" isDisabled> |
There was a problem hiding this comment.
this is always disabled by purpose? will be implemented in another pr?
There was a problem hiding this comment.
yes, the create flow will be implemented in separate PR and that is when the button gets enabled :)
There was a problem hiding this comment.
why this page is under components/IdentityProvider/ instead of pages/tenant/ ?
There was a problem hiding this comment.
actually i would like to go away from this page/<role> pattern. There will be pages that are not role specific (or shared among X roles).
i'd much rather go with components/<resource> where you would have all list/create/other pages related to that resource.
| const resolveIdpType = (configCase: string | undefined): string => { | ||
| switch (configCase) { | ||
| case 'oidc': | ||
| return 'OIDC'; |
There was a problem hiding this comment.
we need to use t() functions here maybe?
![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 `@libs/ui-components/src/api/v1/identity-provider.ts`:
- Around line 7-12: Update libs/ui-components/src/api/v1/identity-provider.ts
(lines 7-12) in the useIdentityProviders hook to extract the active tenant
context and pass a tenant-scoped filter in the params before calling
client.list(params), ensuring IdentityProviders.List receives the
metadata.tenant constraint (unless public.v1.IdentityProviders already derives
scope from authentication context). Then update
libs/ui-components/src/test-utils/createMockConnectTransport.ts (lines 212-217)
to apply the same req.filter tenant-scope validation to the fixture results so
tests verify tenant-scope filtering is working correctly.
In
`@libs/ui-components/src/components/IdentityProvider/IdentityProviderListPage.tsx`:
- Around line 41-43: Update IdentityProviderListPage’s provider-name handling so
filtering and table rendering use the same shared display-name helper, falling
back to idp.metadata?.name consistently instead of idp.id when spec.title is
absent. Add a test fixture lacking spec.title but containing metadata.name, and
verify searching by that displayed name returns the provider.
In
`@libs/ui-components/src/components/IdentityProvider/IdentityProviderStatusLabel.tsx`:
- Around line 15-42: Replace identityProviderPhaseMap and its phaseMap lookup in
IdentityProviderStatusLabel with a switch on phase that returns the
corresponding StatusLabelProps for READY, ERROR, UNKNOWN, and UNSPECIFIED; use
the unspecified label in the default branch, including undefined or unmapped
values, and invoke translations only for the selected case.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ab66338d-cafa-4c9f-a8f8-8f9c81dd7c56
📒 Files selected for processing (14)
apps/app-frontend/src/shell/AppShell.tsxapps/app-frontend/src/shell/shellNav.test.tsapps/app-frontend/src/shell/shellNav.tsapps/app-frontend/src/shell/shellRoutes.test.tsapps/app-frontend/src/shell/shellRoutes.tslibs/i18n/locales/en/translation.jsonlibs/types/src/index.tslibs/ui-components/src/api/types.tslibs/ui-components/src/api/v1/identity-provider.test.tslibs/ui-components/src/api/v1/identity-provider.tslibs/ui-components/src/components/IdentityProvider/IdentityProviderListPage.test.tsxlibs/ui-components/src/components/IdentityProvider/IdentityProviderListPage.tsxlibs/ui-components/src/components/IdentityProvider/IdentityProviderStatusLabel.tsxlibs/ui-components/src/test-utils/createMockConnectTransport.ts
| export const useIdentityProviders = (params: ListParams = {}) => { | ||
| const client = useApiFetch(IdentityProviders); | ||
| return useApiQuery({ | ||
| queryKey: apiQueryKey('v1/identity_providers', undefined, params), | ||
| queryFn: () => client.list(params), | ||
| select: (data) => data.items, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'export type ListParams|useIdentityProviders\s*\(' libs apps
rg -n -C 4 'IdentityProviders.*list|metadata\.tenant|IdentityProvidersList' libsRepository: osac-project/osac-ui
Length of output: 17403
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Candidate files"
git ls-files | rg 'libs/ui-components/src/(api/v1/identity-provider|api/types|components/IdentityProvider/IdentityProviderListPage|test-utils/createMockConnectTransport).*\.(ts|tsx)$' || true
echo "## identity-provider.ts"
cat -n libs/ui-components/src/api/v1/identity-provider.ts
echo "## api/types relevant"
cat -n libs/ui-components/src/api/types.ts | sed -n '70,115p'
echo "## IdentityProviderListPage"
cat -n libs/ui-components/src/components/IdentityProvider/IdentityProviderListPage.tsx
echo "## createMockConnectTransport relevant"
cat -n libs/ui-components/src/test-utils/createMockConnectTransport.ts | sed -n '190,230p'
echo "## identity-provider tests relevant"
cat -n libs/ui-components/src/api/v1/identity-provider.test.ts | sed -n '1,90p'Repository: osac-project/osac-ui
Length of output: 10281
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## IdentityProviders usage across ui-components"
rg -n -C 3 'IdentityProviders|identityProviders|metadata\.tenant|useActive|activeTenant|useTenant|tenant' libs/ui-components/src --glob '*.ts' --glob '*.tsx' || true
echo "## createMockConnectTransport setup"
cat -n libs/ui-components/src/test-utils/createMockConnectTransport.ts | sed -n '1,140p'
cat -n libs/ui-components/src/test-utils/createMockConnectTransport.ts | sed -n '230,340p'Repository: osac-project/osac-ui
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## IdentityProviderListPage.test.tsx"
cat -n libs/ui-components/src/components/IdentityProvider/IdentityProviderListPage.test.tsx | sed -n '1,120p'
echo "## createMockConnectTransport header/defaults"
cat -n libs/ui-components/src/test-utils/createMockConnectTransport.ts | sed -n '1,90p'Repository: osac-project/osac-ui
Length of output: 7376
Preserve tenant scope for identity-provider lists.
IdentityProviders.List uses metadata.tenant to scope identity providers, but IdentityProviderListPage calls useIdentityProviders() with no filters, so the hook sends an unscoped list request.
libs/ui-components/src/api/v1/identity-provider.ts#L7-L12: passfilter: 'metadata.tenant == "<tenant>"'from the active tenant context before callingclient.list(params), unlesspublic.v1.IdentityProvidersalready derives scope from authentication context and always returns only the caller’s tenant.libs/ui-components/src/test-utils/createMockConnectTransport.ts#L212-L217: apply the samereq.filter/tenant scope to fixture results so tests catch tenant-scope regressions.
📍 Affects 2 files
libs/ui-components/src/api/v1/identity-provider.ts#L7-L12(this comment)libs/ui-components/src/test-utils/createMockConnectTransport.ts#L212-L217
🤖 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 `@libs/ui-components/src/api/v1/identity-provider.ts` around lines 7 - 12,
Update libs/ui-components/src/api/v1/identity-provider.ts (lines 7-12) in the
useIdentityProviders hook to extract the active tenant context and pass a
tenant-scoped filter in the params before calling client.list(params), ensuring
IdentityProviders.List receives the metadata.tenant constraint (unless
public.v1.IdentityProviders already derives scope from authentication context).
Then update libs/ui-components/src/test-utils/createMockConnectTransport.ts
(lines 212-217) to apply the same req.filter tenant-scope validation to the
fixture results so tests verify tenant-scope filtering is working correctly.
| return identityProviders.filter((idp) => { | ||
| const title = idp.spec?.title ?? idp.id; | ||
| return title.toLowerCase().includes(lowerSearch); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Filter by the displayed provider name.
The table falls back to idp.metadata?.name on Line 93, but the filter falls back directly to idp.id. A provider without spec.title is visible by metadata.name but a search for that name returns no result.
Use one shared display-name helper for filtering and rendering. Add a test fixture with no spec.title and a populated metadata.name.
🤖 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
`@libs/ui-components/src/components/IdentityProvider/IdentityProviderListPage.tsx`
around lines 41 - 43, Update IdentityProviderListPage’s provider-name handling
so filtering and table rendering use the same shared display-name helper,
falling back to idp.metadata?.name consistently instead of idp.id when
spec.title is absent. Add a test fixture lacking spec.title but containing
metadata.name, and verify searching by that displayed name returns the provider.
Wire the IdP list page using the public IdentityProviders service with status labels, type column, search filtering, and empty states. Export public IdP types from @osac/types barrel. Make TenantIdFromToken deterministic by sorting org IDs and fix TenantDeleteConfirmModal formatting. Assisted-by: Claude Code <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ElayAharoni, rawagner The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Hey, new persona should not be added at this stage (avoid complexity at init point)
please use tenant admin
liatb-rh
left a comment
liatb-rh
left a comment
There was a problem hiding this comment.
Please avoid adding new persona to ui, use tenant admin. thanks
4 participants
Wire the IdP list page using the public IdentityProviders service with status labels, type column, search filtering, and empty states. Export public IdP types from @osac/types barrel. Make TenantIdFromToken deterministic by sorting org IDs and fix TenantDeleteConfirmModal formatting.
Assisted-by: Claude Code noreply@anthropic.com
Summary by CodeRabbit
New Features
Bug Fixes