Add Privileges > 'Permissions' page - #1140
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
Privileges.tsxthestopIdxis now hard-coded to100, which can get out of sync with pagination controls; consider wiring this back toperPageor another configurable value so the list size matches user-selected page size. - Now that privilege detail pages exist (
/privileges/:cnwith tabs), theNO_LINK_TYPESarray still treatingprivilegesas non-linkable may be inconsistent with the new routing; review whether privilege rows fromMembershipTableshould link to the newPrivilegesTabspage and adjustNO_LINK_TYPESaccordingly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Privileges.tsx` the `stopIdx` is now hard-coded to `100`, which can get out of sync with pagination controls; consider wiring this back to `perPage` or another configurable value so the list size matches user-selected page size.
- Now that privilege detail pages exist (`/privileges/:cn` with tabs), the `NO_LINK_TYPES` array still treating `privileges` as non-linkable may be inconsistent with the new routing; review whether privilege rows from `MembershipTable` should link to the new `PrivilegesTabs` page and adjust `NO_LINK_TYPES` accordingly.
## Individual Comments
### Comment 1
<location path="src/pages/Privileges/PrivilegesPermissions.tsx" line_range="119-126" />
<code_context>
+ );
+
+ // Load available permissions (only when modal is open)
+ const permissionsQuery = useGetPermissionsQuery(adderSearchValue, {
+ skip: !showAddModal,
+ });
+
+ // Trigger available permissions search
+ useEffect(() => {
+ if (showAddModal) {
+ permissionsQuery.refetch();
+ }
+ }, [showAddModal, adderSearchValue, permissionNames]);
+
+ // Update available permissions
</code_context>
<issue_to_address>
**suggestion (performance):** The refetch effect for available permissions may trigger more API calls than necessary due to its dependency list.
Because `permissionNames` is an array that likely changes identity on each update, including it in the `useEffect` dependencies can cause unnecessary `permissionsQuery.refetch()` calls even when `adderSearchValue` is unchanged. Given that `useGetPermissionsQuery` already fetches when `skip` becomes `false` and when `adderSearchValue` changes, consider narrowing the dependencies (e.g., remove `permissionNames`) or relying on RTK Query’s built‑in refetch when `showAddModal` becomes `true` to avoid redundant network requests.
```suggestion
// Update available permissions
```
</issue_to_address>
### Comment 2
<location path="src/pages/Privileges/PrivilegesPermissions.tsx" line_range="154-163" />
<code_context>
+ if (props.privilege.cn === undefined || newPermissionNames.length === 0) {
</code_context>
<issue_to_address>
**nitpick:** Guard clauses check `cn === undefined`, but the type is a string, making the condition slightly misleading.
Given `Privilege.cn` is typed as `string`, these `=== undefined` checks are defensive rather than type-driven. If the goal is to prevent empty IDs, consider a falsy or length check (e.g. `!props.privilege.cn`) so the runtime validation matches the TypeScript type and avoids relying on an impossible `undefined` state.
</issue_to_address>
### Comment 3
<location path="src/pages/Privileges/PrivilegesTabs.tsx" line_range="58" />
<code_context>
+ const privilegeQuery = useGetPrivilegeByIdQuery(cn);
+ const privilege = privilegeQuery.data;
+
+ // Tab
+ const [activeTabKey, setActiveTabKey] = useState(() =>
+ getTabKeyFromSection(section)
</code_context>
<issue_to_address>
**issue (complexity):** Consider deriving the active tab directly from the URL section instead of storing it in state so the tabs are purely URL-driven.
You can simplify the tab/routing logic by removing `activeTabKey` state entirely and deriving it from `section` on render. This keeps all behaviour (URL-driven tabs, redirects, breadcrumbs) but removes overlapping effects and synchronization logic.
Concretely:
1. **Remove `activeTabKey` state and its setters**
`activeTabKey` is always computed from `section`, so you don’t need state or effects to keep it in sync.
```tsx
// Remove:
const [activeTabKey, setActiveTabKey] = useState(() =>
getTabKeyFromSection(section)
);
```
Use a derived value instead:
```tsx
const currentTabKey = getTabKeyFromSection(section);
```
2. **Stop setting tab state inside effects**
The breadcrumb effect and the redirect effect both call `setActiveTabKey`, which becomes unnecessary (and is currently redundant):
```tsx
React.useEffect(() => {
const currentPath: BreadCrumbItem[] = [
{ name: "Privileges", url: "/privileges" },
{ name: cn, url: "/privileges/" + cn, isActive: true },
];
setBreadcrumbItems(currentPath);
// Remove this:
// setActiveTabKey("permissions");
dispatch(updateBreadCrumbPath(currentPath));
}, [cn, dispatch]);
React.useEffect(() => {
if (!section) {
navigate(TAB_ROUTES.permissions(cn));
}
// Remove this:
// setActiveTabKey(getTabKeyFromSection(section));
}, [section, cn, navigate]);
```
3. **Drive `<Tabs>` directly from the URL-derived key**
Use `currentTabKey` as the single source of truth for the active tab:
```tsx
const currentTabKey = getTabKeyFromSection(section);
// ...
<Tabs
activeKey={currentTabKey}
onSelect={handleTabClick}
variant="secondary"
isBox
className="pf-v6-u-ml-lg"
mountOnEnter
unmountOnExit
>
<Tab
eventKey="permissions"
name="permissions-details"
title={<TabTitleText>Permissions</TabTitleText>}
data-cy="privileges-tab-permissions"
>
<PrivilegesPermissions
privilege={partialPrivilegeToPrivilege(privilege)}
/>
</Tab>
</Tabs>
```
4. **Keep navigation logic in one place (the URL)**
`handleTabClick` can remain unchanged, since it already uses `TAB_ROUTES` to navigate and therefore updates `section` via the route:
```tsx
const handleTabClick = (
_event: React.MouseEvent<HTMLElement, MouseEvent>,
tabIndex: number | string
) => {
const tabKey = String(tabIndex);
const toPath = TAB_ROUTES[tabKey];
if (toPath) {
navigate(toPath(cn));
}
};
```
With these changes, the URL (via `section`) is the single source of truth for the active tab, and effects only handle side effects (breadcrumbs, redirect), which should reduce cognitive load and avoid subtle ordering issues.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if (props.privilege.cn === undefined || newPermissionNames.length === 0) { | ||
| return; | ||
| } | ||
|
|
||
| setSpinning(true); | ||
| addPermissionToPrivilege({ | ||
| privilegeCn: props.privilege.cn, | ||
| permissions: newPermissionNames, | ||
| }).then((response) => { | ||
| if ("data" in response) { |
There was a problem hiding this comment.
nitpick: Guard clauses check cn === undefined, but the type is a string, making the condition slightly misleading.
Given Privilege.cn is typed as string, these === undefined checks are defensive rather than type-driven. If the goal is to prevent empty IDs, consider a falsy or length check (e.g. !props.privilege.cn) so the runtime validation matches the TypeScript type and avoids relying on an impossible undefined state.
0119f41 to
0b3f4c7
Compare
0b3f4c7 to
aeb59c9
Compare
carma12
added
needs-review
7cd6c83 to
cd2e891
Compare
duzda
left a comment
duzda
left a comment
There was a problem hiding this comment.
Hi, after navigating to Permissions, I'm unable to get back through clicking on Settings:
The permissions include at most 100 options, just by installing there is around 280 permissions, maybe we should lift the limit here? I also think the list should include a search bar.
Please also see the inline comments
681dd4a to
d430217
Compare
@duzda - I agree on the search bar in the |
veronnicka
left a comment
veronnicka
left a comment
There was a problem hiding this comment.
Hi, apart from the comments, I suggest to add a BulkSelector (select page / select all) as in the other membership tables. Maybe it would be a good idea to add it to the .md files as well
d430217 to
1e7b8e9
Compare
Not sure if adding the |
|
@veronnicka - I fixed the code based on your feedback. |
1e7b8e9 to
22c3228
Compare
duzda
left a comment
duzda
left a comment
There was a problem hiding this comment.
Please do backend filtering, don't filter on the frontend, it will simplify the code by a lot, and will be much more useful.
22c3228 to
6d24df3
Compare
The old webui has this feature, see But if it doesnt align with the new design, Im fine with not adding it, I will leave it up to you. |
6d24df3 to
9880627
Compare
On a second thought, I see that the |
The 'Permissions' subpage allows to grant or revoke some permissions to a given Privilege. Assisted-by: Claude <noreply@anthropic.com> Signed-off-by: Carla Martinez <carlmart@redhat.com>
The `BulkSelector` component allows to perform bulk operations in the table. Assisted-by: Claude <noreply@anthropic.com> Signed-off-by: Carla Martinez <carlmart@redhat.com>
Unless it is specified otherwise, the `BulkSelector` component must be present in all the independent subpages (i.e. any subpage that is not 'Settings', 'Is a member of' , 'Members', or 'ManagedBy`). Assisted-by: Claude <noreply@anthropic.com> Signed-off-by: Carla Martinez <carlmart@redhat.com>
84063a0 to
c01759c
Compare
veronnicka
left a comment
veronnicka
left a comment
There was a problem hiding this comment.
Hi, the delete button is active even though no item is selected.
Please see my other comments as well.
| * @param {PrivilegesFullDataPayload} - Search parameters | ||
| * @returns {BatchRPCResponse} - Batch response with privilege data | ||
| */ | ||
| searchPrivilegesEntries: build.mutation< |
There was a problem hiding this comment.
there is no hook for this so I think this is a redundant function?
| const { cn } = useSafeParams<CnParams>(["cn"]); | ||
| const navigate = useNavigate(); | ||
| const dispatch = useAppDispatch(); | ||
| useContextualHelpTopic("privileges-settings"); |
There was a problem hiding this comment.
maybe this is intentional, but shouldnt the help context change with the tabs?
3 participants
The 'Permissions' subpage allows to
grant or revoke some permissions to
a given Privilege.
Assisted-by: Claude noreply@anthropic.com
Summary by Sourcery
Add a new Privileges Permissions page and wire it into routing and RPC APIs to manage privilege-permission memberships.
New Features:
Enhancements: