OSAC-3385: Add role bindings management UI with list, create, edit, a… - #132
Merged
Merged
Conversation
|
@rawagner: This pull request references OSAC-3385 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds tenant role binding management with list, create, edit, and delete flows. It adds API hooks, form validation, role and user resolution, status handling, navigation, routes, localization, and mock transport support. ChangesRole Binding Management
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TenantAdmin
participant RoleBindingsPage
participant RoleBindingCreatePage
participant roleBindingHooks
participant RoleBindings
TenantAdmin->>RoleBindingsPage: open role bindings
RoleBindingsPage->>roleBindingHooks: fetch bindings and roles
roleBindingHooks->>RoleBindings: list requests
RoleBindings-->>roleBindingHooks: resource collections
roleBindingHooks-->>RoleBindingsPage: resolved table data
TenantAdmin->>RoleBindingCreatePage: create or edit binding
RoleBindingCreatePage->>roleBindingHooks: submit binding spec
roleBindingHooks->>RoleBindings: create or update request
RoleBindings-->>roleBindingHooks: role binding response
roleBindingHooks-->>RoleBindingCreatePage: invalidate role binding queries
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 9
🧹 Nitpick comments (5)
libs/ui-components/src/components/RoleBinding/RoleBindingDeleteModal.tsx (1)
44-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winName the role binding in the confirmation text.
The dialog is destructive, but it does not state which binding it deletes. If the user opens the menu on the wrong row, nothing in the dialog reveals the mistake. Include
roleBinding.metadata?.name || roleBinding.idin the message, and use an interpolated translation key so translators keep the placeholder.🤖 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/RoleBinding/RoleBindingDeleteModal.tsx` around lines 44 - 50, Update the confirmation text in RoleBindingDeleteModal to include the binding name using roleBinding.metadata?.name || roleBinding.id. Replace the static translation call with an interpolated translation key and pass the binding identifier as a named placeholder so translators retain it in the localized message.libs/ui-components/src/components/RoleBinding/RoleBindingsPage.test.tsx (1)
153-162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the popover and the delete flow.
Two paths that this PR adds are untested:
MultipleUsersPopovernever renders. No test clicks the "Multiple users" link. That component resolves users throughthis.id, while the table resolves throughthis.metadata.name. A test that opens the popover and asserts thataliceandbobappear would catch the mismatch described inRoleBindingsPage.tsxLines 19-29.RoleBindingDeleteModalnever opens. Add a test that opens the actions menu, selects "Delete", confirms, and asserts that the delete RPC ran.Both tests need the
Usersmock inlibs/ui-components/src/test-utils/createMockConnectTransport.tsto honorreq.filter. Do you want me to draft these tests?🤖 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/RoleBinding/RoleBindingsPage.test.tsx` around lines 153 - 162, Extend the RoleBindingsPage tests with coverage for the MultipleUsersPopover and RoleBindingDeleteModal flows: click “Multiple users” and assert alice and bob render, then open a row’s Actions menu, select Delete, confirm, and assert the delete RPC was called. Update the Users mock in createMockConnectTransport to filter results according to req.filter so both tests exercise the intended lookup behavior.libs/ui-components/src/test-utils/createMockConnectTransport.ts (2)
423-436: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe generated create ID can be overwritten, and
updateis missing.Two points:
...req.objectspreads afterid. Protobuf message objects always carry anidfield (empty string when unset), so the mock returnsid: ''instead of'new-rb-1'. Put the fallback after the spread. The same pattern exists at Lines 291 and 408, so this is consistent with current code, but new tests that assert on the created ID will fail.- The service registers
list,get,create, anddelete, but notupdate.useUpdateRoleBindingpowers the edit flow inRoleBindingCreatePage.tsx. Tests for the edit path will hit an unimplemented method.♻️ Proposed change
router.service(RoleBindings, { list: () => ({ items: roleBindingsFixtures, size: roleBindingsFixtures.length, total: roleBindingsFixtures.length, }), get: (req) => ({ object: roleBindingsFixtures.find((rb) => rb.id === req.id), }), create: (req) => ({ - object: { id: 'new-rb-1', ...req.object }, + object: { ...req.object, id: req.object?.id || 'new-rb-1' }, }), + update: (req) => ({ object: req.object }), delete: () => ({}), });🤖 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/test-utils/createMockConnectTransport.ts` around lines 423 - 436, Update the RoleBindings service registration to spread req.object before applying the generated fallback id so create always returns "new-rb-1" when the request id is empty, and add an update handler alongside list, get, create, and delete that supports the useUpdateRoleBinding edit flow.
412-421: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe
RolesandUsersmocks ignorereq.filter.
RoleBindingsPagebuilds a filter expression and passes it touseUsers. The mock returns every fixture user regardless of the filter, so the filter-construction path is never exercised. Consider applying a simple filter match forUsers, likematchesReadyStateFilterdoes for other services. Then a test can prove the page requests the correct identifiers.Also applies to: 438-447
🤖 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/test-utils/createMockConnectTransport.ts` around lines 412 - 421, The Roles and Users mock handlers in the transport setup currently ignore req.filter; update their list implementations to apply the requested filter before constructing items, size, and total, reusing the existing matchesReadyStateFilter-style helper or equivalent established matching logic. Keep unfiltered requests returning all fixtures so RoleBindingsPage tests can verify the identifiers included in its filter expression.libs/ui-components/src/components/RoleBinding/RoleBindingsPage.tsx (1)
46-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the identifier maps to match the key they use.
userIds,rolesById, andusersByIdare all keyed bymetadata.name, not byid. The names state the opposite. Rename them touserNames,rolesByName, andusersByName. This removes the ambiguity that the popover inconsistency above exposes.Also consider a
'-'fallback at Line 124 for the single-user cell, to match the role cell at Line 120. While the users query loads, the cell is blank.Also applies to: 67-85
🤖 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/RoleBinding/RoleBindingsPage.tsx` around lines 46 - 54, Rename the identifier maps keyed by metadata.name throughout the RoleBindingsPage component: userIds to userNames, rolesById to rolesByName, and usersById to usersByName, updating all references. In the single-user cell rendering, add the same "-" fallback used by the role cell when the user value is unavailable.
🤖 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 `@apps/app-frontend/src/shell/shellNav.ts`:
- Line 21: The Role Bindings navigation path does not match the registered
route. In apps/app-frontend/src/shell/shellNav.ts:21-21, update the
role-bindings entry to use /tenant/role-binding; in
apps/app-frontend/src/shell/shellNav.test.ts:53-60, assert that the
role-bindings child uses the same path.
In `@libs/ui-components/src/api/v1/user.ts`:
- Line 7: Update getTenantUsersFilter to stop directly interpolating tenantId
into the filter expression; validate it at this trust boundary against the
established allow-list of tenant identifiers, then escape the validated value as
a filter literal before constructing the string, or use the structured filter
API if available. Preserve tenant equality filtering and reject unknown
identifiers rather than relying on deny-list sanitization.
In
`@libs/ui-components/src/components/RoleBinding/CreatePage/RoleBindingCreatePage.tsx`:
- Around line 178-183: Update the error message rendered in the update-error
alert within RoleBindingCreatePage so it uses updateErr rather than createErr.
Keep the existing updateErr condition, alert title, and layout unchanged.
In `@libs/ui-components/src/components/RoleBinding/CreatePage/validation.ts`:
- Line 10: Update the tenant validation rule to wrap the required message with
the existing translation function t, matching the localization pattern used by
the other validation messages.
In `@libs/ui-components/src/components/RoleBinding/RoleBindingsPage.tsx`:
- Around line 56-62: Escape or strictly validate all interpolated values used by
the role-binding and user filter expressions, including the values handled in
the usersFilter useMemo and the corresponding role-binding filter. Prefer an
allow-list identifier validation at this trust boundary; otherwise use a shared
escaping helper that safely handles quotes and backslashes before interpolation,
while preserving empty-filter behavior.
- Around line 113-136: Update the Users cell in the roleBindings map to handle
zero users explicitly, rendering the existing placeholder for empty or missing
spec.users instead of the Multiple users popover. Preserve the single-user
lookup and only render the popover when more than one user exists.
- Around line 19-29: Update the RoleBindingsPage single-user lookup and indexing
logic to use each user’s internal id consistently with the popover and
usersFilter, replacing metadata.name references where applicable. Add or adjust
a test fixture with distinct id and metadata.name values, and assert the correct
user is resolved through both lookup paths.
In `@libs/ui-components/src/components/RoleBinding/RoleBindingStatusLabel.tsx`:
- Around line 43-46: Update the status selection in RoleBindingStatusLabel to
fall back to statusMap[RoleBindingState.UNSPECIFIED] when
statusMap[rb.status.state] is undefined, including unknown protobuf enum values.
Preserve the existing unspecified-state fallback when rb.status.state itself is
absent.
- Around line 37-39: Update the deletion branch in RoleBindingStatusLabel to
return ResourceStatusLabel like the other status branches, passing the
translated “Deleting” text via t(...). Verify the “Deleting” translation exists
in the English translation catalog and use the established “progressing” status
value used by other delete flows.
---
Nitpick comments:
In `@libs/ui-components/src/components/RoleBinding/RoleBindingDeleteModal.tsx`:
- Around line 44-50: Update the confirmation text in RoleBindingDeleteModal to
include the binding name using roleBinding.metadata?.name || roleBinding.id.
Replace the static translation call with an interpolated translation key and
pass the binding identifier as a named placeholder so translators retain it in
the localized message.
In `@libs/ui-components/src/components/RoleBinding/RoleBindingsPage.test.tsx`:
- Around line 153-162: Extend the RoleBindingsPage tests with coverage for the
MultipleUsersPopover and RoleBindingDeleteModal flows: click “Multiple users”
and assert alice and bob render, then open a row’s Actions menu, select Delete,
confirm, and assert the delete RPC was called. Update the Users mock in
createMockConnectTransport to filter results according to req.filter so both
tests exercise the intended lookup behavior.
In `@libs/ui-components/src/components/RoleBinding/RoleBindingsPage.tsx`:
- Around line 46-54: Rename the identifier maps keyed by metadata.name
throughout the RoleBindingsPage component: userIds to userNames, rolesById to
rolesByName, and usersById to usersByName, updating all references. In the
single-user cell rendering, add the same "-" fallback used by the role cell when
the user value is unavailable.
In `@libs/ui-components/src/test-utils/createMockConnectTransport.ts`:
- Around line 423-436: Update the RoleBindings service registration to spread
req.object before applying the generated fallback id so create always returns
"new-rb-1" when the request id is empty, and add an update handler alongside
list, get, create, and delete that supports the useUpdateRoleBinding edit flow.
- Around line 412-421: The Roles and Users mock handlers in the transport setup
currently ignore req.filter; update their list implementations to apply the
requested filter before constructing items, size, and total, reusing the
existing matchesReadyStateFilter-style helper or equivalent established matching
logic. Keep unfiltered requests returning all fixtures so RoleBindingsPage tests
can verify the identifiers included in its filter expression.
🪄 Autofix
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: CHILL
Plan: Enterprise
Run ID: 5d4d7fa6-327a-45d7-88e1-55a2a533683b
📒 Files selected for processing (21)
apps/app-frontend/src/shell/AppShell.tsxapps/app-frontend/src/shell/shellNav.test.tsapps/app-frontend/src/shell/shellNav.tslibs/i18n/locales/en/translation.jsonlibs/types/src/index.tslibs/ui-components/src/api/types.tslibs/ui-components/src/api/v1/private/tenant.tslibs/ui-components/src/api/v1/role-binding.tslibs/ui-components/src/api/v1/role.tslibs/ui-components/src/api/v1/user.tslibs/ui-components/src/components/RoleBinding/CreatePage/RoleBindingCreatePage.tsxlibs/ui-components/src/components/RoleBinding/CreatePage/payload.tslibs/ui-components/src/components/RoleBinding/CreatePage/validation.tslibs/ui-components/src/components/RoleBinding/CreatePage/values.tslibs/ui-components/src/components/RoleBinding/RoleBindingActionsMenu.tsxlibs/ui-components/src/components/RoleBinding/RoleBindingDeleteModal.tsxlibs/ui-components/src/components/RoleBinding/RoleBindingRoutes.tsxlibs/ui-components/src/components/RoleBinding/RoleBindingStatusLabel.tsxlibs/ui-components/src/components/RoleBinding/RoleBindingsPage.test.tsxlibs/ui-components/src/components/RoleBinding/RoleBindingsPage.tsxlibs/ui-components/src/test-utils/createMockConnectTransport.ts
Contributor
|
Add screenshots please |
10 tasks
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/i18n/locales/en/translation.json`:
- Around line 133-137: Resolve all Git conflict markers in the English
translation resource, including the sections around the listed entries, while
preserving the distinct translation keys and values from both branches. Ensure
the resulting translation.json is valid JSON with no conflict-marker lines and
remains loadable by the localization system.
🪄 Autofix
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: CHILL
Plan: Enterprise
Run ID: 32aea467-081a-4d80-bdf8-76d8bd859222
📒 Files selected for processing (9)
apps/app-frontend/src/shell/AppShell.tsxapps/app-frontend/src/shell/shellNav.test.tsapps/app-frontend/src/shell/shellNav.tslibs/i18n/locales/en/translation.jsonlibs/ui-components/src/api/types.tslibs/ui-components/src/components/IdentityProvider/CreateWizard/payload.tslibs/ui-components/src/components/RoleBinding/RoleBindingStatusLabel.tsxlibs/ui-components/src/components/RoleBinding/RoleBindingsPage.tsxlibs/ui-components/src/test-utils/createMockConnectTransport.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/app-frontend/src/shell/shellNav.test.ts
- libs/ui-components/src/api/types.ts
- apps/app-frontend/src/shell/AppShell.tsx
- libs/ui-components/src/components/RoleBinding/RoleBindingsPage.tsx
- libs/ui-components/src/test-utils/createMockConnectTransport.ts
- libs/ui-components/src/components/RoleBinding/RoleBindingStatusLabel.tsx
- apps/app-frontend/src/shell/shellNav.ts
rawagner
force-pushed
the
OSAC-3385
branch
2 times, most recently
from
August 11, 2026 08:29
36c2c79 to
8f5fb3c
Compare
Contributor
Author
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
Contributor
Author
|
@batzionb added screenshots. PTAL |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/components/RoleBinding/RoleBindingStatusLabel.tsx`:
- Around line 37-39: Update the deletionTimestamp branch in
RoleBindingStatusLabel to add the required semicolon to the returned
ResourceStatusLabel JSX statement, preserving its existing status and text
values.
🪄 Autofix
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: CHILL
Plan: Enterprise
Run ID: f42ebff3-86e2-45be-84c0-24556c32d0ad
📒 Files selected for processing (1)
libs/ui-components/src/components/RoleBinding/RoleBindingStatusLabel.tsx
batzionb
reviewed
Aug 11, 2026
| headerContent={t('Users')} | ||
| bodyContent={<MultipleUsersPopover roleBinding={roleBinding} />} | ||
| > | ||
| <Button variant="link" isInline> |
Contributor
There was a problem hiding this comment.
how about just have simple text: users without the popover
n number of users
when we have details page for rolebinding it can show there
Contributor
Author
There was a problem hiding this comment.
once we have UX, we will align. My preference would be to keep the popover for now.
batzionb
reviewed
Aug 11, 2026
|
|
||
| navigateToList(); | ||
| } catch { | ||
| // tanstack handles the err |
Contributor
There was a problem hiding this comment.
As far as I know tanstack doesn't surface the error to the user
There should be an alert in case of an error
Contributor
Author
There was a problem hiding this comment.
tanstack handles catching the error and storing it in state.
we render the error as alert see
…nd delete Add a new tenant-scoped Role Bindings section that lets tenant admins and IdP managers assign roles to users. Includes a list page with role/user resolution, a create/edit form with role and user selection, a delete confirmation modal, and a status label component. Wire the new routes into the shell navigation for tenant-admin, idp-manager, and admin roles, and add API hooks for the RoleBindings and Roles services. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: rawagner <rawagner@redhat.com>
batzionb
approved these changes
Aug 12, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: batzionb, 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…nd delete
Add a new tenant-scoped Role Bindings section that lets tenant admins and IdP managers assign roles to users. Includes a list page with role/user resolution, a create/edit form with role and user selection, a delete confirmation modal, and a status label component. Wire the new routes into the shell navigation for tenant-admin, idp-manager, and admin roles, and add API hooks for the RoleBindings and Roles services.
Assisted-by: Claude Code noreply@anthropic.com
Summary by CodeRabbit
New Features
Bug Fixes
Tests