Add 'Self service permissions' main page - #1153
Conversation
There was a problem hiding this comment.
Hey - I've found 6 issues, and left some high level feedback:
- In
TypeAheadWithCheckbox, theif (allowCreation && previousTarget !== creationProps?.onChangeTarget)block performs state updates (setPreviousTarget,setLocalOptions,setInputValue) during render; this should be moved into auseEffectthat depends oncreationProps?.onChangeTargetto avoid React warnings and potential render loops. - In
getSelfServicePermissionsFullDatathesizeLimitparameter is accepted but never used andtotalCountis derived fromfindResponse.result.result.lengthinstead of the server-reportedcount/truncatedfields; consider either honoringsizeLimitand using the API’scountmetadata or removing the unused argument to keep behavior and signature aligned.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `TypeAheadWithCheckbox`, the `if (allowCreation && previousTarget !== creationProps?.onChangeTarget)` block performs state updates (`setPreviousTarget`, `setLocalOptions`, `setInputValue`) during render; this should be moved into a `useEffect` that depends on `creationProps?.onChangeTarget` to avoid React warnings and potential render loops.
- In `getSelfServicePermissionsFullData` the `sizeLimit` parameter is accepted but never used and `totalCount` is derived from `findResponse.result.result.length` instead of the server-reported `count`/`truncated` fields; consider either honoring `sizeLimit` and using the API’s `count` metadata or removing the unused argument to keep behavior and signature aligned.
## Individual Comments
### Comment 1
<location path="src/components/TypeAheadWithCheckbox.tsx" line_range="56-57" />
<code_context>
+ const [previousTarget, setPreviousTarget] = useState<T | null>(null);
+ const allowCreation = creationProps !== undefined;
+
+ if (allowCreation && previousTarget !== creationProps?.onChangeTarget) {
+ setPreviousTarget(creationProps.onChangeTarget);
+ setLocalOptions(options);
+ setInputValue("");
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid calling setState during render when reacting to creationProps changes
This code calls `setPreviousTarget`, `setLocalOptions`, and `setInputValue` during render whenever `creationProps?.onChangeTarget` changes, which can cause render loops and violates React’s state update rules. Move this logic into a `useEffect` that depends on `allowCreation`, `options`, and `creationProps?.onChangeTarget` so the reset happens after render instead of during it.
</issue_to_address>
### Comment 2
<location path="src/pages/SelfServicePermissions/SelfServicePermissions.tsx" line_range="103-121" />
<code_context>
+ return { elementsList: [], totalCount: 0 };
+ }, [batchResponse]);
+
+ React.useEffect(() => {
+ if (isFetching) {
+ globalErrors.clear();
+ }
+ }, [isFetching]);
+
+ React.useEffect(() => {
+ if (
+ !isBatchLoading &&
+ !isFetching &&
+ dataResponse.isError &&
+ dataResponse.error !== undefined
+ ) {
+ const err = dataResponse.error;
+ let contextMsg = "Error loading self-service permissions";
+ if ("error" in err && typeof err.error === "string" && err.error) {
+ contextMsg += ": " + err.error;
+ }
+ globalErrors.addError(
+ err,
+ contextMsg,
+ "selfservice-permissions-fetch-error"
+ );
+ }
+ }, [dataResponse.isError, isBatchLoading, isFetching]);
+
+ const refreshData = () => {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Effect handling fetch errors has a narrow dependency list and may miss updates
This effect reads `dataResponse.error` and `globalErrors` but doesn’t list them as dependencies. If `dataResponse.error` changes while `isError` stays true (or `globalErrors` changes), the effect won’t re-run and the error handling can become stale. Please add them to the dependency array:
```ts
}, [dataResponse.isError, dataResponse.error, isBatchLoading, isFetching, globalErrors]);
```
```suggestion
React.useEffect(() => {
if (
!isBatchLoading &&
!isFetching &&
dataResponse.isError &&
dataResponse.error !== undefined
) {
const err = dataResponse.error;
let contextMsg = "Error loading self-service permissions";
if ("error" in err && typeof err.error === "string" && err.error) {
contextMsg += ": " + err.error;
}
globalErrors.addError(
err,
contextMsg,
"selfservice-permissions-fetch-error"
);
}
}, [
dataResponse.isError,
dataResponse.error,
isBatchLoading,
isFetching,
globalErrors,
]);
```
</issue_to_address>
### Comment 3
<location path="src/services/rpcSelfServicePermissions.ts" line_range="44" />
<code_context>
+ SelfServicePermissionsFullDataPayload
+ >({
+ async queryFn(payloadData, _queryApi, _extraOptions, fetchWithBQ) {
+ const { searchValue, apiVersion, startIdx, stopIdx } = payloadData;
+
+ const params = {
</code_context>
<issue_to_address>
**suggestion:** Unused sizeLimit parameter suggests either dead config or missing enforcement
`sizeLimit` is defined on `SelfServicePermissionsFullDataPayload` but never used in the query; results are always bounded only by `stopIdx`. Please either remove `sizeLimit` from the payload if it’s not needed, or apply it when constructing `ids` so it actually limits the batch size and doesn’t mislead callers.
Suggested implementation:
```typescript
const { searchValue, apiVersion, startIdx, stopIdx, sizeLimit } = payloadData;
const effectiveStopIdx =
typeof sizeLimit === "number"
? Math.min(stopIdx, startIdx + sizeLimit)
: stopIdx;
```
To fully apply `sizeLimit`, replace usages of `stopIdx` that control result bounds with `effectiveStopIdx`. For example, if later in this function you have something like:
- `const ids = pkeys.slice(startIdx, stopIdx);` it should become `const ids = pkeys.slice(startIdx, effectiveStopIdx);`
- Any other logic that uses `stopIdx` as an upper bound for batching should similarly use `effectiveStopIdx`.
If `SelfServicePermissionsFullDataPayload` currently types `sizeLimit` as required, consider making it optional if callers are not required to provide it.
</issue_to_address>
### Comment 4
<location path="src/components/modals/SelfServicePermissionModals/AddSelfServicePermissionModal.tsx" line_range="146-149" />
<code_context>
+ const onAdd = () => {
+ setIsAddButtonSpinning(true);
+
+ addSelfServicePermission({
+ aciname: selfServiceName,
+ attrs: selectedAttrs,
+ }).then((response) => {
+ if ("data" in response) {
+ const data = response.data?.result;
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Mutation result handling assumes errors are embedded in data rather than using RTK Query error channel
This handler only inspects `response.data?.error as SerializedError`, but RTK Query surfaces request failures via `response.error`. In a network/HTTP failure, `response.data` would be undefined and no error would be surfaced. Consider branching on `'error' in response` and handling `response.error`, or using `unwrap()` to centralize success/error handling.
Suggested implementation:
```typescript
const onAdd = () => {
setIsAddButtonSpinning(true);
addSelfServicePermission({
aciname: selfServiceName,
attrs: selectedAttrs,
}).then((response) => {
if ("error" in response && response.error) {
const error = response.error as SerializedError;
dispatch(
addAlert({
name: "add-self-service-permission-error",
title: error.message,
variant: "danger",
})
);
return;
}
if ("data" in response) {
const data = response.data?.result;
const error = response.data?.error as SerializedError;
if (error) {
dispatch(
addAlert({
name: "add-self-service-permission-error",
title: error.message,
variant: "danger",
})
);
```
1. If you want the spinner to always stop even on errors, add `setIsAddButtonSpinning(false);` in both the `'error' in response` branch and any existing error/success branches that complete the flow, or add a `.finally(() => setIsAddButtonSpinning(false));` after the `.then(...)`.
2. If your project prefers `unwrap()`, you could instead change the call to `addSelfServicePermission(...).unwrap().then(...).catch(...)` and centralize success/error handling in those branches; the rest of the handler body would need to be adjusted accordingly.
</issue_to_address>
### Comment 5
<location path="src/components/TypeAheadWithCheckbox.tsx" line_range="33" />
<code_context>
+const NO_RESULTS = "no results";
+const CREATE_NEW = "create";
+
+export const TypeAheadWithCheckbox = <T,>({
+ id,
+ dataCy,
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying this component by tightening state derivation, extracting shared selection logic, and optionally moving keyboard navigation into a small hook.
You can noticeably reduce complexity without changing behavior by tightening up state management and extracting a couple of small helpers.
### 1. Remove `setState` from render for `localOptions`
Instead of mutating state during render based on `creationProps.onChangeTarget`, track that via `useEffect` (or a `key` reset).
Current:
```ts
const [localOptions, setLocalOptions] =
useState<SelectOptionProps[]>(options);
const [previousTarget, setPreviousTarget] = useState<T | null>(null);
const allowCreation = creationProps !== undefined;
if (allowCreation && previousTarget !== creationProps?.onChangeTarget) {
setPreviousTarget(creationProps.onChangeTarget);
setLocalOptions(options);
setInputValue("");
}
```
Suggested:
```ts
const [localOptions, setLocalOptions] =
useState<SelectOptionProps[]>(options);
const allowCreation = creationProps !== undefined;
useEffect(() => {
if (!allowCreation) return;
setLocalOptions(options);
setInputValue("");
}, [allowCreation, creationProps?.onChangeTarget, options]);
```
Or, if acceptable, drive this from a `key` on the component where it’s used instead of internal bookkeeping.
### 2. Treat `availableOptions` as derived state
You don’t need `useEffect` + mutable `newSelectOptions`. A `useMemo` keeps behavior but removes imperative mutation and the `eslint` suppression.
Current:
```ts
const [availableOptions, setAvailableOptions] =
useState<SelectOptionProps[]>(localOptions);
useEffect(() => {
let newSelectOptions: SelectOptionProps[] = localOptions;
// ... mutate newSelectOptions ...
setAvailableOptions(newSelectOptions);
}, [inputValue, localOptions]);
```
Suggested:
```ts
const availableOptions = React.useMemo(() => {
let newSelectOptions: SelectOptionProps[] = localOptions;
if (inputValue) {
newSelectOptions = localOptions.filter((menuItem) =>
String(menuItem.children).toLowerCase().includes(inputValue.toLowerCase()),
);
if (allowCreation) {
if (!localOptions.some((option) => option.value === inputValue)) {
newSelectOptions = [
...newSelectOptions,
{
children: `Create new option "${inputValue}"`,
value: CREATE_NEW,
"data-cy": `${dataCy}-create-new-option`,
},
];
}
}
if (newSelectOptions.length === 0) {
newSelectOptions = [
{
"data-cy": `${dataCy}-no-results`,
isAriaDisabled: true,
children: `No results found for "${inputValue}"`,
value: NO_RESULTS,
hasCheckbox: false,
},
];
}
}
return newSelectOptions;
}, [localOptions, inputValue, allowCreation, dataCy]);
```
Then drop `availableOptions`’s state and setter:
```ts
// remove:
// const [availableOptions, setAvailableOptions] = useState(...);
```
### 3. Factor selection toggling into a helper
`onSelect` has duplicated “toggle in array” logic (for both `CREATE_NEW` and normal values). A small helper clarifies behavior and shortens the function.
Current snippets:
```ts
setSelected(
selected.includes(inputValue)
? selected.filter((selection) => selection !== inputValue)
: [...selected, inputValue]
);
setSelected(
selected.includes(value)
? selected.filter((selection) => selection !== value)
: [...selected, value]
);
```
Suggested:
```ts
const toggleSelection = (current: string[], value: string) =>
current.includes(value)
? current.filter((selection) => selection !== value)
: [...current, value];
const onSelect = (value: string) => {
if (!value || value === NO_RESULTS) {
textInputRef.current?.focus();
return;
}
if (value === CREATE_NEW) {
if (!availableOptions.some((item) => item.value === inputValue)) {
setLocalOptions([
...localOptions,
{
value: inputValue,
children: inputValue,
"data-cy": `${dataCy}-${inputValue}-create-new-option`,
},
]);
}
setSelected(toggleSelection(selected, inputValue));
resetActiveAndFocusedItem();
} else {
setSelected(toggleSelection(selected, value));
}
textInputRef.current?.focus();
};
```
### 4. Consider extracting keyboard navigation into a small hook
The `focusedItemIndex`/`activeItemId` logic is correct but dense. You can move it into a hook without changing behavior:
```ts
function useListKeyboardNavigation(options: SelectOptionProps[]) {
const [focusedItemIndex, setFocusedItemIndex] = useState<number | null>(null);
const [activeItemId, setActiveItemId] = useState<string | null>(null);
const setActiveAndFocusedItem = (index: number) => {
setFocusedItemIndex(index);
const focusedItem = options[index];
setActiveItemId(`select-multi-typeahead-${String(focusedItem.value).replace(" ", "-")}`);
};
const reset = () => {
setFocusedItemIndex(null);
setActiveItemId(null);
};
return {
focusedItemIndex,
activeItemId,
setActiveAndFocusedItem,
reset,
};
}
```
Then the main component just calls this hook and keeps `handleMenuArrowKeys`/`onInputKeyDown` shorter:
```ts
const {
focusedItemIndex,
activeItemId,
setActiveAndFocusedItem,
reset: resetActiveAndFocusedItem,
} = useListKeyboardNavigation(availableOptions);
```
This keeps all current behavior but makes the main component easier to follow.
</issue_to_address>
### Comment 6
<location path="src/pages/SelfServicePermissions/SelfServicePermissions.tsx" line_range="148" />
<code_context>
+ isSelfServicePermissionSelectable
+ );
+
+ const updateSelectedPermissions = (
+ permissions: SelfServicePermission[],
+ isSelected: boolean
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the selection state handling by using set-like helpers and deriving the delete button disabled flag from the selection length instead of maintaining extra mutable state.
You can simplify the selection logic and remove the heavy cloning without changing behavior.
### 1. Simplify `updateSelectedPermissions`
Current implementation reimplements set semantics with nested loops and `JSON.parse(JSON.stringify(...))`. You can keep the same behavior with array helpers and a `Set` of keys:
```ts
const updateSelectedPermissions = (
permissions: SelfServicePermission[],
isSelected: boolean
) => {
setSelectedPermissions((prev) => {
if (isSelected) {
const existing = new Map(prev.map((p) => [p.aciname, p]));
permissions.forEach((p) => {
if (!existing.has(p.aciname)) {
existing.set(p.aciname, p);
}
});
const next = Array.from(existing.values());
setIsDeleteButtonDisabled(next.length === 0);
return next;
} else {
const removeSet = new Set(permissions.map((p) => p.aciname));
const next = prev.filter((p) => !removeSet.has(p.aciname));
setIsDeleteButtonDisabled(next.length === 0);
return next;
}
});
};
```
This removes the deep copy, avoids nested loops, and keeps the “no duplicates by `aciname`” semantics intact.
### 2. Optional: derive delete button disabled flag
`isDeleteButtonDisabled` is always derived from selection length. You can avoid keeping it as separate mutable state to reduce coupling:
```ts
// remove useState for isDeleteButtonDisabled
// const [isDeleteButtonDisabled, setIsDeleteButtonDisabled] = useState<boolean>(true);
// derive directly
const isDeleteButtonDisabled = selectedPermissions.length === 0;
```
And then remove `updateIsDeleteButtonDisabled` from `buttonsData` and the modal props where it’s not strictly required. If generic components expect this prop, you can still pass a no-op while transitioning:
```ts
buttonsData={{ updateIsDeleteButtonDisabled: () => {} }}
```
</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 (allowCreation && previousTarget !== creationProps?.onChangeTarget) { | ||
| setPreviousTarget(creationProps.onChangeTarget); |
There was a problem hiding this comment.
issue (bug_risk): Avoid calling setState during render when reacting to creationProps changes
This code calls setPreviousTarget, setLocalOptions, and setInputValue during render whenever creationProps?.onChangeTarget changes, which can cause render loops and violates React’s state update rules. Move this logic into a useEffect that depends on allowCreation, options, and creationProps?.onChangeTarget so the reset happens after render instead of during it.
| const NO_RESULTS = "no results"; | ||
| const CREATE_NEW = "create"; | ||
|
|
||
| export const TypeAheadWithCheckbox = <T,>({ |
There was a problem hiding this comment.
issue (complexity): Consider simplifying this component by tightening state derivation, extracting shared selection logic, and optionally moving keyboard navigation into a small hook.
You can noticeably reduce complexity without changing behavior by tightening up state management and extracting a couple of small helpers.
1. Remove setState from render for localOptions
Instead of mutating state during render based on creationProps.onChangeTarget, track that via useEffect (or a key reset).
Current:
const [localOptions, setLocalOptions] =
useState<SelectOptionProps[]>(options);
const [previousTarget, setPreviousTarget] = useState<T | null>(null);
const allowCreation = creationProps !== undefined;
if (allowCreation && previousTarget !== creationProps?.onChangeTarget) {
setPreviousTarget(creationProps.onChangeTarget);
setLocalOptions(options);
setInputValue("");
}Suggested:
const [localOptions, setLocalOptions] =
useState<SelectOptionProps[]>(options);
const allowCreation = creationProps !== undefined;
useEffect(() => {
if (!allowCreation) return;
setLocalOptions(options);
setInputValue("");
}, [allowCreation, creationProps?.onChangeTarget, options]);Or, if acceptable, drive this from a key on the component where it’s used instead of internal bookkeeping.
2. Treat availableOptions as derived state
You don’t need useEffect + mutable newSelectOptions. A useMemo keeps behavior but removes imperative mutation and the eslint suppression.
Current:
const [availableOptions, setAvailableOptions] =
useState<SelectOptionProps[]>(localOptions);
useEffect(() => {
let newSelectOptions: SelectOptionProps[] = localOptions;
// ... mutate newSelectOptions ...
setAvailableOptions(newSelectOptions);
}, [inputValue, localOptions]);Suggested:
const availableOptions = React.useMemo(() => {
let newSelectOptions: SelectOptionProps[] = localOptions;
if (inputValue) {
newSelectOptions = localOptions.filter((menuItem) =>
String(menuItem.children).toLowerCase().includes(inputValue.toLowerCase()),
);
if (allowCreation) {
if (!localOptions.some((option) => option.value === inputValue)) {
newSelectOptions = [
...newSelectOptions,
{
children: `Create new option "${inputValue}"`,
value: CREATE_NEW,
"data-cy": `${dataCy}-create-new-option`,
},
];
}
}
if (newSelectOptions.length === 0) {
newSelectOptions = [
{
"data-cy": `${dataCy}-no-results`,
isAriaDisabled: true,
children: `No results found for "${inputValue}"`,
value: NO_RESULTS,
hasCheckbox: false,
},
];
}
}
return newSelectOptions;
}, [localOptions, inputValue, allowCreation, dataCy]);Then drop availableOptions’s state and setter:
// remove:
// const [availableOptions, setAvailableOptions] = useState(...);3. Factor selection toggling into a helper
onSelect has duplicated “toggle in array” logic (for both CREATE_NEW and normal values). A small helper clarifies behavior and shortens the function.
Current snippets:
setSelected(
selected.includes(inputValue)
? selected.filter((selection) => selection !== inputValue)
: [...selected, inputValue]
);
setSelected(
selected.includes(value)
? selected.filter((selection) => selection !== value)
: [...selected, value]
);Suggested:
const toggleSelection = (current: string[], value: string) =>
current.includes(value)
? current.filter((selection) => selection !== value)
: [...current, value];
const onSelect = (value: string) => {
if (!value || value === NO_RESULTS) {
textInputRef.current?.focus();
return;
}
if (value === CREATE_NEW) {
if (!availableOptions.some((item) => item.value === inputValue)) {
setLocalOptions([
...localOptions,
{
value: inputValue,
children: inputValue,
"data-cy": `${dataCy}-${inputValue}-create-new-option`,
},
]);
}
setSelected(toggleSelection(selected, inputValue));
resetActiveAndFocusedItem();
} else {
setSelected(toggleSelection(selected, value));
}
textInputRef.current?.focus();
};4. Consider extracting keyboard navigation into a small hook
The focusedItemIndex/activeItemId logic is correct but dense. You can move it into a hook without changing behavior:
function useListKeyboardNavigation(options: SelectOptionProps[]) {
const [focusedItemIndex, setFocusedItemIndex] = useState<number | null>(null);
const [activeItemId, setActiveItemId] = useState<string | null>(null);
const setActiveAndFocusedItem = (index: number) => {
setFocusedItemIndex(index);
const focusedItem = options[index];
setActiveItemId(`select-multi-typeahead-${String(focusedItem.value).replace(" ", "-")}`);
};
const reset = () => {
setFocusedItemIndex(null);
setActiveItemId(null);
};
return {
focusedItemIndex,
activeItemId,
setActiveAndFocusedItem,
reset,
};
}Then the main component just calls this hook and keeps handleMenuArrowKeys/onInputKeyDown shorter:
const {
focusedItemIndex,
activeItemId,
setActiveAndFocusedItem,
reset: resetActiveAndFocusedItem,
} = useListKeyboardNavigation(availableOptions);This keeps all current behavior but makes the main component easier to follow.
| isSelfServicePermissionSelectable | ||
| ); | ||
|
|
||
| const updateSelectedPermissions = ( |
There was a problem hiding this comment.
issue (complexity): Consider simplifying the selection state handling by using set-like helpers and deriving the delete button disabled flag from the selection length instead of maintaining extra mutable state.
You can simplify the selection logic and remove the heavy cloning without changing behavior.
1. Simplify updateSelectedPermissions
Current implementation reimplements set semantics with nested loops and JSON.parse(JSON.stringify(...)). You can keep the same behavior with array helpers and a Set of keys:
const updateSelectedPermissions = (
permissions: SelfServicePermission[],
isSelected: boolean
) => {
setSelectedPermissions((prev) => {
if (isSelected) {
const existing = new Map(prev.map((p) => [p.aciname, p]));
permissions.forEach((p) => {
if (!existing.has(p.aciname)) {
existing.set(p.aciname, p);
}
});
const next = Array.from(existing.values());
setIsDeleteButtonDisabled(next.length === 0);
return next;
} else {
const removeSet = new Set(permissions.map((p) => p.aciname));
const next = prev.filter((p) => !removeSet.has(p.aciname));
setIsDeleteButtonDisabled(next.length === 0);
return next;
}
});
};This removes the deep copy, avoids nested loops, and keeps the “no duplicates by aciname” semantics intact.
2. Optional: derive delete button disabled flag
isDeleteButtonDisabled is always derived from selection length. You can avoid keeping it as separate mutable state to reduce coupling:
// remove useState for isDeleteButtonDisabled
// const [isDeleteButtonDisabled, setIsDeleteButtonDisabled] = useState<boolean>(true);
// derive directly
const isDeleteButtonDisabled = selectedPermissions.length === 0;And then remove updateIsDeleteButtonDisabled from buttonsData and the modal props where it’s not strictly required. If generic components expect this prop, you can still pass a no-op while transitioning:
buttonsData={{ updateIsDeleteButtonDisabled: () => {} }}|
I have generated the |
There was a problem hiding this comment.
Hi, please look into sourcery comments, I found similar things so I think it is worthwhile to check it. Also, there is a bug, when you have two pages of items and delete all items from the second page, it doesnt reload correctly. I will provide a recording soon.
UPDATE:
I will do the recording once the sizeLimit is fixed, now Im getting

917d32a to
e12beee
Compare
|
@veronnicka - Just adapted the code based on your feedback. |
e12beee to
da22e72
Compare
da22e72 to
9de502e
Compare
veronnicka
left a comment
There was a problem hiding this comment.
Hi, please also see the bug I reported in the comment.
9de502e to
69ae4af
Compare
The 'Self service permissions' page must show a table with all the entries from `selfservice_find` API command and allow refresh, add, and delete operations. Assisted-by: Claude <noreply@anthropic.com> Signed-off-by: Carla Martinez <carlmart@redhat.com>
69ae4af to
510f62e
Compare
This has been amended. |

The 'Self service permissions' page
must show a table with all the entries
from
selfservice_findAPI command andallow refresh, add, and delete operations.
Assisted-by: Claude noreply@anthropic.com
Summary by Sourcery
Add a new Self service permissions management page wired into navigation and routing, backed by RPC endpoints for listing, creating, and deleting self-service permissions.
New Features:
Enhancements: