Self service permissions > 'Settings' page - #1164
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
TypeAheadWithCheckbox, thepreviousTarget/localOptionsreset logic callssetStatedirectly during render whencreationProps.onChangeTargetchanges; this should be moved into auseEffectoncreationProps?.onChangeTargetto avoid React warnings and unnecessary re-renders. - The
TypeAheadWithCheckboxcomponent mutateslocalOptionsandavailableOptionsin the same render cycle and useseslint-disableto work arounduseEffectbehavior; consider refactoring the options filtering/creation logic into pure derivations from props and input state to make the component easier to reason about and avoid reliance on side effects for basic data shaping.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `TypeAheadWithCheckbox`, the `previousTarget`/`localOptions` reset logic calls `setState` directly during render when `creationProps.onChangeTarget` changes; this should be moved into a `useEffect` on `creationProps?.onChangeTarget` to avoid React warnings and unnecessary re-renders.
- The `TypeAheadWithCheckbox` component mutates `localOptions` and `availableOptions` in the same render cycle and uses `eslint-disable` to work around `useEffect` behavior; consider refactoring the options filtering/creation logic into pure derivations from props and input state to make the component easier to reason about and avoid reliance on side effects for basic data shaping.
## Individual Comments
### Comment 1
<location path="src/components/TypeAheadWithCheckbox.tsx" line_range="54-57" />
<code_context>
+ const [activeItemId, setActiveItemId] = useState<string | null>(null);
+ const textInputRef = useRef<HTMLInputElement>(undefined);
+
+ const [previousTarget, setPreviousTarget] = useState<T | null>(null);
+ const allowCreation = creationProps !== undefined;
+
+ if (allowCreation && previousTarget !== creationProps?.onChangeTarget) {
+ setPreviousTarget(creationProps.onChangeTarget);
+ setLocalOptions(
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid calling setState during render when creationProps.onChangeTarget changes
This logic runs during render and triggers `setPreviousTarget`/`setLocalOptions`, introducing side effects in the render phase. In React (especially under StrictMode), this can cause repeated renders and unpredictable behavior. Move this into a `useEffect` that depends on `creationProps?.onChangeTarget` (and `allowCreation`/`options` as needed) so `previousTarget` and `localOptions` are updated after render.
```ts
useEffect(() => {
if (!allowCreation || !creationProps) return;
setPreviousTarget(creationProps.onChangeTarget);
setLocalOptions(
[...options].sort((a, b) => String(a.value).localeCompare(String(b.value)))
);
setInputValue("");
}, [allowCreation, creationProps?.onChangeTarget, options]);
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
duzda
left a comment
There was a problem hiding this comment.
On save please clear the attributes box.
Screencast.From.2026-08-19.10-50-04.mp4
There was a problem hiding this comment.
Please don't add the barrel file without unit tests.
There was a problem hiding this comment.
I can create a unit test for that component.
| const selectedValues = React.useMemo(() => { | ||
| return toArray(rawValue).filter( | ||
| (val): val is string => typeof val === "string" && val !== "" | ||
| ); | ||
| }, [rawValue]); |
There was a problem hiding this comment.
Strange piece of code but fine with keeping it in. The BasicType is way too broad.
There was a problem hiding this comment.
I know, this is my attempt to normalize the values here and preventing undesired values. But now I'm thinking that maybe this can be simplified to something like:
const selectedValues = React.useMemo(() => {
return toArray(rawValue) as string[];
}, [rawValue]);
Not sure if the casting here will be a bad practice though...
There was a problem hiding this comment.
Typically as is a bad practice, but I'd use as here, you're not really narrowing here - the variable is incorrectly typed, you know it's a string, also value "" is still a valid string.
There was a problem hiding this comment.
Ok, will change it then.
ce0b6af to
61aeb1d
Compare
I have modified the original |
61aeb1d to
f97b9fc
Compare
|
Just amended the code mentioned in this comment. |
| const result = data.result; | ||
|
|
||
| if (result) { | ||
| if ("error" in result.results[0] && result.results[0].error) { |
There was a problem hiding this comment.
batch delete only inspects results[0] ? What if first delete succeeds and a later one fails?
There was a problem hiding this comment.
Same: it belongs to PR #1153 and should be fixed there.
|
|
||
| // If no option matches the filter exactly, display creation option | ||
| if (allowCreation) { | ||
| if (!localOptions.some((option) => option.value === inputValue)) { |
There was a problem hiding this comment.
IPA stores attrs lowercase, but this comparison is case-sensitive typing Mail can show “Create new” next to mail. Lowercase on create and match case-insensitively.
There was a problem hiding this comment.
You are right, will fix....
| id: "modal-form-self-service-name", | ||
| name: "Self-service name", | ||
| pfComponent: ( | ||
| <InputRequiredText |
There was a problem hiding this comment.
Please also validate aciname against ^[-_ a-zA-Z0-9]+$ and reject leading/trailing spaces, this currently only checks that the name is non-empty.
There was a problem hiding this comment.
I think this was introduced in the original PR (#1153) from which belongs the first commit of this PR, so maybe should be fixed there.
f97b9fc to
811cbef
Compare
|
@pranav210798 - I have amended some of your suggestions, specifically the ones that belong to the current PR / last commit, as it has been built on the top of #1153 and the first commit belong to this one and should be fix there. |
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>
The 'Settings' page must show information about a specific self-service permission and allow further modifications. Assisted-by: Claude <noreply@anthropic.com> Signed-off-by: Carla Martinez <carlmart@redhat.com>
811cbef to
2266860
Compare
This PR depends on this one to be merged: #1153
Summary by Sourcery
Provide end-to-end self-service permission management, including a Settings page for viewing and updating permission attributes.
New Features:
Enhancements:
Documentation:
Tests: