Wizard: Add groups section to Users step (HMS-8784) #3929
Conversation
mgold1234
requested review from
avitova,
regexowl and
tkoscieln
and removed request for
a team
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
GroupInfo,GroupRowis keyed by the array index while the slice and validation also rely on index-based access; when groups are inserted/removed this can cause React to reuse rows with mismatched state/validation—consider using a stable identifier (e.g. gid or an explicitid) as the key instead of the index. - The
handleGroupNameChangelogic inGroupRowremoves and re-adds groups (and sometimes regenerates gids) based on the current value andgroupId, which relies heavily on index ordering and can be hard to reason about; simplifying this to keep a row in place and just mutate its properties (name/gid) would make the behavior more predictable and reduce the chance of subtle indexing bugs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `GroupInfo`, `GroupRow` is keyed by the array index while the slice and validation also rely on index-based access; when groups are inserted/removed this can cause React to reuse rows with mismatched state/validation—consider using a stable identifier (e.g. gid or an explicit `id`) as the key instead of the index.
- The `handleGroupNameChange` logic in `GroupRow` removes and re-adds groups (and sometimes regenerates gids) based on the current value and `groupId`, which relies heavily on index ordering and can be hard to reason about; simplifying this to keep a row in place and just mutate its properties (name/gid) would make the behavior more predictable and reduce the chance of subtle indexing bugs.
## Individual Comments
### Comment 1
<location> `src/store/wizardSlice.ts:1206-1212` </location>
<code_context>
state.groups.splice(index, 1);
}
},
+ addUserGroup: (state, action: PayloadAction<string>) => {
+ const existingGroupIndex = state.userGroups.findIndex(
+ (group) => group.name === action.payload,
+ );
+
+ if (existingGroupIndex === -1) {
+ const randomGid = Math.floor(Math.random() * (65534 - 1000 + 1)) + 1000;
+ const newUserGroup = {
+ name: action.payload,
</code_context>
<issue_to_address>
**issue:** Avoid generating random GIDs inside the reducer to keep state updates deterministic.
`addUserGroup` currently calls `Math.random` inside the reducer, which breaks the expectation that reducers are pure and deterministic. Move GID generation to an action creator (or thunk/saga) and include the computed `gid` in the action payload instead of generating it in the reducer.
</issue_to_address>
### Comment 2
<location> `README.md:382` </location>
<code_context>
In order to run the Boot tests on a PR, you can open the `Boot tests` workflow in Github Actions and type in the number of PR into a `Pull Request number to run tests against` field. The action will then pull the code from a PR of that number and execute as usual.
#### Where can I see results?
-You can find artifacts and link to a Currents report directly in the workflow run detail, but Currents will also link the report back to the PR when the workflow finishes as a *check*.
\ No newline at end of file
+You can find artifacts and link to a Currents report directly in the workflow run detail, but Currents will also link the report back to the PR when the workflow finishes as a *check*.
</code_context>
<issue_to_address>
**issue (typo):** Consider adjusting the phrase "artifacts and link" for grammatical correctness.
Consider changing this to "artifacts and a link to a Currents report" or "artifacts and links to Currents reports" so the coordinated nouns are grammatically consistent.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds first-class group management to the image creation wizard. New React components GroupInfo and GroupRow render and manage groups. Redux state gains UserGroup type, deterministic ids, automatic GID assignment, selectors, and reducers (add/set/remove). Validation utilities introduce GroupsStepValidation, useUserGroupsValidation, a generic unique-name validator, and updated group name rules. Request mapping, on‑prem/hosted blueprint mappers, and export normalization (GID placeholder) were updated. Playwright tests and helpers include a verifyGroups helper and expanded group-related flows. The wizard step label changed to "Groups and users". Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/store/wizardSlice.ts (1)
1206-1218: Potential duplicate GID generation.The random GID generation doesn't check for uniqueness against existing groups. If multiple groups are added, there's a small chance of GID collision.
Consider checking for uniqueness:
addUserGroup: (state, action: PayloadAction<string>) => { const existingGroupIndex = state.userGroups.findIndex( (group) => group.name === action.payload, ); if (existingGroupIndex === -1) { - const randomGid = Math.floor(Math.random() * (65534 - 1000 + 1)) + 1000; + let randomGid: number; + const existingGids = new Set(state.userGroups.map(g => g.gid)); + do { + randomGid = Math.floor(Math.random() * (65534 - 1000 + 1)) + 1000; + } while (existingGids.has(randomGid)); const newUserGroup = { name: action.payload, gid: randomGid, }; state.userGroups.push(newUserGroup); } },src/Components/CreateImageWizard/steps/Users/index.tsx (1)
19-22: Consider adding descriptive content for the Groups section.The Users section includes a
<Content>component with helpful text (lines 14-17). For consistency and user clarity, consider adding similar guidance for the Groups section explaining its purpose.<Title headingLevel='h1' size='xl'> Groups </Title> + <Content> + Define groups before assigning users to them. Each group will be created on systems using this image. + </Content> <GroupInfo />src/Components/CreateImageWizard/steps/Users/components/GroupRow.tsx (1)
73-78: Refactor remove-and-add pattern to avoid state disruption.The current approach of removing and re-adding a group to assign a gid can cause:
- Loss of input focus, disrupting the user experience
- Unnecessary state churn and re-renders
- Potential edge cases with validation state
Consider either:
- Having the Redux action
updateUserGroupByIndexhandle gid assignment internally when a valid name is set- Using a dedicated action like
assignGidToUserGroup(index)that updates the group in placeThis would maintain better UX and cleaner state management.
To confirm if this pattern causes focus loss, test the following scenario:
- Type a valid group name (e.g., "developers")
- Observe if the input loses focus when the name becomes valid
If focus is lost, this refactor should be prioritized to essential.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
README.md(1 hunks)playwright/Customizations/Users.spec.ts(4 hunks)src/Components/CreateImageWizard/steps/Packages/Packages.tsx(3 hunks)src/Components/CreateImageWizard/steps/Users/components/GroupInfo.tsx(1 hunks)src/Components/CreateImageWizard/steps/Users/components/GroupRow.tsx(1 hunks)src/Components/CreateImageWizard/steps/Users/components/UserRow.tsx(2 hunks)src/Components/CreateImageWizard/steps/Users/index.tsx(2 hunks)src/Components/CreateImageWizard/utilities/requestMapper.ts(4 hunks)src/Components/CreateImageWizard/utilities/useValidation.tsx(5 hunks)src/Components/CreateImageWizard/validators.ts(1 hunks)src/store/wizardSlice.ts(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
src/Components/CreateImageWizard/steps/Users/components/GroupRow.tsx (1)
src/store/hooks.ts (1)
useAppDispatch(6-6)
src/Components/CreateImageWizard/steps/Users/components/GroupInfo.tsx (3)
src/store/hooks.ts (1)
useAppSelector(7-7)src/store/wizardSlice.ts (1)
selectUserGroups(526-528)src/Components/CreateImageWizard/utilities/useValidation.tsx (1)
useUserGroupsValidation(1040-1087)
playwright/Customizations/Users.spec.ts (4)
playwright/helpers/navHelpers.ts (1)
fillInImageOutput(18-44)playwright/helpers/wizardHelpers.ts (7)
registerLater(56-61)fillInDetails(37-49)createBlueprint(16-27)exportBlueprint(117-136)verifyExportedBlueprint(143-149)importBlueprint(157-170)fillInImageOutputGuest(67-70)playwright/helpers/helpers.ts (1)
isHosted(22-24)playwright/fixtures/data/exportBlueprintContents.ts (1)
exportedUsersBP(142-174)
src/Components/CreateImageWizard/utilities/useValidation.tsx (3)
src/store/wizardSlice.ts (2)
UserGroup(90-93)selectUserGroups(526-528)src/Components/CreateImageWizard/validators.ts (1)
isUserGroupValid(99-107)src/store/hooks.ts (1)
useAppSelector(7-7)
src/Components/CreateImageWizard/utilities/requestMapper.ts (2)
src/store/index.ts (1)
RootState(156-156)src/store/wizardSlice.ts (1)
selectUserGroups(526-528)
src/store/wizardSlice.ts (2)
src/store/index.ts (1)
RootState(156-156)src/Components/CreateImageWizard/steps/Packages/Packages.tsx (1)
GroupWithRepositoryInfo(126-131)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: testing-farm:fedora-41-x86_64:self
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: Sourcery review
- GitHub Check: Red Hat Konflux / image-builder-frontend-on-pull-request
🔇 Additional comments (22)
playwright/Customizations/Users.spec.ts (3)
359-395: Good test coverage for the new Groups functionality.The new test steps for group creation and validation provide comprehensive coverage of the Groups section. The use of
delay: 50in thetypecalls helps ensure reliable input handling in the E2E tests.
397-440: Thorough validation error testing.Tests cover key validation scenarios including invalid group names (
@invalid), duplicate group names, and proper cleanup. The pattern of verifying error states before and after invalid input is well-structured.
474-493: Keyboard navigation test for groups.Good accessibility testing by verifying Tab navigation through group name, group ID, and remove button fields.
src/Components/CreateImageWizard/steps/Users/components/UserRow.tsx (1)
22-23: Action renames improve semantic clarity.The renamed actions (
assignGroupToUserByIndex,removeGroupFromUserByIndex) better convey their purpose — managing group assignments to specific users — distinguishing them from the new standalone user group management actions.Also applies to: 201-206
src/store/wizardSlice.ts (3)
90-93: Well-defined UserGroup type.The type structure with required
nameand optionalgidis appropriate for the use case.
1220-1233: Good defensive checks in group update/remove reducers.The index bounds checking in
updateUserGroupByIndexandremoveUserGroupByIndexprevents potential runtime errors from invalid indices.
1562-1566: No issues found. TheremoveUserGroupByIndexreducer is properly defined at line 1228 and correctly exported at line 1612 as part of thewizardSlice.actionsdestructuring, alongside related user group operations.README.md (1)
382-382: Minor formatting improvement.Removing the list marker for a single-item section improves readability.
src/Components/CreateImageWizard/validators.ts (1)
102-102: LGTM! Minimum length constraint added.The addition of the minimum length constraint (2 characters) for group names is consistent with similar validators in the codebase and aligns with the new user group validation requirements introduced in this PR.
src/Components/CreateImageWizard/steps/Users/components/GroupInfo.tsx (1)
1-32: LGTM! Clean implementation of group management UI.The component correctly handles the placeholder scenario (rendering an empty group when no groups exist) and properly integrates with the validation hook. The props passed to GroupRow include all necessary data for per-row validation and interaction.
src/Components/CreateImageWizard/steps/Packages/Packages.tsx (3)
83-87: LGTM! Action imports updated to distinguish package groups from user groups.The renaming from
addGroup/removeGrouptoaddPackageGroup/removePackageGroupcorrectly differentiates package group operations from the newly introduced user group operations, preventing naming collisions and improving code clarity.
948-951: LGTM! Dispatch calls updated consistently.The action dispatches are correctly updated to use the renamed
addPackageGroupandremovePackageGroupactions, maintaining consistency with the import changes.
1031-1031: LGTM! Final dispatch call updated.The dispatch call in the confirmation modal toggle handler is correctly updated to use
addPackageGroup.src/Components/CreateImageWizard/utilities/requestMapper.ts (4)
116-116: LGTM! Import added for user groups selector.The import of
selectUserGroupsis correctly added to support the new user group mapping functionality.
491-495: LGTM! User groups mapped from request to state.The mapping correctly extracts group name and optional gid from the request customizations, maintaining the data structure for user groups in the wizard state.
804-804: LGTM! User groups included in customizations.The addition of
groups: getUserGroups(state)correctly integrates user groups into the blueprint customizations payload.
979-988: LGTM! Helper function correctly normalizes user groups.The
getUserGroupshelper appropriately filters out groups with empty names, preserves optional gid values, and returns undefined when no valid groups exist, ensuring clean payload generation.src/Components/CreateImageWizard/utilities/useValidation.tsx (5)
45-47: LGTM! Imports added for user groups validation.The imports of
selectUserGroupsandUserGrouptype are correctly added to support the new user group validation functionality.
91-97: LGTM! Type definition for group validation.The
GroupsStepValidationtype appropriately extends the validation pattern used elsewhere in the codebase, with the addition ofhasGroupWithoutNameflag for specific group validation scenarios.
112-127: LGTM! User groups validation integrated into blueprint validation.The integration correctly includes user groups validation in the overall blueprint validity check, ensuring the wizard prevents progression when group validation fails.
1020-1038: LGTM! Helper function validates group names correctly.The
validateUserGroupNamefunction properly checks for:
- Required value
- Valid group name format (using
isUserGroupValid)- Duplicate group names (excluding the current index)
The implementation follows the same pattern as
validateUserNameand provides clear error messages.
1040-1087: LGTM! User groups validation hook implemented correctly.The
useUserGroupsValidationhook:
- Returns early with no errors when no groups exist
- Validates each group name using the helper function
- Correctly skips the "Required value" error for the last (empty placeholder) group
- Aggregates errors and determines if navigation should be disabled
- Returns appropriate flags for UI state management
The logic aligns well with the user group management flow introduced in this PR.
mgold1234
force-pushed
the
add_group_section
branch
2 times, most recently
from
96397f6 to
421399b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/store/wizardSlice.ts (1)
1208-1236: Potential infinite loop in GID generation.The
do...whileloop generating a unique random GID will loop indefinitely if all 64,535 possible GIDs (1000-65534) are already in use. While extremely unlikely in practice, consider adding a maximum iteration count as a safeguard.if (isUserGroupValid(action.payload.trim())) { let randomGid: number; const existingGids = new Set( state.userGroups .map((g) => g.gid) .filter((gid) => gid !== undefined), ); + let attempts = 0; + const maxAttempts = 1000; do { randomGid = Math.floor(Math.random() * (65534 - 1000 + 1)) + 1000; - } while (existingGids.has(randomGid)); + attempts++; + } while (existingGids.has(randomGid) && attempts < maxAttempts); state.userGroups.push({ ...baseGroup, gid: randomGid });src/Components/CreateImageWizard/steps/Users/components/GroupInfo.tsx (1)
24-24: Unnecessary variable assignment.
groupsToRenderis just an alias forgroupswith no transformation. Consider usinggroupsdirectly in the render.- const groupsToRender = groups; - return ( <> - {groupsToRender.map((group, index) => ( + {groups.map((group, index) => (
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
playwright/Customizations/Users.spec.ts(5 hunks)src/Components/CreateImageWizard/steps/Users/components/GroupInfo.tsx(1 hunks)src/Components/CreateImageWizard/steps/Users/components/GroupRow.tsx(1 hunks)src/Components/CreateImageWizard/steps/Users/index.tsx(2 hunks)src/Components/CreateImageWizard/utilities/requestMapper.ts(4 hunks)src/store/wizardSlice.ts(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Components/CreateImageWizard/steps/Users/index.tsx
🧰 Additional context used
🧬 Code graph analysis (5)
playwright/Customizations/Users.spec.ts (1)
playwright/helpers/wizardHelpers.ts (2)
fillInDetails(37-49)createBlueprint(16-27)
src/store/wizardSlice.ts (3)
src/store/index.ts (1)
RootState(156-156)src/Components/CreateImageWizard/steps/Packages/Packages.tsx (1)
GroupWithRepositoryInfo(126-131)src/Components/CreateImageWizard/validators.ts (1)
isUserGroupValid(99-107)
src/Components/CreateImageWizard/utilities/requestMapper.ts (2)
src/store/index.ts (1)
RootState(156-156)src/store/wizardSlice.ts (1)
selectUserGroups(528-530)
src/Components/CreateImageWizard/steps/Users/components/GroupRow.tsx (4)
src/store/hooks.ts (1)
useAppDispatch(6-6)src/Components/CreateImageWizard/utilities/useValidation.tsx (1)
useUserGroupsValidation(1040-1087)src/Components/CreateImageWizard/validators.ts (1)
isUserGroupValid(99-107)src/Components/CreateImageWizard/ValidatedInput.tsx (1)
ValidatedInputAndTextArea(47-116)
src/Components/CreateImageWizard/steps/Users/components/GroupInfo.tsx (3)
src/store/hooks.ts (2)
useAppDispatch(6-6)useAppSelector(7-7)src/store/wizardSlice.ts (1)
selectUserGroups(528-530)src/Components/CreateImageWizard/utilities/useValidation.tsx (1)
useUserGroupsValidation(1040-1087)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: testing-farm:fedora-41-x86_64:self
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: Sourcery review
- GitHub Check: Red Hat Konflux / image-builder-frontend-on-pull-request
🔇 Additional comments (21)
playwright/Customizations/Users.spec.ts (7)
359-395: Comprehensive test coverage for Groups creation.The test step properly verifies group creation flow including:
- Initial group input visibility and enableability
- Name input and validation state
- Auto-generated ID placeholder
- Adding a second group and verifying both values persist
397-440: Good validation error test coverage.The test step correctly validates:
- Invalid group name format (with
@)- Duplicate group name detection
- Add button disabled state during errors
- Group removal and count verification
442-472: Group add/remove functionality tests are well structured.Tests verify:
- Remove button disabled when only one group remains
- Adding new groups after removal works correctly
- Remove buttons enabled when multiple groups exist
474-493: Keyboard navigation test ensures accessibility compliance.The test verifies proper Tab order through group name → group ID → remove button.
500-537: Edit blueprint flow now includes group modifications.The test step properly:
- Scrolls to Groups section
- Modifies existing group name
- Adds new groups
- Proceeds to save
562-579: Group persistence verification is thorough.Uses
Promise.allwithArray.frompattern to collect all group values, then verifies expected groups are present. ThetoContainassertions are appropriate for checking presence without relying on order.
606-641: Import/export verification includes groups.Properly verifies groups survive the export/import cycle by checking the same group names are present after import.
src/store/wizardSlice.ts (4)
91-95: Well-defined UserGroup type.The type correctly includes:
id: string for unique identificationname: string for group namegid?: optional number for group ID
1243-1271: updateUserGroupById correctly handles GID generation on name validation.The reducer properly:
- Updates the name
- Generates a GID only if the name is valid and no GID exists yet
- Preserves existing GID if one is already set
Same minor concern about the
do...whileloop as noted above.
1508-1524: assignGroupToUserByIndex properly sets administrator flag.The logic correctly sets
isAdministrator: truewhen adding the 'wheel' group to a user, maintaining consistency with the existing wheel group behavior.
1525-1538: removeGroupFromUserByIndex correctly clears administrator flag.Symmetrical with
assignGroupToUserByIndex- properly clearsisAdministratorwhen removing the 'wheel' group.src/Components/CreateImageWizard/utilities/requestMapper.ts (3)
491-497: Correct mapping of API groups to internal UserGroup format.The mapping properly:
- Generates a new UUID for each group (allowing internal tracking)
- Preserves name and conditionally spreads gid only when defined
- Defaults to empty array if no groups exist
980-989: getUserGroups helper correctly filters and maps userGroups for API request.The function:
- Filters out groups with empty names
- Conditionally includes gid only when defined
- Returns undefined when no valid groups exist (avoiding empty array in API payload)
805-805: Groups field correctly populated in getCustomizations.Using
getUserGroups(state)properly populates the groups field in the Customizations object, which is distinct from package groups (handled ingetPackages).src/Components/CreateImageWizard/steps/Users/components/GroupInfo.tsx (2)
17-22: Auto-initialization of empty group ensures UI never shows empty state.The useEffect correctly ensures at least one group row is always present for the user to fill in. The dependency on
groups.lengthprevents unnecessary dispatches.
28-37: Props are correctly passed to GroupRow.The conditional spread for
groupGidensures the prop is only passed when defined, avoiding passingundefinedexplicitly.src/Components/CreateImageWizard/steps/Users/components/GroupRow.tsx (5)
43-50: getValidationByIndex correctly extracts per-row validation.The helper properly checks if the index exists in errors and returns an appropriate shape for
ValidatedInputAndTextArea.
60-67: handleGroupNameChange correctly updates group in place.Dispatching
updateUserGroupByIdwith the id and new name allows the store to handle GID generation when the name becomes valid, keeping the component simple.
86-96: Add button placement and logic are correct.The button is conditionally rendered only on the last row when the name is non-empty. It's disabled when:
- The current name is invalid
- There are other validation errors (
isAddButtonDisabled)This prevents adding new groups while existing ones have errors.
112-122: Remove button correctly prevents orphaning the Groups section.The
isDisabled={groupCount <= 1}ensures at least one group row always remains, maintaining UI consistency with the initialization logic in GroupInfo.
101-106: Group ID display handles empty state correctly.The value is only shown when both
hasNameandgroupGidare truthy, otherwise the "Auto-generated" placeholder is shown via the empty string value.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Components/CreateImageWizard/utilities/useValidation.tsx (1)
1040-1087: Consider refactoring the hasGroupWithoutName detection.The validation logic correctly handles the empty placeholder group (last group can be empty without error). However, the
hasGroupWithoutNamedetection on lines 1078-1080 relies on matching the exact error message text "Required value". If this error message text changes in the future, the logic will break silently.Consider refactoring to check the condition directly rather than matching error message text:
- const hasGroupWithoutName = Object.values(errors).some( - (groupErrors) => groupErrors.groupName === 'Required value', - ); + const hasGroupWithoutName = userGroups.some( + (group, index) => !group.name && index !== userGroups.length - 1 + );This makes the intent clearer and removes the dependency on specific error message text.
src/store/wizardSlice.ts (1)
1208-1236: GID generation is correct but duplicated.The
addUserGroupaction correctly generates a unique GID in the range 1000-65534 when the name is immediately valid. However, this GID generation logic is duplicated inupdateUserGroupById(lines 1257-1269).Consider extracting the GID generation into a helper function to eliminate duplication:
const generateUniqueGid = (existingGroups: UserGroup[]): number => { const existingGids = new Set( existingGroups.map((g) => g.gid).filter((gid) => gid !== undefined) ); let randomGid: number; do { randomGid = Math.floor(Math.random() * (65534 - 1000 + 1)) + 1000; } while (existingGids.has(randomGid)); return randomGid; };Then use it in both actions:
state.userGroups.push({ ...baseGroup, gid: generateUniqueGid(state.userGroups) });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
playwright/Customizations/Users.spec.ts(5 hunks)src/Components/CreateImageWizard/steps/Packages/Packages.tsx(3 hunks)src/Components/CreateImageWizard/steps/Users/components/GroupInfo.tsx(1 hunks)src/Components/CreateImageWizard/steps/Users/components/GroupRow.tsx(1 hunks)src/Components/CreateImageWizard/steps/Users/components/UserRow.tsx(2 hunks)src/Components/CreateImageWizard/steps/Users/index.tsx(2 hunks)src/Components/CreateImageWizard/utilities/requestMapper.ts(4 hunks)src/Components/CreateImageWizard/utilities/useValidation.tsx(5 hunks)src/Components/CreateImageWizard/validators.ts(1 hunks)src/store/wizardSlice.ts(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Components/CreateImageWizard/validators.ts
- src/Components/CreateImageWizard/utilities/requestMapper.ts
- src/Components/CreateImageWizard/steps/Users/components/UserRow.tsx
- src/Components/CreateImageWizard/steps/Users/components/GroupInfo.tsx
🧰 Additional context used
🧬 Code graph analysis (4)
src/Components/CreateImageWizard/utilities/useValidation.tsx (3)
src/store/wizardSlice.ts (2)
UserGroup(91-95)selectUserGroups(528-530)src/Components/CreateImageWizard/validators.ts (1)
isUserGroupValid(99-107)src/store/hooks.ts (1)
useAppSelector(7-7)
src/Components/CreateImageWizard/steps/Users/components/GroupRow.tsx (4)
src/store/hooks.ts (1)
useAppDispatch(6-6)src/Components/CreateImageWizard/utilities/useValidation.tsx (1)
useUserGroupsValidation(1040-1087)src/Components/CreateImageWizard/validators.ts (1)
isUserGroupValid(99-107)src/Components/CreateImageWizard/ValidatedInput.tsx (1)
ValidatedInputAndTextArea(47-116)
playwright/Customizations/Users.spec.ts (1)
playwright/helpers/wizardHelpers.ts (2)
fillInDetails(37-49)createBlueprint(16-27)
src/store/wizardSlice.ts (3)
src/store/index.ts (1)
RootState(156-156)src/Components/CreateImageWizard/steps/Packages/Packages.tsx (1)
GroupWithRepositoryInfo(126-131)src/Components/CreateImageWizard/validators.ts (1)
isUserGroupValid(99-107)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (22)
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: testing-farm:fedora-41-x86_64:self
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: Sourcery review
- GitHub Check: Red Hat Konflux / image-builder-frontend-on-pull-request
- GitHub Check: Service Tests
- GitHub Check: playwright-tests
🔇 Additional comments (15)
src/Components/CreateImageWizard/steps/Users/index.tsx (1)
5-5: LGTM!The Groups section is cleanly integrated into the Users step, following the same pattern as the existing Users section. The descriptive text clearly explains the purpose of defining groups before assigning users to them.
Also applies to: 19-26
src/Components/CreateImageWizard/utilities/useValidation.tsx (3)
45-45: LGTM!The GroupsStepValidation type is well-defined and includes the
hasGroupWithoutNamefield which provides useful state for the UI to disable the Add button appropriately.Also applies to: 47-47, 91-97
112-112: LGTM!The user groups validation is correctly integrated into the overall blueprint validation flow, following the same pattern as existing validation checks.
Also applies to: 127-127
1020-1038: LGTM!The validation logic correctly enforces group name requirements: non-empty, valid format per
isUserGroupValid, and uniqueness within the list. The duplicate check properly excludes the current index.src/Components/CreateImageWizard/steps/Packages/Packages.tsx (1)
83-83: LGTM!The renaming from
addGroup/removeGrouptoaddPackageGroup/removePackageGroupclearly distinguishes package groups from the newly introduced user groups. This improves code clarity and maintainability.Also applies to: 87-87, 948-948, 951-951, 1031-1031
playwright/Customizations/Users.spec.ts (3)
359-494: LGTM!The test coverage for Groups is comprehensive, including:
- Creating valid groups
- Validation errors (invalid names, duplicates)
- Add/remove functionality
- Keyboard navigation for accessibility
The tests follow good practices with clear step descriptions and proper assertions.
500-584: LGTM!The tests properly verify that groups persist through the full edit → save → reload workflow. The verification checks both the presence of groups and their correct values, ensuring data integrity.
606-643: LGTM!The import/export verification ensures that groups are properly persisted in the blueprint export format and correctly reconstructed on import. This is critical for blueprint portability.
src/Components/CreateImageWizard/steps/Users/components/GroupRow.tsx (3)
23-50: LGTM!The component props are well-defined and the
getValidationByIndexhelper correctly extracts per-index validation errors from the step validation. This provides proper isolation of validation state for each group row.
52-72: LGTM!The event handlers properly delegate to Redux actions without managing local state. The comment on line 65 accurately describes that GID generation happens in the store, maintaining good separation of concerns.
74-124: LGTM!The render logic correctly implements the group row UI:
- Group name input with integrated validation feedback
- Add button appears only on the last row when there's a name, disabled for invalid names
- Group ID field is read-only and shows "Auto-generated" placeholder
- Remove button is disabled when there's only one group, preventing removal of all groups
The UX is intuitive and prevents invalid states.
src/store/wizardSlice.ts (4)
40-40: LGTM!The
UserGrouptype is well-defined with an optionalgidfield, allowing groups to exist without assigned IDs during creation. The state initialization and selector follow established patterns in the slice.Also applies to: 91-95, 160-160, 317-317, 528-530
1183-1207: LGTM!The renaming from
addGroup/removeGrouptoaddPackageGroup/removePackageGroupclearly distinguishes package groups from user groups, improving code maintainability.
1237-1242: LGTM!The
removeUserGroupByIndexandremoveUserGroupByIdactions correctly remove groups from the state. Having both index-based and ID-based removal provides flexibility for different use cases.Also applies to: 1272-1279
1508-1538: LGTM!The renamed
assignGroupToUserByIndexandremoveGroupFromUserByIndexactions correctly manage user group assignments. The special handling for the "wheel" group to set/clear administrator status is properly implemented.
mgold1234
force-pushed
the
add_group_section
branch
from
421399b to
51c55df
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/store/wizardSlice.ts (2)
46-73: Consider edge case when all GIDs are exhausted.The
generateUniqueGidfunction has a theoretical edge case: if all 64,535 GIDs (1000-65534) are taken, the fallback sequential search will not find an available GID andrandomGidwill retain its last random value (which was already in use).In practice, this is extremely unlikely given the limited number of groups expected, but for defensive coding, consider adding handling:
🔎 Optional: Add explicit exhaustion handling
// Fallback to sequential search if random generation failed if (attempts >= maxAttempts) { for (let gid = 1000; gid <= 65534; gid++) { if (!existingGids.has(gid)) { - randomGid = gid; - break; + return gid; } } + // All GIDs exhausted - this should never happen in practice + throw new Error('Unable to generate unique GID: all GIDs in range 1000-65534 are in use'); } return randomGid;
1237-1256: Minor inconsistency: Usingcrypto.randomUUID()whileuuidv4is imported elsewhere.The codebase imports
uuidv4(line 2) and uses it in other places (e.g., line 788). Usingcrypto.randomUUID()here works fine but introduces a minor inconsistency. Consider using the already-importeduuidv4()for consistency.🔎 Optional: Use uuidv4 for consistency
if (existingGroupIndex === -1) { const baseGroup = { - id: crypto.randomUUID(), + id: uuidv4(), name: action.payload, };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
playwright/Customizations/Users.spec.ts(5 hunks)src/Components/CreateImageWizard/steps/Users/components/GroupInfo.tsx(1 hunks)src/Components/CreateImageWizard/steps/Users/components/GroupRow.tsx(1 hunks)src/Components/CreateImageWizard/steps/Users/index.tsx(2 hunks)src/Components/CreateImageWizard/utilities/requestMapper.ts(4 hunks)src/Components/CreateImageWizard/utilities/useValidation.tsx(5 hunks)src/Components/CreateImageWizard/validators.ts(1 hunks)src/store/wizardSlice.ts(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/Components/CreateImageWizard/steps/Users/index.tsx
- src/Components/CreateImageWizard/steps/Users/components/GroupInfo.tsx
- src/Components/CreateImageWizard/validators.ts
- src/Components/CreateImageWizard/steps/Users/components/GroupRow.tsx
- src/Components/CreateImageWizard/utilities/requestMapper.ts
🧰 Additional context used
🧬 Code graph analysis (2)
playwright/Customizations/Users.spec.ts (1)
playwright/helpers/wizardHelpers.ts (2)
fillInDetails(37-49)createBlueprint(16-27)
src/Components/CreateImageWizard/utilities/useValidation.tsx (3)
src/store/wizardSlice.ts (2)
UserGroup(120-124)selectUserGroups(557-559)src/Components/CreateImageWizard/validators.ts (1)
isUserGroupValid(99-107)src/store/hooks.ts (1)
useAppSelector(7-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: testing-farm:fedora-41-x86_64:self
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: ESLint
- GitHub Check: playwright-tests
- GitHub Check: Service Tests
- GitHub Check: Sourcery review
- GitHub Check: Red Hat Konflux / image-builder-frontend-on-pull-request
🔇 Additional comments (18)
src/Components/CreateImageWizard/utilities/useValidation.tsx (4)
92-98: LGTM! Type definition is well-structured.The
GroupsStepValidationtype appropriately extends the validation pattern used for users, with the addition ofhasGroupWithoutNameto support the placeholder group UX (allowing an empty last group without error).
113-128: LGTM! Group validation correctly integrated into blueprint validity.The integration follows the established pattern, ensuring group validation errors will block blueprint creation/save.
1022-1040: LGTM! Validation helper follows established patterns.The
validateUserGroupNamefunction correctly validates emptiness, format (viaisUserGroupValid), and uniqueness with proper index exclusion for self-comparison.
1054-1088: LGTM! Validation logic handles placeholder group UX correctly.The hook properly:
- Allows the last group to be empty without showing "Required value" error (placeholder behavior)
- Still validates non-empty names for format and uniqueness
- Correctly computes
hasGroupWithoutNameto catch intermediate empty groups- Gates
disabledNexton both empty groups and validation errorsplaywright/Customizations/Users.spec.ts (7)
359-395: LGTM! Comprehensive test for initial group creation.Good coverage including:
- Input visibility and enabling
- Value persistence
- Auto-generated ID field visibility
- Helper text visibility
- Add button functionality
397-440: LGTM! Good validation error coverage.Tests appropriately cover:
- Invalid group name format triggering error
- Duplicate group name detection
- Add button disabled state when errors exist
- Proper cleanup after error testing
442-472: LGTM! Thorough add/remove functionality testing.Good coverage of edge cases including the single-group remove button disabled state.
474-493: LGTM! Keyboard navigation testing.Appropriate accessibility testing for tab order through group fields.
522-531: LGTM! Edit workflow extended for groups.Appropriately tests group modification during blueprint editing, including adding new groups.
562-579: LGTM! Proper verification of group persistence after save.The pattern using
Promise.allwithArray.fromfor collecting input values is appropriate for async Playwright operations.
624-640: LGTM! Import verification includes groups.Good end-to-end testing ensuring groups persist through the export/import cycle.
src/store/wizardSlice.ts (7)
120-124: LGTM! Clean type definition.The optional
gidfield appropriately supports deferred GID generation until the group name is validated.
189-189: LGTM! State properly extended for user groups.
557-559: LGTM! Standard selector pattern.
1257-1262: LGTM! Proper bounds checking before removal.
1263-1284: LGTM! GID generation logic is well-designed.The update logic appropriately:
- Allows explicit GID override via payload
- Auto-generates GID only when name first becomes valid
- Preserves existing GID even if name is later edited (prevents accidental loss on typos)
1285-1292: LGTM! Standard removal pattern.
1623-1625: LGTM! Actions properly exported.
mgold1234
force-pushed
the
add_group_section
branch
from
51c55df to
25ff2f6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
playwright/Customizations/Users.spec.ts(5 hunks)src/store/wizardSlice.ts(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/store/wizardSlice.ts (2)
src/store/index.ts (1)
RootState(156-156)src/Components/CreateImageWizard/validators.ts (1)
isUserGroupValid(99-107)
playwright/Customizations/Users.spec.ts (1)
playwright/helpers/wizardHelpers.ts (2)
fillInDetails(37-49)createBlueprint(16-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (22)
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: testing-farm:fedora-41-x86_64:self
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: Service Tests
- GitHub Check: Sourcery review
- GitHub Check: Red Hat Konflux / image-builder-frontend-on-pull-request
- GitHub Check: playwright-tests
🔇 Additional comments (12)
playwright/Customizations/Users.spec.ts (7)
359-395: LGTM! Comprehensive group creation testing.The test step thoroughly validates the initial group creation flow, including:
- Input visibility and enablement
- Group name entry with typing delay for realism
- Validation state verification
- Auto-generated GID field presence
- Add button state checks
- Multiple group creation
397-440: LGTM! Thorough validation error coverage.The test properly exercises group validation scenarios:
- Invalid group name format (@invalid)
- Duplicate group name detection
- Add button disabled state during validation errors
- Group removal and state verification
442-472: LGTM! Well-designed add/remove functionality tests.The test validates important UX behaviors:
- Proper removal of groups
- Last Remove button disabled when only one group remains (prevents empty state)
- Ability to add new groups after removal
- Both Remove buttons enabled when multiple groups exist
474-493: LGTM! Good accessibility coverage with keyboard navigation.The test validates the expected tab order through group fields: name → ID → remove button. This ensures keyboard-only users can navigate the Groups section effectively.
500-537: LGTM! Groups integrated into edit blueprint flow.The test properly extends the edit blueprint scenario to include group modifications alongside user changes, verifying that groups can be added and persisted during blueprint editing.
562-579: LGTM! Comprehensive save verification.The test uses
expect(groupCount).toBeGreaterThanOrEqual(2)and then checks that specific groups (developers, managers, operators) are present. This flexible approach accounts for potential default or system groups while ensuring the user-created groups persist.
606-641: LGTM! End-to-end persistence validation.The test verifies that groups survive the complete export/import cycle, confirming the serialization and deserialization logic works correctly for the new Groups feature.
src/store/wizardSlice.ts (5)
120-124: LGTM! Well-defined UserGroup type.The type definition is clean and appropriate, with
gidcorrectly marked as optional since it may not be present until the group name is validated.
1237-1256: LGTM! Proper conditional GID generation.The action correctly generates a GID only when the group name is immediately valid, preventing invalid groups from receiving GIDs. The duplicate check by name prevents adding the same group twice.
1263-1284: LGTM! Smart GID assignment logic.The update logic properly handles three scenarios:
- GID explicitly provided → use it
- No GID provided, name valid, group lacks GID → generate one
- Otherwise → no GID assignment
This allows groups to receive GIDs when their names become valid during editing.
1257-1262: LGTM! Standard removal implementations.Both removal actions (by index and by ID) include proper bounds/existence checks and use standard array mutation patterns.
Also applies to: 1285-1292
189-189: LGTM! Complete Redux integration.The userGroups state is properly:
- Declared in the wizardState type
- Initialized in initialState
- Exported via selector
- Exposed through exported actions
Also applies to: 346-346, 557-559, 1623-1625, 1672-1672
|
Quick observations:
Also a group can be assigned to a user without it being created first, would we be able to at least partially safeguard that or maybe add some floating info like "You've assigned user to a custom group that is not defined in 'Groups', make sure it will exist on the system"? I assume there would be a default set of system groups that will work just fine, groups that will be created with a package installation and groups that need to be specified in "Groups" in order for them to work. @achilleas-k would we be able to formulate at least a list for the first case of "system defined groups" based on distro/target/... or not really? |
mgold1234
force-pushed
the
add_group_section
branch
from
25ff2f6 to
f0b5cc5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/store/wizardSlice.ts (1)
46-78: GID exhaustion edge case now handled correctly.The addition of the exhaustion check (lines 71-74) addresses the critical issue flagged in the previous review. The function now correctly throws an error when all GIDs in the range are exhausted, preventing the return of an invalid GID.
Optional: Cleaner implementation available
While the current code works correctly, the previous review suggested a slightly cleaner approach with early returns that eliminates the need for the
attempts >= maxAttemptsconditional:// Try random generation first do { randomGid = Math.floor(Math.random() * (65534 - 1000 + 1)) + 1000; attempts++; } while (existingGids.has(randomGid) && attempts < maxAttempts); // Return immediately if random generation succeeded if (!existingGids.has(randomGid)) { return randomGid; } // Fallback to sequential search for (let gid = 1000; gid <= 65534; gid++) { if (!existingGids.has(gid)) { return gid; } } // All GIDs exhausted throw new Error('All GIDs in range 1000-65534 are exhausted');This is purely a readability improvement - the current implementation is functionally correct.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
playwright/Customizations/Users.spec.ts(5 hunks)src/Components/CreateImageWizard/CreateImageWizard.tsx(1 hunks)src/store/wizardSlice.ts(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
playwright/Customizations/Users.spec.ts (1)
playwright/helpers/wizardHelpers.ts (2)
fillInDetails(37-49)createBlueprint(16-27)
src/store/wizardSlice.ts (2)
src/store/index.ts (1)
RootState(156-156)src/Components/CreateImageWizard/validators.ts (1)
isUserGroupValid(99-107)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (22)
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: testing-farm:fedora-41-x86_64:self
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: Sourcery review
- GitHub Check: Red Hat Konflux / image-builder-frontend-on-pull-request
- GitHub Check: Service Tests
- GitHub Check: playwright-tests
🔇 Additional comments (4)
src/store/wizardSlice.ts (2)
125-129: LGTM! Clean Redux state management.The user groups integration follows proper Redux patterns:
- Type definition is clear and minimal (lines 125-129)
- State properly initialized in both type and initialState (lines 194, 351)
- Selector follows naming convention (lines 562-564)
- Actions include proper validation and duplicate checking (line 1243-1245, 1254)
- Removal actions include appropriate bounds checking (lines 1264-1266, 1291-1293)
- All new actions properly exported (lines 1628-1631, 1677)
The implementation integrates cleanly with the existing wizard state structure.
Also applies to: 194-194, 351-351, 562-564, 1242-1267, 1290-1297, 1628-1631, 1677-1677
1268-1289: The validation layer properly prevents saving groups with invalid names. TheuseUserGroupsValidation()hook validates all group names viavalidateUserGroupName(), which returns an error for invalid names and setsdisabledNext: true. The blueprint submission check inuseIsBlueprintValid()includes!userGroups.disabledNext, preventing any blueprint save when group validation fails. While theupdateUserGroupByIdaction preserves the GID when a name is updated, users cannot submit a blueprint containing a group with an invalid name, so this behavior is safe.src/Components/CreateImageWizard/CreateImageWizard.tsx (1)
611-611: LGTM! Step label accurately reflects expanded functionality.The update from "Users" to "Users and groups" clearly communicates the step's purpose and aligns with the PR objectives.
Note: A reviewer previously mentioned that since groups are a prerequisite for user assignment, placing Groups before Users might be more intuitive. However, since both are within the same wizard step, the current ordering is reasonable and this is primarily a UX consideration for the step's internal layout rather than the step label itself.
playwright/Customizations/Users.spec.ts (1)
359-641: Excellent test coverage for Groups functionality.The test suite comprehensively covers the new Groups feature:
- Creation and validation (lines 359-440): Tests valid group creation, auto-generated GIDs, invalid names, duplicate detection, and validation error states
- UI interactions (lines 442-472): Verifies add/remove functionality and the business rule that at least one group must exist (line 459)
- Accessibility (lines 474-493): Tests keyboard navigation through group fields (name → GID → remove button), ensuring proper focus management
- Persistence (lines 500-641): Validates that groups persist correctly through edit, save, export, and import workflows
The tests follow the existing patterns from user tests and integrate well with the broader test suite. The coverage ensures both happy paths and error scenarios are validated.
it makes sense to put "Group" above the "Users", I changed it and will confirm that with Shayna. |
mgold1234
force-pushed
the
add_group_section
branch
from
f0b5cc5 to
26b148d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
playwright/Customizations/Users.spec.ts(5 hunks)src/Components/CreateImageWizard/CreateImageWizard.tsx(1 hunks)src/Components/CreateImageWizard/steps/Users/components/GroupInfo.tsx(1 hunks)src/Components/CreateImageWizard/steps/Users/components/GroupRow.tsx(1 hunks)src/Components/CreateImageWizard/steps/Users/components/UserRow.tsx(2 hunks)src/Components/CreateImageWizard/steps/Users/index.tsx(1 hunks)src/Components/CreateImageWizard/utilities/requestMapper.ts(4 hunks)src/Components/CreateImageWizard/utilities/useValidation.tsx(8 hunks)src/Components/CreateImageWizard/validators.ts(1 hunks)src/store/wizardSlice.ts(8 hunks)src/test/Components/CreateImageWizard/CreateImageWizard.test.tsx(1 hunks)src/test/Components/CreateImageWizard/steps/Packages/Packages.test.tsx(1 hunks)src/test/Components/CreateImageWizard/steps/Timezone/Timezone.test.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/test/Components/CreateImageWizard/steps/Packages/Packages.test.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- src/Components/CreateImageWizard/validators.ts
- src/Components/CreateImageWizard/CreateImageWizard.tsx
- src/Components/CreateImageWizard/utilities/requestMapper.ts
- src/Components/CreateImageWizard/steps/Users/components/GroupRow.tsx
- src/Components/CreateImageWizard/steps/Users/index.tsx
🧰 Additional context used
🧬 Code graph analysis (4)
src/test/Components/CreateImageWizard/steps/Timezone/Timezone.test.tsx (1)
src/test/Components/CreateImageWizard/wizardTestUtils.tsx (2)
renderCreateMode(98-103)clickBack(280-284)
src/Components/CreateImageWizard/utilities/useValidation.tsx (3)
src/store/hooks.ts (1)
useAppSelector(7-7)src/store/wizardSlice.ts (2)
selectUserGroups(562-564)UserGroup(125-129)src/Components/CreateImageWizard/validators.ts (1)
isUserGroupValid(100-108)
src/Components/CreateImageWizard/steps/Users/components/GroupInfo.tsx (3)
src/store/hooks.ts (2)
useAppDispatch(6-6)useAppSelector(7-7)src/store/wizardSlice.ts (1)
selectUserGroups(562-564)src/Components/CreateImageWizard/utilities/useValidation.tsx (1)
useUserGroupsValidation(1101-1148)
src/store/wizardSlice.ts (2)
src/store/index.ts (1)
RootState(156-156)src/Components/CreateImageWizard/validators.ts (1)
isUserGroupValid(100-108)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: Sourcery review
- GitHub Check: Service Tests
- GitHub Check: playwright-tests
- GitHub Check: Red Hat Konflux / image-builder-frontend-on-pull-request
🔇 Additional comments (20)
playwright/Customizations/Users.spec.ts (5)
359-396: LGTM: Well-structured groups creation test.The test properly validates initial group creation with appropriate waits and visibility checks. The typing delay helps prevent flakiness.
398-441: LGTM: Comprehensive validation test coverage.The test effectively covers invalid characters, duplicate names, and proper button state management during validation errors.
443-489: LGTM: Thorough functional and accessibility testing.The tests properly verify add/remove functionality and keyboard navigation. The comment at line 481 confirms that the GID field is disabled/read-only, which aligns with the PR objectives to prevent user editing of auto-generated GIDs.
496-534: LGTM: Edit workflow includes groups modifications.The test properly verifies that groups can be modified during blueprint editing and integrates well with the existing user modification tests.
559-640: LGTM: Comprehensive persistence verification.The tests effectively verify that groups persist correctly through save, export, and import operations, ensuring data integrity throughout the blueprint lifecycle.
src/test/Components/CreateImageWizard/steps/Timezone/Timezone.test.tsx (1)
99-104: LGTM: Test updated to reflect new step label.The test expectation correctly reflects the wizard step label change from "Users" to "Users and groups".
src/test/Components/CreateImageWizard/CreateImageWizard.test.tsx (1)
70-70: LGTM: Navigation test updated correctly.src/Components/CreateImageWizard/steps/Users/components/UserRow.tsx (1)
211-220: LGTM: Warning alert addresses reviewer concern.The inline warning alert for undefined groups provides a good user experience by informing users when they've assigned a user to a custom group that hasn't been defined in the Groups section. This addresses the data integrity concern raised in the PR review comments.
src/Components/CreateImageWizard/steps/Users/components/GroupInfo.tsx (2)
17-22: LGTM: Consistent pattern with Users section.The useEffect ensures at least one group row is always available, following the same UX pattern as the Users section. The empty string is properly validated by the groups validation logic.
24-38: LGTM: Clean rendering with proper optional prop handling.The conditional spreading of
groupGidonly when defined is a good pattern that prevents unnecessary prop passing and maintains type safety.src/Components/CreateImageWizard/utilities/useValidation.tsx (4)
93-99: LGTM: Well-structured validation type.The
GroupsStepValidationtype follows the established pattern and includes the necessary fields for tracking validation state.
899-912: LGTM: Non-blocking warning implementation is correct.The logic properly distinguishes between blocking errors and non-blocking warnings by filtering out
undefinedGroupswhen calculating whether the user can proceed. This provides helpful guidance without forcing users to define all groups upfront.
1081-1099: LGTM: Validation helper follows established patterns.The
validateUserGroupNamefunction correctly validates required values, format, and uniqueness, consistent with the existingvalidateUserNameimplementation.
1101-1148: LGTM: Validation hook with good UX considerations.The validation logic properly handles the edge case where the last (empty) group shouldn't show a "Required value" error immediately when added. This prevents jarring error messages while maintaining validation integrity.
src/store/wizardSlice.ts (6)
46-78: LGTM: GID exhaustion edge case properly handled.The function correctly implements random generation with sequential fallback, and the exhaustion check (lines 71-74) ensures that an invalid GID is never returned. This addresses the concern raised in previous review comments.
125-129: LGTM: Well-defined UserGroup type.The optional
gidfield properly reflects that GIDs are only generated when the group name is valid, preventing generation of GIDs for invalid placeholder groups.
1242-1261: LGTM: Smart conditional GID generation.The action correctly generates GIDs only for valid group names, avoiding wasted GID allocation for placeholder or invalid groups. The duplicate check prevents state inconsistency.
1262-1267: LGTM: Safe index-based removal.
1268-1289: LGTM: Smart GID management during updates.The action properly handles the scenario where a group name is corrected from invalid to valid by generating a GID at that point, while preserving existing GIDs for already-valid groups.
1290-1297: LGTM: Standard removal by ID.
mgold1234
force-pushed
the
add_group_section
branch
3 times, most recently
from
1bd99f9 to
8e30551
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/store/wizardSlice.ts (2)
1242-1261: Consider trimming group names before storage.The action validates the trimmed name for GID generation (line 1254) but stores the untrimmed name (line 1249). This could allow groups with leading/trailing whitespace to be stored, which may cause issues during system provisioning.
🔎 Proposed fix to trim group names before storage
addUserGroup: (state, action: PayloadAction<string>) => { + const trimmedName = action.payload.trim(); const existingGroupIndex = state.userGroups.findIndex( - (group) => group.name === action.payload, + (group) => group.name === trimmedName, ); if (existingGroupIndex === -1) { const baseGroup = { id: uuidv4(), - name: action.payload, + name: trimmedName, }; // Only generate GID if name is immediately valid - if (isUserGroupValid(action.payload.trim())) { + if (isUserGroupValid(trimmedName)) { const gid = generateUniqueGid(state.userGroups); state.userGroups.push({ ...baseGroup, gid }); } else { state.userGroups.push(baseGroup); } } },Apply the same trimming approach to
updateUserGroupByIdfor consistency.
1268-1289: Same trimming concern applies here.Similar to
addUserGroup, this action validates the trimmed name (line 1282) but stores the untrimmed value (line 1276). For consistency, apply the same trimming approach suggested in theaddUserGroupcomment.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
playwright/Customizations/Users.spec.tssrc/Components/CreateImageWizard/CreateImageWizard.tsxsrc/Components/CreateImageWizard/steps/Users/components/GroupInfo.tsxsrc/Components/CreateImageWizard/steps/Users/components/GroupRow.tsxsrc/Components/CreateImageWizard/steps/Users/components/UserRow.tsxsrc/Components/CreateImageWizard/steps/Users/index.tsxsrc/Components/CreateImageWizard/utilities/requestMapper.tssrc/Components/CreateImageWizard/utilities/useValidation.tsxsrc/Components/CreateImageWizard/validators.tssrc/store/wizardSlice.tssrc/test/Components/CreateImageWizard/CreateImageWizard.test.tsxsrc/test/Components/CreateImageWizard/steps/Packages/Packages.test.tsxsrc/test/Components/CreateImageWizard/steps/Timezone/Timezone.test.tsx
🚧 Files skipped from review as they are similar to previous changes (8)
- src/Components/CreateImageWizard/CreateImageWizard.tsx
- src/test/Components/CreateImageWizard/steps/Packages/Packages.test.tsx
- src/Components/CreateImageWizard/utilities/requestMapper.ts
- src/Components/CreateImageWizard/steps/Users/components/UserRow.tsx
- playwright/Customizations/Users.spec.ts
- src/Components/CreateImageWizard/steps/Users/components/GroupInfo.tsx
- src/Components/CreateImageWizard/utilities/useValidation.tsx
- src/Components/CreateImageWizard/validators.ts
🧰 Additional context used
🧬 Code graph analysis (3)
src/Components/CreateImageWizard/steps/Users/components/GroupRow.tsx (4)
src/store/hooks.ts (1)
useAppDispatch(6-6)src/Components/CreateImageWizard/utilities/useValidation.tsx (1)
useUserGroupsValidation(1100-1147)src/Components/CreateImageWizard/validators.ts (1)
isUserGroupValid(100-108)src/Components/CreateImageWizard/ValidatedInput.tsx (1)
ValidatedInputAndTextArea(49-124)
src/test/Components/CreateImageWizard/steps/Timezone/Timezone.test.tsx (1)
src/test/Components/CreateImageWizard/wizardTestUtils.tsx (2)
renderCreateMode(98-103)clickBack(280-284)
src/store/wizardSlice.ts (2)
src/store/index.ts (1)
RootState(156-156)src/Components/CreateImageWizard/validators.ts (1)
isUserGroupValid(100-108)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: Sourcery review
- GitHub Check: Red Hat Konflux / image-builder-frontend-on-pull-request
- GitHub Check: Service Tests
- GitHub Check: playwright-tests
🔇 Additional comments (17)
src/store/wizardSlice.ts (7)
46-78: LGTM! GID generation logic is sound.The function correctly handles both random generation and sequential fallback, with proper exhaustion detection. The past review concern about GID exhaustion has been properly addressed.
125-129: LGTM! Type definition is appropriate.The
UserGrouptype is well-defined withgidcorrectly marked as optional, supporting the validation-driven GID generation strategy.
194-194: LGTM! State initialization is correct.The
userGroupsarray is properly typed and initialized to an empty array in the initial state.Also applies to: 351-351
562-564: LGTM! Selector follows standard pattern.The selector is correctly implemented and consistent with other selectors in the file.
1262-1267: LGTM! Index-based removal with proper bounds checking.The action correctly validates the index before removal.
1290-1297: LGTM! ID-based removal is correct.The action properly finds and removes groups by ID.
1628-1630: LGTM! All user group actions are properly exported.The exports are complete and correctly placed.
Also applies to: 1677-1677
src/Components/CreateImageWizard/steps/Users/components/GroupRow.tsx (5)
1-30: LGTM! Imports and type definitions are well-organized.The component props are clearly defined and all necessary dependencies are imported.
32-50: LGTM! Hook usage and validation helper are correctly implemented.The
getValidationByIndexhelper appropriately isolates per-row validation while maintaining the globaldisabledNextstate.
52-67: LGTM! Event handlers are correctly implemented.The handlers properly delegate to Redux actions, and the comment accurately describes the GID generation behavior.
69-72: LGTM! Derived state calculations are correct.The computed values appropriately support the conditional rendering logic.
74-124: LGTM! Render logic implements good UX patterns.The conditional rendering is well-thought-out:
- Add button only appears on the last row when there's a name
- Add button is disabled until the name is valid
- GID displays only when generated (after name becomes valid)
- Remove button prevents deletion of the last group
src/test/Components/CreateImageWizard/CreateImageWizard.test.tsx (1)
70-70: LGTM! Test updated to reflect wizard step rename.The test expectation correctly reflects the UI change from "Users" to "Users and groups".
src/test/Components/CreateImageWizard/steps/Timezone/Timezone.test.tsx (1)
99-103: LGTM! Test updated to reflect the wizard step rename.Both the test description and assertion correctly reflect the change from "Users" to "Users and groups".
src/Components/CreateImageWizard/steps/Users/index.tsx (3)
5-5: LGTM! Import added for new Groups functionality.The GroupInfo component import is correctly placed.
12-21: LGTM! Groups section properly positioned before Users.The structural changes align with the PR objectives and reviewer feedback:
- Main title reflects combined "Users and groups" functionality
- Groups section comes first, establishing them as a prerequisite
- Explanatory text clearly communicates the workflow
22-24: LGTM! Heading hierarchy is correct.The Users section is appropriately demoted to an h2 subsection, maintaining proper heading structure under the main "Users and groups" h1.
avitova
left a comment
avitova
left a comment
There was a problem hiding this comment.
Works well. I added just a few comments regarding the code. I will not be able to ACK the PR in January, so just take it as a random input that you can take into consideration or not. :)
mgold1234
force-pushed
the
add_group_section
branch
from
8e30551 to
ad999d3
Compare
regexowl
left a comment
regexowl
left a comment
There was a problem hiding this comment.
Wanted to mark as "Request changes" instead of just comment, sorry about that...
mgold1234
force-pushed
the
add_group_section
branch
from
ab9a722 to
32acea9
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/Components/Blueprints/helpers/onPremToHostedBlueprintMapper.tsx:
- Around line 308-310: The code assigns blueprint.customizations.groups to
result.customizations!.groups but the type defines the property as group
(singular); update the assignment to use the correct property name: assign
blueprint.customizations.group to result.customizations!.group (or map
blueprint.customizations.groups -> result.customizations!.group if blueprint
actually contains plural data) and adjust any other references to "groups" to
the singular "group" to satisfy BlueprintCustomizations type; target the
assignment site where result.customizations!.groups and
blueprint.customizations.groups are used.
In @src/store/wizardSlice.ts:
- Around line 1281-1285: The TypeScript error comes from assigning undefined to
an optional numeric property under exactOptionalPropertyTypes; instead of
"state.userGroups[index].gid = undefined" remove the property using the delete
operator: "delete state.userGroups[index].gid". Update the branch where
hasDuplicate is true (the code manipulating state.userGroups[index].gid) to use
delete, keep the existing comment about clearing GID/validation, and ensure no
other code assumes gid exists after deletion.
- Around line 62-65: Remove the redundant console.error in the GID exhaustion
handler inside the function that throws the error (the block referencing
MIN_USER_GID and MAX_USER_GID); keep only the throw new Error(...) statement and
ensure its message is formatted cleanly (e.g., `All GIDs in range
${MIN_USER_GID}-${MAX_USER_GID} are exhausted`) so ESLint no longer flags an
unexpected console statement and the thrown error carries the needed context.
🧹 Nitpick comments (6)
playwright/Customizations/Users.spec.ts (1)
32-56: Consider using Playwright's native assertions for better error messages.The helper function works correctly, but mixing Jest-style assertions (
expect(groupCount).toBe()) with Playwright locators can produce less informative error messages on failure. Consider using Playwright's nativetoHaveCount()for locator counts.♻️ Suggested improvement
async function verifyGroups( frame: FrameLocator | Page, expectedGroups: string[], ) { await frame .getByRole('heading', { name: 'Groups' }) .first() .scrollIntoViewIfNeeded(); const groupNameInputs = frame.getByPlaceholder('Set group name'); await expect(groupNameInputs.first()).toBeVisible(); - const groupCount = await groupNameInputs.count(); - expect(groupCount).toBe(expectedGroups.length); + await expect(groupNameInputs).toHaveCount(expectedGroups.length); + const groupCount = await groupNameInputs.count(); const groupValues = await Promise.all( Array.from({ length: groupCount }, (_, i) => groupNameInputs.nth(i).inputValue(), ), ); expectedGroups.forEach((group) => expect(groupValues).toContain(group)); }src/Components/CreateImageWizard/steps/Users/components/GroupInfo.tsx (1)
19-25: Potential for re-render loop if state update fails.The
useEffectdepends ongroups.length, but the comment mentions a "defensive check" that doesn't actually exist in the code. IfaddUserGroup('')somehow doesn't incrementgroups.length(e.g., due to deduplication logic in the reducer), this could cause repeated dispatches on every render.The reducer does check for duplicates by
trimmedName, but since empty string trimmed is still empty string, subsequent calls with''would be ignored after the first one. This is actually fine, but the comment is misleading.Consider simplifying the comment
- // Ensure there's always at least one empty group in the store - // This provides a UX placeholder for users to start entering groups useEffect(() => { - // Only add initial empty group if none exist and we haven't already initialized - // This defensive check prevents potential infinite loops if addUserGroup fails + // Initialize with one empty group placeholder for UX if (groups.length === 0) { dispatch(addUserGroup('')); } }, [groups.length, dispatch]);src/Components/Blueprints/helpers/onPremToHostedBlueprintMapper.tsx (1)
169-193: Groups merging logic is sound but has ESLint formatting issues.The deduplication logic correctly:
- Merges both
groupsandgrouparrays for backwards compatibility- Prefers entries with defined GIDs over those without
- Returns only name and gid (when defined)
However, line 181 triggers an ESLint formatting error.
Fix ESLint formatting
// Keep the group with a defined GID, or the first occurrence if both/neither have GIDs - if (!existing || (grp.gid !== undefined && existing.gid === undefined)) { + if ( + !existing || + (grp.gid !== undefined && existing.gid === undefined) + ) { groupMap.set(grp.name, grp); }src/store/wizardSlice.ts (1)
51-60: Linear GID allocation may be slow with many groups.The
allocateUniqueGidfunction iterates from 1000 to 65534 to find an available GID. While this is unlikely to be a bottleneck in practice (users won't create thousands of groups), a more efficient approach would be to track the next available GID or use a random selection with collision detection.For the current use case, the linear approach is acceptable since:
- Users will likely create only a handful of groups
- The range is large (64,534 possible GIDs)
src/Components/CreateImageWizard/utilities/useValidation.tsx (2)
775-806: Good refactoring with generic validator.The
validateUniqueNamefunction is a clean abstraction that reduces code duplication between user and group name validation.Minor ESLint formatting fix needed at line 813:
Fix formatting
const validateUserName = ( users: UserWithAdditionalInfo[], userName: string, currentIndex: number, ): string => { - return validateUniqueName(users, userName, currentIndex, isUserNameValid, 'user'); + return validateUniqueName( + users, + userName, + currentIndex, + isUserNameValid, + 'user', + ); };
1101-1107: Fix ESLint formatting issue.Fix formatting at line 1106
const validateUserGroupName = ( userGroups: UserGroup[], groupName: string, currentIndex: number, ): string => { - return validateUniqueName(userGroups, groupName, currentIndex, isUserGroupValid, 'group'); + return validateUniqueName( + userGroups, + groupName, + currentIndex, + isUserGroupValid, + 'group', + ); };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
playwright/Customizations/Users.spec.tsplaywright/fixtures/data/exportBlueprintContents.tsplaywright/helpers/wizardHelpers.tssrc/Components/Blueprints/helpers/onPremToHostedBlueprintMapper.tsxsrc/Components/CreateImageWizard/CreateImageWizard.tsxsrc/Components/CreateImageWizard/steps/Users/components/GroupInfo.tsxsrc/Components/CreateImageWizard/steps/Users/components/GroupRow.tsxsrc/Components/CreateImageWizard/steps/Users/components/UserRow.tsxsrc/Components/CreateImageWizard/steps/Users/index.tsxsrc/Components/CreateImageWizard/utilities/requestMapper.tssrc/Components/CreateImageWizard/utilities/useValidation.tsxsrc/Components/CreateImageWizard/validators.tssrc/store/wizardSlice.tssrc/test/Components/CreateImageWizard/CreateImageWizard.test.tsxsrc/test/Components/CreateImageWizard/steps/Packages/Packages.test.tsxsrc/test/Components/CreateImageWizard/steps/Timezone/Timezone.test.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- src/Components/CreateImageWizard/steps/Users/index.tsx
- src/Components/CreateImageWizard/steps/Users/components/GroupRow.tsx
- src/test/Components/CreateImageWizard/steps/Packages/Packages.test.tsx
- src/Components/CreateImageWizard/utilities/requestMapper.ts
- playwright/fixtures/data/exportBlueprintContents.ts
🧰 Additional context used
🧬 Code graph analysis (6)
playwright/Customizations/Users.spec.ts (2)
playwright/helpers/navHelpers.ts (1)
fillInImageOutput(18-44)playwright/helpers/wizardHelpers.ts (4)
registerLater(56-61)fillInDetails(37-49)createBlueprint(16-27)fillInImageOutputGuest(67-70)
src/Components/CreateImageWizard/utilities/useValidation.tsx (3)
src/store/wizardSlice.ts (3)
UserWithAdditionalInfo(81-85)selectUserGroups(549-551)UserGroup(112-116)src/Components/CreateImageWizard/validators.ts (2)
isUserNameValid(90-98)isUserGroupValid(100-108)src/store/hooks.ts (1)
useAppSelector(7-7)
src/store/wizardSlice.ts (2)
src/store/index.ts (1)
RootState(156-156)src/Components/CreateImageWizard/validators.ts (1)
isUserGroupValid(100-108)
src/Components/CreateImageWizard/CreateImageWizard.tsx (1)
src/Components/CreateImageWizard/utilities/useValidation.tsx (1)
useUserGroupsValidation(1109-1154)
src/Components/CreateImageWizard/steps/Users/components/GroupInfo.tsx (3)
src/store/hooks.ts (2)
useAppDispatch(6-6)useAppSelector(7-7)src/store/wizardSlice.ts (1)
selectUserGroups(549-551)src/Components/CreateImageWizard/utilities/useValidation.tsx (1)
useUserGroupsValidation(1109-1154)
src/test/Components/CreateImageWizard/steps/Timezone/Timezone.test.tsx (1)
src/test/Components/CreateImageWizard/wizardTestUtils.tsx (2)
renderCreateMode(98-103)clickBack(280-284)
🪛 GitHub Actions: Development checks
playwright/Customizations/Users.spec.ts
[warning] 649-649: playwright/consistent-spacing-between-blocks: Expected blank line before this statement.
src/store/wizardSlice.ts
[error] 1284-1284: TS2412: Type 'undefined' is not assignable to type 'number' with 'exactOptionalPropertyTypes: true'. Consider adding 'undefined' to the type of the target.
src/Components/Blueprints/helpers/onPremToHostedBlueprintMapper.tsx
[error] 309-309: TS2551: Property 'groups' does not exist on type 'BlueprintCustomizations'. Did you mean 'group'?
playwright/helpers/wizardHelpers.ts
[warning] 120-120: playwright/consistent-spacing-between-blocks: Expected blank line before this statement.
🪛 GitHub Check: ESLint
src/Components/CreateImageWizard/utilities/useValidation.tsx
[failure] 813-813:
Replace users,·userName,·currentIndex,·isUserNameValid,·'user' with ⏎····users,⏎····userName,⏎····currentIndex,⏎····isUserNameValid,⏎····'user',⏎··
[failure] 1106-1106:
Replace userGroups,·groupName,·currentIndex,·isUserGroupValid,·'group' with ⏎····userGroups,⏎····groupName,⏎····currentIndex,⏎····isUserGroupValid,⏎····'group',⏎··
src/store/wizardSlice.ts
[failure] 64-64:
Replace All·GIDs·in·range·${MIN_USER_GID}-${MAX_USER_GID}·are·exhausted with ⏎····All·GIDs·in·range·${MIN_USER_GID}-${MAX_USER_GID}·are·exhausted,⏎··
[failure] 63-63:
Replace All·GIDs·in·range·${MIN_USER_GID}-${MAX_USER_GID}·are·exhausted with ⏎····All·GIDs·in·range·${MIN_USER_GID}-${MAX_USER_GID}·are·exhausted,⏎··
[failure] 63-63:
Unexpected console statement
src/Components/Blueprints/helpers/onPremToHostedBlueprintMapper.tsx
[failure] 181-181:
Replace !existing·||·(grp.gid·!==·undefined·&&·existing.gid·===·undefined) with ⏎············!existing·||⏎············(grp.gid·!==·undefined·&&·existing.gid·===·undefined)⏎··········
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: Sourcery review
- GitHub Check: Red Hat Konflux / image-builder-frontend-on-pull-request
- GitHub Check: playwright-tests
- GitHub Check: Service Tests
🔇 Additional comments (22)
src/test/Components/CreateImageWizard/steps/Timezone/Timezone.test.tsx (1)
99-104: LGTM!The test name and heading assertion correctly reflect the renamed "Users and groups" step.
playwright/helpers/wizardHelpers.ts (2)
155-163: LGTM!The
verifyExportedBlueprintfunction now properly normalizes both actual and expected content before comparison, enabling stable assertions for blueprints with auto-generated GIDs.
144-148: The GID normalization is intentionally designed for TOML format only.The
verifyExportedBlueprinttest explicitly skips verification whenisHosted()is true (hosted environments that export JSON):step.skip(isHosted(), 'Only verify the contents of the exported blueprint in cockpit'). Verification only runs for TOML exports (non-hosted environments), where the regex pattern correctly handles thegid = <number>syntax. JSON format blueprints are intentionally excluded from verification at the test level, so extendingnormalizeGidsto handle JSON syntax is not needed.playwright/Customizations/Users.spec.ts (5)
79-118: LGTM!Comprehensive test coverage for creating initial groups, including validation of group names, GID auto-generation, and the "Add group" button behavior.
120-169: LGTM!Good coverage of validation error scenarios including invalid characters, duplicate group names, and group removal functionality.
217-238: LGTM!The undefined group warning test validates the UX flow for custom groups that aren't defined in the Groups section, including the warning dismissal.
500-543: LGTM!Good test coverage for group add/remove functionality, including the edge case where the last remaining group's remove button is disabled.
545-568: LGTM!Keyboard navigation testing ensures accessibility compliance for the Groups section.
src/test/Components/CreateImageWizard/CreateImageWizard.test.tsx (1)
70-70: LGTM!The navigation button text assertion correctly reflects the renamed step.
src/Components/CreateImageWizard/steps/Users/components/UserRow.tsx (2)
211-220: LGTM!The inline warning Alert provides clear feedback when users reference groups that aren't defined in the Groups section. This addresses the reviewer feedback about safeguarding undefined group assignments.
4-4: LGTM!Alert import correctly added from PatternFly.
src/Components/CreateImageWizard/steps/Users/components/GroupInfo.tsx (1)
27-42: LGTM!The component structure is clean. The conditional spread for
groupGidand the props passed toGroupRoware well organized.src/Components/CreateImageWizard/validators.ts (1)
100-108: LGTM!The explicit
group.length >= 1check adds clarity and aligns with theman groupaddspecification comment, even though the regex would implicitly enforce a minimum length.src/Components/CreateImageWizard/CreateImageWizard.tsx (3)
56-57: LGTM!Clean import of the new validation hook alongside existing user validation.
289-290: LGTM!Proper initialization of the groups validation hook following the established pattern for other validations.
613-632: LGTM!The step name change to "Users and groups" and the combined validation logic using OR (
||) is correct. Both validations must pass for the user to proceed.src/Components/Blueprints/helpers/onPremToHostedBlueprintMapper.tsx (1)
248-279: LGTM!The reverse mapping logic correctly separates regular packages from package groups (prefixed with
@) and maps them to appropriate structures.src/store/wizardSlice.ts (1)
1229-1248: LGTM!The
addUserGroupreducer correctly:
- Trims input and checks for duplicates
- Generates GID only for valid group names
- Uses uuidv4 for unique IDs
src/Components/CreateImageWizard/utilities/useValidation.tsx (4)
79-81: LGTM!Good addition of
SYSTEM_GROUPSconstant to exclude common system groups from the "undefined groups" warning. This prevents false positives when users assign users towheelorsudo.
96-102: LGTM!The
GroupsStepValidationtype is well-structured with thehasGroupWithoutNameflag enabling differentiated UX handling.
877-912: Well-designed non-blocking warning for undefined groups.The implementation correctly:
- Excludes system groups (
wheel,sudo) from the check- Only warns about valid group names that aren't defined
- Treats
undefinedGroupsas a warning (doesn't block progression)This aligns with the PR discussion about adding informational warnings for users assigned to undefined groups.
1109-1154: LGTM!The
useUserGroupsValidationhook is well-implemented:
- Returns early with valid state when no groups exist
- Uses the "last empty group" UX pattern to avoid immediate errors
- Correctly computes
hasGroupWithoutNamefor differentiated handlingdisabledNextproperly blocks on any error or missing required names
src/store/wizardSlice.ts
Outdated
| // Gracefully handle GID exhaustion (unlikely but defensive) | ||
| console.error(`All GIDs in range ${MIN_USER_GID}-${MAX_USER_GID} are exhausted`); | ||
| throw new Error(`All GIDs in range ${MIN_USER_GID}-${MAX_USER_GID} are exhausted`); | ||
| }; |
There was a problem hiding this comment.
Remove redundant console statement and fix formatting.
The console.error is flagged by ESLint (Unexpected console statement). Since the function immediately throws an error, the console statement is redundant - the error message will be visible in the stack trace.
Proposed fix
// Gracefully handle GID exhaustion (unlikely but defensive)
- console.error(`All GIDs in range ${MIN_USER_GID}-${MAX_USER_GID} are exhausted`);
- throw new Error(`All GIDs in range ${MIN_USER_GID}-${MAX_USER_GID} are exhausted`);
+ throw new Error(
+ `All GIDs in range ${MIN_USER_GID}-${MAX_USER_GID} are exhausted`,
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Gracefully handle GID exhaustion (unlikely but defensive) | |
| console.error(`All GIDs in range ${MIN_USER_GID}-${MAX_USER_GID} are exhausted`); | |
| throw new Error(`All GIDs in range ${MIN_USER_GID}-${MAX_USER_GID} are exhausted`); | |
| }; | |
| // Gracefully handle GID exhaustion (unlikely but defensive) | |
| throw new Error( | |
| `All GIDs in range ${MIN_USER_GID}-${MAX_USER_GID} are exhausted`, | |
| ); | |
| }; |
🧰 Tools
🪛 GitHub Check: ESLint
[failure] 64-64:
Replace All·GIDs·in·range·${MIN_USER_GID}-${MAX_USER_GID}·are·exhausted with ⏎····All·GIDs·in·range·${MIN_USER_GID}-${MAX_USER_GID}·are·exhausted,⏎··
[failure] 63-63:
Replace All·GIDs·in·range·${MIN_USER_GID}-${MAX_USER_GID}·are·exhausted with ⏎····All·GIDs·in·range·${MIN_USER_GID}-${MAX_USER_GID}·are·exhausted,⏎··
[failure] 63-63:
Unexpected console statement
🤖 Prompt for AI Agents
In @src/store/wizardSlice.ts around lines 62 - 65, Remove the redundant
console.error in the GID exhaustion handler inside the function that throws the
error (the block referencing MIN_USER_GID and MAX_USER_GID); keep only the throw
new Error(...) statement and ensure its message is formatted cleanly (e.g., `All
GIDs in range ${MIN_USER_GID}-${MAX_USER_GID} are exhausted`) so ESLint no
longer flags an unexpected console statement and the thrown error carries the
needed context.
src/store/wizardSlice.ts
Outdated
| } else if (hasDuplicate && state.userGroups[index].gid) { | ||
| // Clear GID if duplicate is detected (to prevent inconsistent state) | ||
| // Validation will prevent progression with duplicate names | ||
| state.userGroups[index].gid = undefined; | ||
| } |
There was a problem hiding this comment.
Fix TypeScript error: cannot assign undefined to optional number property.
The pipeline reports TS2412: Type 'undefined' is not assignable to type 'number' with 'exactOptionalPropertyTypes: true'.
With exactOptionalPropertyTypes enabled, you cannot assign undefined to an optional property - you must delete the property instead.
Proposed fix using delete operator
} else if (hasDuplicate && state.userGroups[index].gid) {
// Clear GID if duplicate is detected (to prevent inconsistent state)
// Validation will prevent progression with duplicate names
- state.userGroups[index].gid = undefined;
+ delete state.userGroups[index].gid;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else if (hasDuplicate && state.userGroups[index].gid) { | |
| // Clear GID if duplicate is detected (to prevent inconsistent state) | |
| // Validation will prevent progression with duplicate names | |
| state.userGroups[index].gid = undefined; | |
| } | |
| } else if (hasDuplicate && state.userGroups[index].gid) { | |
| // Clear GID if duplicate is detected (to prevent inconsistent state) | |
| // Validation will prevent progression with duplicate names | |
| delete state.userGroups[index].gid; | |
| } |
🧰 Tools
🪛 GitHub Actions: Development checks
[error] 1284-1284: TS2412: Type 'undefined' is not assignable to type 'number' with 'exactOptionalPropertyTypes: true'. Consider adding 'undefined' to the type of the target.
🤖 Prompt for AI Agents
In @src/store/wizardSlice.ts around lines 1281 - 1285, The TypeScript error
comes from assigning undefined to an optional numeric property under
exactOptionalPropertyTypes; instead of "state.userGroups[index].gid = undefined"
remove the property using the delete operator: "delete
state.userGroups[index].gid". Update the branch where hasDuplicate is true (the
code manipulating state.userGroups[index].gid) to use delete, keep the existing
comment about clearing GID/validation, and ensure no other code assumes gid
exists after deletion.
32acea9 to
1e9da7e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/store/wizardSlice.ts:
- Around line 1214-1233: The handler setUserGroupNameByIndex stores the
untrimmed name but validates against trimmedName, causing inconsistent
validation and GID assignment; change it to store the trimmed value (use
trimmedName for state.userGroups[index].name) before running isUserGroupValid
and the GID-generation logic so the stored name and the validation/gid state are
consistent; keep the rest of the logic (existingGids, nextGid loop) as-is and
reference setUserGroupNameByIndex and isUserGroupValid when making the change.
🧹 Nitpick comments (1)
playwright/Customizations/Users.spec.ts (1)
114-117: Consider more robust GID verification.The assertion
not.toBeEmpty()on disabled inputs may be unreliable for verifying auto-generated GIDs. Since GIDs are auto-generated asynchronously, consider adding a wait or checking for a specific pattern:💡 Suggested improvement
// Verify GIDs are generated for valid groups const gidInputs = frame.getByPlaceholder('Auto-generated'); - await expect(gidInputs.nth(0)).not.toBeEmpty(); - await expect(gidInputs.nth(1)).not.toBeEmpty(); + // Wait for GIDs to be populated (numeric values) + await expect(gidInputs.nth(0)).toHaveValue(/^\d+$/); + await expect(gidInputs.nth(1)).toHaveValue(/^\d+$/);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
playwright/Customizations/Users.spec.tsplaywright/fixtures/data/exportBlueprintContents.tsplaywright/helpers/wizardHelpers.tssrc/Components/Blueprints/helpers/onPremToHostedBlueprintMapper.tsxsrc/Components/CreateImageWizard/CreateImageWizard.tsxsrc/Components/CreateImageWizard/steps/Users/components/GroupInfo.tsxsrc/Components/CreateImageWizard/steps/Users/components/GroupRow.tsxsrc/Components/CreateImageWizard/steps/Users/components/UserRow.tsxsrc/Components/CreateImageWizard/steps/Users/index.tsxsrc/Components/CreateImageWizard/utilities/requestMapper.tssrc/Components/CreateImageWizard/utilities/useValidation.tsxsrc/Components/CreateImageWizard/validators.tssrc/store/wizardSlice.tssrc/test/Components/CreateImageWizard/CreateImageWizard.test.tsxsrc/test/Components/CreateImageWizard/steps/Packages/Packages.test.tsxsrc/test/Components/CreateImageWizard/steps/Timezone/Timezone.test.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- src/test/Components/CreateImageWizard/steps/Packages/Packages.test.tsx
- src/Components/CreateImageWizard/steps/Users/components/GroupInfo.tsx
- src/Components/CreateImageWizard/CreateImageWizard.tsx
- src/Components/CreateImageWizard/steps/Users/components/UserRow.tsx
- src/Components/CreateImageWizard/validators.ts
- playwright/helpers/wizardHelpers.ts
- src/test/Components/CreateImageWizard/steps/Timezone/Timezone.test.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
playwright/Customizations/Users.spec.ts (2)
playwright/helpers/navHelpers.ts (1)
fillInImageOutput(18-44)playwright/helpers/wizardHelpers.ts (4)
registerLater(56-61)fillInDetails(37-49)createBlueprint(16-27)fillInImageOutputGuest(105-108)
src/store/wizardSlice.ts (2)
src/store/index.ts (1)
RootState(156-156)src/Components/CreateImageWizard/validators.ts (1)
isUserGroupValid(100-108)
src/Components/CreateImageWizard/utilities/requestMapper.ts (2)
src/store/index.ts (1)
RootState(156-156)src/store/wizardSlice.ts (1)
selectUserGroups(528-530)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: Sourcery review
- GitHub Check: Red Hat Konflux / image-builder-frontend-on-pull-request
- GitHub Check: ESLint
- GitHub Check: Service Tests
- GitHub Check: Cockpit Tests
- GitHub Check: playwright-tests
🔇 Additional comments (22)
src/Components/Blueprints/helpers/onPremToHostedBlueprintMapper.tsx (4)
80-90: LGTM! Good documentation of deprecated field.The JSDoc deprecation notice clearly documents the backwards compatibility support for the
groupfield while directing new usage togroups.
169-196: LGTM! Proper merge and deduplication logic.The IIFE correctly:
- Merges both
groupsandgrouparrays to prevent data loss- Deduplicates by name, preferring entries with defined GIDs
- Returns
undefinedfor empty results
251-281: LGTM! Clean separation of packages and package groups.The logic correctly identifies package groups by the
@prefix and maps them to the appropriate structures.
311-313: Confirm mapping to deprecatedgroupfield is intentional.This maps
blueprint.customizations.groupstoresult.customizations!.group(singular). This maintains backwards compatibility with on-prem blueprints that expect the singular form, but verify this is the intended behavior for the on-prem format.src/test/Components/CreateImageWizard/CreateImageWizard.test.tsx (1)
70-70: LGTM!The test expectation correctly reflects the renamed step from "Users" to "Users and groups".
src/Components/CreateImageWizard/steps/Users/index.tsx (1)
11-22: LGTM! Clear section structure.The Groups section is properly placed before Users with appropriate heading hierarchy (h1 for page title, h2 for sections) and helpful descriptive text.
src/Components/CreateImageWizard/utilities/requestMapper.ts (2)
491-498: LGTM! Deterministic ID generation.Using
group-${grp.name}as the ID provides stability across blueprint reloads. The conditional spread forgidcorrectly handles optional GID values.
982-991: LGTM! Proper filtering and mapping.The helper correctly:
- Filters out groups with empty/whitespace-only names
- Conditionally includes
gidonly when defined- Returns
undefinedfor empty arrays to avoid unnecessary API payloadsrc/Components/CreateImageWizard/steps/Users/components/GroupRow.tsx (2)
76-123: LGTM! Well-structured component layout.The Grid layout is clean with:
- Group name input with validation
- Conditional Add button on the last row
- Read-only GID field with "Auto-generated" placeholder
- Remove button disabled for the last remaining group
61-69: The dispatch order is safe; no early return is needed.Redux reducers execute synchronously, and no re-render occurs between the
addUserGroup()andsetUserGroupNameByIndex()dispatches within the same event handler. WhengroupCount === 0, the new group is added at index 0 (viapushto an empty array), andsetUserGroupNameByIndex({ index: 0, name })immediately targets that same position. Theindexprop remains valid and correctly maps to the newly added group.playwright/Customizations/Users.spec.ts (3)
32-56: LGTM! Clean helper function.The
verifyGroupshelper efficiently verifies group names using parallel promises and validates the expected count.
79-118: Comprehensive group creation tests.The tests thoroughly cover:
- Creating groups with valid names
- Verifying Add button behavior
- Checking GID auto-generation
- Confirming UI guidance text
217-235: Good coverage of undefined group warning.The test properly verifies that assigning a user to a custom group not defined in Groups shows an appropriate warning, and that the warning can be dismissed.
playwright/fixtures/data/exportBlueprintContents.ts (1)
145-156: The test fixture implementation is correct and does not require changes.The
<GID_PLACEHOLDER>syntax in the fixture is intentional and not a TOML parsing concern. The fixture is a TypeScript string template used for test comparison, not for direct TOML parsing. ThenormalizeGidsfunction inwizardHelpers.tsproperly handles the comparison by replacing actual numeric GIDs (from exported blueprints) with<GID_PLACEHOLDER>before comparing against the fixture. This pattern correctly isolates the test from auto-generated GID values while verifying the rest of the blueprint structure.Likely an incorrect or invalid review comment.
src/store/wizardSlice.ts (3)
91-95: LGTM!The
UserGrouptype is well-defined with appropriate fields. The optionalgidallows for the auto-generation pattern implemented in the reducer.
1208-1213: LGTM!The
addUserGroupreducer correctly initializes a new group with a unique ID and empty name, deferring GID assignment until a valid name is provided.
528-530: LGTM!Simple and consistent selector following the established pattern.
src/Components/CreateImageWizard/utilities/useValidation.tsx (5)
79-80: LGTM!
wheelandsudoare standard system groups that should not require explicit definition in the Groups section.
773-804: Nice refactor with the generic validator.Good abstraction that reduces code duplication between user and group name validation while maintaining type safety.
881-888: Good implementation of undefined groups detection.The logic correctly:
- Identifies groups assigned to users but not defined in the Groups section
- Excludes system groups (
wheel,sudo) that are expected to exist- Excludes invalid group names (already caught by other validation)
- Treats this as a warning rather than a blocking error (lines 922-924)
This aligns with the PR objective of preventing build failures from missing group definitions while allowing flexibility.
115-130: LGTM!Group validation is correctly integrated into the blueprint validity check, following the same pattern as other step validations.
1107-1144: Well-structured validation hook.The implementation correctly:
- Returns early when no groups exist
- Allows the last group row to be empty (facilitating the "add new" UX pattern)
- Requires valid, unique names for all non-last groups
- Blocks progression when validation errors exist
mgold1234
force-pushed
the
add_group_section
branch
from
1e9da7e to
c305f09
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/Components/CreateImageWizard/steps/Users/components/GroupInfo.tsx (1)
24-34: Consider usinggroup.idas the React key instead ofindex.Using index as key can cause incorrect component state preservation when groups are reordered or removed mid-list. Since each group has a unique
id(either from store or the placeholder), use that instead:♻️ Proposed fix
{groupsToRender.map((_group, index) => ( <GroupRow - key={index} + key={groupsToRender[index].id} index={index} isAddButtonDisabled={!!groupsValidation.disabledNext} stepValidation={groupsValidation} /> ))}playwright/Customizations/Users.spec.ts (1)
32-56: Good reusable helper for group verification.The
verifyGroupsfunction cleanly encapsulates group validation logic. However, theforEachwithexpectat line 55 won't properly fail the test if an expectation fails sinceforEachdoesn't propagate errors in the expected way for Playwright.♻️ Proposed fix using for...of loop
- expectedGroups.forEach((group) => expect(groupValues).toContain(group)); + for (const group of expectedGroups) { + expect(groupValues).toContain(group); + }src/Components/CreateImageWizard/utilities/useValidation.tsx (2)
79-80: SYSTEM_GROUPS list may need expansion.Per the PR discussion, the list of system-defined groups can vary by distribution and target image type. Currently only
wheelandsudoare included. Consider documenting this limitation or creating a follow-up issue to expand this list based on distro/image type.
912-915: Consider refining the warning message.The current message is slightly informal. A more concise alternative:
💡 Suggested message
if (undefinedGroups.length > 0) { userErrors.undefinedGroups = - "You've assigned user to a custom group that is not defined in 'Groups', make sure it will exist on the system"; + "User assigned to undefined group(s). Ensure these groups exist on the system or define them in the 'Groups' section."; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
playwright/Customizations/Users.spec.tsplaywright/fixtures/data/exportBlueprintContents.tsplaywright/helpers/wizardHelpers.tssrc/Components/Blueprints/helpers/onPremToHostedBlueprintMapper.tsxsrc/Components/CreateImageWizard/CreateImageWizard.tsxsrc/Components/CreateImageWizard/steps/Users/components/GroupInfo.tsxsrc/Components/CreateImageWizard/steps/Users/components/GroupRow.tsxsrc/Components/CreateImageWizard/steps/Users/components/UserRow.tsxsrc/Components/CreateImageWizard/steps/Users/index.tsxsrc/Components/CreateImageWizard/utilities/requestMapper.tssrc/Components/CreateImageWizard/utilities/useValidation.tsxsrc/Components/CreateImageWizard/validators.tssrc/store/wizardSlice.tssrc/test/Components/CreateImageWizard/CreateImageWizard.test.tsxsrc/test/Components/CreateImageWizard/steps/Packages/Packages.test.tsxsrc/test/Components/CreateImageWizard/steps/Timezone/Timezone.test.tsx
🚧 Files skipped from review as they are similar to previous changes (8)
- src/Components/CreateImageWizard/CreateImageWizard.tsx
- src/test/Components/CreateImageWizard/steps/Timezone/Timezone.test.tsx
- playwright/helpers/wizardHelpers.ts
- src/Components/CreateImageWizard/validators.ts
- src/test/Components/CreateImageWizard/CreateImageWizard.test.tsx
- src/Components/CreateImageWizard/steps/Users/components/UserRow.tsx
- src/test/Components/CreateImageWizard/steps/Packages/Packages.test.tsx
- src/store/wizardSlice.ts
🧰 Additional context used
🧬 Code graph analysis (3)
src/Components/CreateImageWizard/steps/Users/components/GroupRow.tsx (5)
src/Components/CreateImageWizard/utilities/useValidation.tsx (1)
GroupsStepValidation(95-100)src/store/hooks.ts (2)
useAppDispatch(6-6)useAppSelector(7-7)src/store/wizardSlice.ts (1)
selectUserGroups(528-530)src/Components/CreateImageWizard/validators.ts (1)
isUserGroupValid(100-108)src/Components/CreateImageWizard/ValidatedInput.tsx (1)
ValidatedInputAndTextArea(49-124)
src/Components/CreateImageWizard/utilities/useValidation.tsx (3)
src/store/wizardSlice.ts (3)
UserWithAdditionalInfo(60-64)selectUserGroups(528-530)UserGroup(91-95)src/Components/CreateImageWizard/validators.ts (2)
isUserNameValid(90-98)isUserGroupValid(100-108)src/store/hooks.ts (1)
useAppSelector(7-7)
src/Components/CreateImageWizard/utilities/requestMapper.ts (2)
src/store/index.ts (1)
RootState(156-156)src/store/wizardSlice.ts (1)
selectUserGroups(528-530)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: Red Hat Konflux / image-builder-frontend-on-pull-request
- GitHub Check: playwright-tests
- GitHub Check: Service Tests
🔇 Additional comments (22)
src/Components/Blueprints/helpers/onPremToHostedBlueprintMapper.tsx (4)
80-90: Good documentation for the deprecated field.The JSDoc comments clearly indicate which field is canonical (
groups) and which is deprecated (group), along with the backwards compatibility rationale.
136-141: Clear separation of package groups from user groups.The comment clarifies the naming distinction well. The packages array construction correctly combines regular packages and package groups.
Also applies to: 159-164
169-196: Deduplication logic for merging groups is well-structured.The approach of preferring groups with defined GIDs when merging duplicates is sound. The IIFE keeps the logic contained and readable.
311-313: Verify intended field name:groupvsgroups.This maps
blueprint.customizations.groupstoresult.customizations!.group(singular). This appears intentional for backwards compatibility with on-prem blueprints that use the deprecatedgroupfield, but please confirm this asymmetry is expected.src/Components/CreateImageWizard/utilities/requestMapper.ts (3)
491-498: Deterministic ID approach works well for stable state hydration.Using
group-${grp.name}ensures consistent IDs across blueprint loads. The conditional GID inclusion is clean.One consideration: if a user renames a group during editing, the ID will change. This is likely acceptable since the ID is only for React keys and internal tracking, not persisted.
982-991: Clean helper function for extracting user groups.Properly filters out empty/whitespace names and conditionally includes GID only when defined. Returns
undefinedfor empty arrays as expected by the API.
807-807: Groups field properly integrated into customizations.The placement alongside other customization getters is consistent with the existing pattern.
playwright/fixtures/data/exportBlueprintContents.ts (1)
145-156: Test fixture correctly uses placeholder for auto-generated GIDs.The
<GID_PLACEHOLDER>pattern allows test verification logic to normalize the actual GID values. The groups are correctly positioned before users in the blueprint, matching the on-prem format with the[[customizations.group]]syntax.src/Components/CreateImageWizard/steps/Users/index.tsx (1)
11-22: Groups section correctly positioned before Users.The layout follows the feedback from reviewers to place groups first since they are a prerequisite for user assignments. The descriptive text clearly explains the auto-generated ID behavior.
src/Components/CreateImageWizard/steps/Users/components/GroupRow.tsx (4)
76-77: Verify intentional Gridmd={11}value.The Grid uses
md={11}which leaves 1 column unused at medium breakpoints. If this is intentional for alignment with other form elements, consider adding a comment. Otherwise,md={12}would use full width.
103-109: GID field correctly set to read-only with placeholder.The implementation follows the PR discussion: GID is non-editable and auto-generated by the OS to avoid conflicts with reserved system ranges.
111-121: Remove button correctly disabled when only one group remains.Using
groupCount <= 1ensures users cannot remove the last group, maintaining at least one group row for UX consistency with the placeholder pattern.
61-69: No timing issues with sequential dispatches.The sequential dispatches of
addUserGroup()andsetUserGroupNameByIndex()work correctly because Redux reducers execute synchronously. WhengroupCount === 0, theaddUserGroup()reducer synchronously pushes a new group to the array beforesetUserGroupNameByIndex()is dispatched, so there is no race condition or timing issue here.playwright/Customizations/Users.spec.ts (4)
73-118: Comprehensive test coverage for group creation flow.Tests cover initial group creation, validation of required fields, Add button visibility, and GID auto-generation. The assertions are well-structured and follow the user workflow.
120-169: Thorough validation error testing for groups.Covers invalid characters, duplicates, and removal behavior. The test properly verifies that Add button is disabled when validation fails.
217-235: Good coverage for undefined/custom group warnings.Tests the warning message when assigning users to groups not defined in the Groups section, and verifies the warning can be dismissed.
499-567: Solid coverage for group add/remove and keyboard navigation.Tests verify that the last group cannot be removed (button disabled), groups can be added/removed mid-list, and keyboard navigation works correctly between group fields.
src/Components/CreateImageWizard/utilities/useValidation.tsx (5)
95-100: LGTM!Type definition is consistent with the existing
UsersStepValidationpattern.
773-804: Good refactoring to reduce code duplication.The generic
validateUniqueNamefunction cleanly abstracts the common validation pattern for both users and groups.
1093-1105: LGTM!Clean wrapper following the established validation pattern.
1107-1144: LGTM!The implementation correctly handles the UX pattern of allowing the last empty group row while requiring names for all other groups. The hook properly integrates with the overall blueprint validity check.
115-131: LGTM!Proper integration of group validation into the overall blueprint validity check.
mgold1234
force-pushed
the
add_group_section
branch
from
c305f09 to
955af78
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/store/wizardSlice.ts`:
- Around line 40-42: Remove the stray blank line between the import of
isUserGroupValid and the constants RHEL_10, RHEL_10_IMAGE_MODE, and X86_64 in
wizardSlice.ts so the imports are contiguous and comply with the ESLint
import/order rule; ensure the imports remain in the same order (isUserGroupValid
followed by RHEL_10, RHEL_10_IMAGE_MODE, X86_64) and run lint to verify the
import/order rule passes.
🧹 Nitpick comments (1)
src/store/wizardSlice.ts (1)
1243-1262: LGTM with optional improvement.The simplified sequential GID allocation is cleaner than the previous random approach. The trimming fix addresses the earlier feedback.
Consider adding an upper bound check for the GID range (typically 1000-65534 for user-created groups). While exceeding this limit is extremely unlikely in practice, it would provide a safety net.
Optional: Add upper bound check
if (isUserGroupValid(trimmedName) && !state.userGroups[index].gid) { const existingGids = new Set( state.userGroups.map((g) => g.gid).filter((gid) => gid !== undefined), ); - let nextGid = 1000; - while (existingGids.has(nextGid)) { + const MAX_USER_GID = 65534; + let nextGid = 1000; + while (existingGids.has(nextGid) && nextGid <= MAX_USER_GID) { nextGid++; } - state.userGroups[index].gid = nextGid; + if (nextGid <= MAX_USER_GID) { + state.userGroups[index].gid = nextGid; + } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Components/CreateImageWizard/utilities/requestMapper.tssrc/store/wizardSlice.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Components/CreateImageWizard/utilities/requestMapper.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/store/wizardSlice.ts (2)
src/store/index.ts (1)
RootState(156-156)src/Components/CreateImageWizard/validators.ts (1)
isUserGroupValid(100-108)
🪛 GitHub Actions: Development checks
src/store/wizardSlice.ts
[error] 41-41: eslint: import/order - There should be no empty line within import group.
🪛 GitHub Check: ESLint
src/store/wizardSlice.ts
[failure] 41-41:
There should be no empty line within import group
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (48)
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: Sourcery review
- GitHub Check: Red Hat Konflux / image-builder-frontend-on-pull-request
- GitHub Check: Service Tests
- GitHub Check: playwright-tests
🔇 Additional comments (6)
src/store/wizardSlice.ts (6)
99-103: LGTM!The
UserGrouptype is well-defined with appropriate optionality forgid.
170-170: LGTM!State type extension follows existing patterns.
548-551: LGTM!Selector follows the established pattern.
1237-1242: LGTM!Clean implementation - deferring GID assignment until the name is valid is the right approach per PR discussion.
1263-1265: LGTM!Follows the existing
removeUserpattern.
1598-1600: LGTM!All new actions are properly exported.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
mgold1234
force-pushed
the
add_group_section
branch
2 times, most recently
from
e13affe to
6665818
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/store/wizardSlice.ts`:
- Around line 40-41: The GID assignment behavior is ambiguous: update the
implementation and UI to match the intended strategy. Either (A) if you want
OS-driven GID assignment, change setUserGroupNameByIndex and requestMapper to
leave gid undefined (do not populate sequential GIDs) and update any UI
text/placeholder that currently says "Each group will automatically be assigned
an ID number" to explicitly state the OS will assign GIDs; or (B) if client-side
assignment is intentional, add a clear comment near setUserGroupNameByIndex and
the gid handling in requestMapper explaining why frontend GID generation
(sequential IDs starting at 1000) is used instead of leaving gid undefined, and
update the UI text to say something like "This client will assign group IDs
starting at 1000" so users aren’t misled.
6665818 to
75e27a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@playwright/helpers/wizardHelpers.ts`:
- Around line 185-188: The normalizeGids function currently only replaces
TOML-style "gid = <number>"; update it to also normalize JSON-style keys by
matching patterns like `"gid"\s*:\s*\d+` (and variants without quotes if needed)
so hosted JSON exports like `"gid": 1000` are replaced with the same
placeholder; modify the replacement logic in normalizeGids to run both regexes
(or a single combined regex) and return content where both TOML `gid = <number>`
and JSON `"gid": <number>` are normalized to `gid = <GID_PLACEHOLDER>` (or a
consistent placeholder string) so comparisons no longer fail.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/Components/Blueprints/helpers/onPremToHostedBlueprintMapper.tsx`:
- Around line 169-170: Add a missing newline before the "groups:" property
assignment to satisfy Prettier; locate the return/object block in
onPremToHostedBlueprintMapper (the section handling customizations) and ensure
there is a blank line above the line containing groups:
blueprint.customizations?.groups || blueprint.customizations?.group so the file
is properly formatted.
In `@src/Components/CreateImageWizard/utilities/requestMapper.ts`:
- Around line 433-444: The compliance object literal in requestMapper.ts has
inconsistent indentation in the ternary branches (the block setting
complianceType, policyID, profileID, policyTitle), causing ESLint failures; fix
by normalizing indentation and alignment for both branches of the ternary
(ensure each property line is indented the same way and the object braces
align), or refactor the ternary into a small helper (e.g.,
buildComplianceObject) that returns the correctly formatted object—update the
branch that sets complianceType: 'compliance' and the branch that sets
complianceType: 'openscap' (including policyID/profileID/policyTitle) so both
conform to project ESLint formatting rules.
In `@src/Components/sharedComponents/NetworkInstallerAlert.tsx`:
- Around line 7-16: The JSDoc for NetworkInstallerAlert says it should show only
when "network-installer" is selected along with other targets, but the component
currently uses useHasNetworkInstaller() which is true even when it's the sole
selection; either update the JSDoc to reflect the current behavior or change the
condition to use useHasNetworkInstallerOnly() so the alert only renders for
mixed selections. Locate the NetworkInstallerAlert component and replace the
hasNetworkInstaller check with useHasNetworkInstallerOnly() if you want
mixed-selection behavior, or edit the comment above NetworkInstallerAlert to
accurately describe that the alert appears whenever network-installer is
selected.
src/Components/Blueprints/helpers/onPremToHostedBlueprintMapper.tsx
Outdated
Show resolved
Hide resolved
mgold1234
force-pushed
the
add_group_section
branch
2 times, most recently
from
ddf6d97 to
b5ba019
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@playwright/Customizations/Users.spec.ts`:
- Around line 219-237: The test 'Verify undefined group warning' incorrectly
assumes the undefined-group warning disappears after removing only 'customgroup'
while 'wheel' (assigned earlier to admin1) remains undefined; update the test to
either define 'wheel' in the Groups setup (so it's no longer an undefined group)
or remove the 'wheel' assignment before asserting the warning is hidden—locate
the test.step named 'Verify undefined group warning' and modify the flow around
groupInputs, warningMessage, and closeButton so that all undefined groups
(including 'wheel') are resolved (by adding 'wheel' to the Groups list or
deleting its assignment) prior to the final expect(warningMessage).toBeHidden()
assertion.
🧹 Nitpick comments (3)
src/store/wizardSlice.ts (1)
1243-1262: Consider adding a safeguard for GID upper bound.The sequential GID search starting at 1000 is clean and simple. However, the
whileloop has no upper bound check—if all GIDs in the valid range (1000-65534) were somehow exhausted, this would loop indefinitely. While practically impossible in this UI context, a defensive upper bound would make this more robust.🛡️ Optional defensive fix
if (isUserGroupValid(trimmedName) && !state.userGroups[index].gid) { const existingGids = new Set( state.userGroups.map((g) => g.gid).filter((gid) => gid !== undefined), ); let nextGid = 1000; - while (existingGids.has(nextGid)) { + while (existingGids.has(nextGid) && nextGid <= 65534) { nextGid++; } - state.userGroups[index].gid = nextGid; + if (nextGid <= 65534) { + state.userGroups[index].gid = nextGid; + } }playwright/Customizations/Users.spec.ts (2)
75-120: Lock in GID non-editable behavior with an explicit assertion.
Since GIDs are auto-generated, add a read-only/disabled check to prevent regressions.🔧 Suggested assertion (adjust to readonly vs disabled as implemented)
const gidInputs = frame.getByPlaceholder('Auto-generated'); await expect(gidInputs.nth(0)).toHaveValue(/^\d+$/); await expect(gidInputs.nth(1)).toHaveValue(/^\d+$/); +await expect(gidInputs.nth(0)).toBeDisabled(); // or toHaveAttribute('readonly', '') +await expect(gidInputs.nth(1)).toBeDisabled();
122-156: Scope group validation assertions to the Groups section.
SeveralgetByText(...)checks are global and could become flaky if Users introduces similar messages.♻️ Example scoping approach (use a stable container/testid if available)
- await expect(frame.getByText('Invalid group name')).toBeVisible(); + const groupsSection = frame + .getByRole('heading', { name: 'Groups' }) + .first() + .locator('..'); + await expect(groupsSection.getByText('Invalid group name')).toBeVisible();
mgold1234
force-pushed
the
add_group_section
branch
from
b5ba019 to
fb465fc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Components/CreateImageWizard/steps/Users/index.tsx`:
- Around line 17-21: Prettier is failing on the JSX text node inside the Content
component; fix by reformatting the text so it complies with Prettier (either
make the paragraph a single line or break lines at natural sentence boundaries)
in the JSX element named Content in
src/Components/CreateImageWizard/steps/Users/index.tsx; you can manually adjust
the line breaks to match project Prettier rules or run prettier --write on the
file to apply the correct formatting automatically.
♻️ Duplicate comments (1)
src/Components/CreateImageWizard/steps/Users/components/GroupRow.tsx (1)
49-51: Remove dead code - this condition is unreachable.Per the past review comment that was marked as addressed, this block should be removed. When
GroupInforendersGroupRow, it always ensures at least one group exists (either from state or a placeholder), sogroupCountwill never be 0 when this handler is called.🧹 Proposed fix
const handleGroupNameChange = ( _e: React.FormEvent<HTMLInputElement | HTMLTextAreaElement>, value: string, ) => { - if (groupCount === 0) { - dispatch(addUserGroup()); - } dispatch(setUserGroupNameByIndex({ index, name: value })); };
🧹 Nitpick comments (1)
src/Components/Blueprints/helpers/onPremToHostedBlueprintMapper.tsx (1)
168-170: Consider deduplicating merged groups.When merging
blueprint.customizations?.groupsandblueprint.customizations?.group, if the same group name exists in both arrays with different GIDs, duplicates will be present in the output. This was flagged in a previous review but the deduplication logic appears to not have been added.♻️ Proposed fix to deduplicate groups by name
// Support both 'groups' (hosted/current) and 'group' (on-prem/legacy) formats - groups: - blueprint.customizations?.groups || blueprint.customizations?.group, + groups: (() => { + const allGroups = [ + ...(blueprint.customizations?.groups || []), + ...(blueprint.customizations?.group || []), + ]; + // Deduplicate by group name, preferring later occurrences + const uniqueGroups = Array.from( + new Map(allGroups.map((grp) => [grp.name, grp])).values(), + ); + return uniqueGroups.length > 0 ? uniqueGroups : undefined; + })(),
Adds a new "Groups" section to the Users step, allowing users to define groups prior to assigning users to them. This prevents image build failures caused by missing group definitions.
and update exiting tests with the changes of rename step
add support for both `customizations.groups` (hosted) and `customizations.group` (on-premise) formats. Updated mapper and export logic to handle format conversion between JSON and TOML blueprints.
mgold1234
force-pushed
the
add_group_section
branch
from
fb465fc to
d673399
Compare
regexowl
left a comment
regexowl
left a comment
There was a problem hiding this comment.
Left some comments and nitpicks. Didn't manage to go through the tests yet (apart form a quick note), will try to get to them soon.
| const group = groups[index] || { | ||
| id: 'placeholder', | ||
| name: '', | ||
| gid: undefined, | ||
| }; |
There was a problem hiding this comment.
| groups.length === 0 | ||
| ? [ | ||
| { | ||
| id: 'placeholder', |
There was a problem hiding this comment.
Why is the extra id needed? Both here and in users. As the name of the user has to be unique, we can just use that to look up a specific group.
| users.length === 0 | ||
| ? [ | ||
| { | ||
| id: 'temp-user', |
There was a problem hiding this comment.
Why is the extra id needed for user?
|
|
||
| [[customizations.group]] | ||
| name = "developers" | ||
| gid = <GID_PLACEHOLDER> |
There was a problem hiding this comment.
I see, so since the GIDs are generated, we don't have any way to test with the "real" values, is that right?
| // Test 3: Duplicate username error | ||
| await expect( | ||
| frame.getByText('Username already exists').first(), | ||
| frame.getByText('User name already exists').first(), |
There was a problem hiding this comment.
"Username" is a technical term and I believe there shouldn't be a space.
| name: pkg, | ||
| version: '*', | ||
| }; | ||
| // Separate regular packages from package groups (those starting with @) |
There was a problem hiding this comment.
Unnecessary comment, the intention is clear from the code
| } | ||
| }); | ||
|
|
||
| // Map regular packages to packages array |
There was a problem hiding this comment.
Unnecessary comment, the intention is clear from the code
| }); | ||
| } | ||
|
|
||
| // Map package groups to blueprint.groups array |
There was a problem hiding this comment.
Unnecessary comment, the intention is clear from the code
|
decided to split this work into smaller task, so I am putting this one into draft, and will push smaller prs |
|
decided to split that into smaller prs |
4 participants
This added the new "Groups" section to the Users step, allowing users to define groups prior to
assigning users to them. This prevents image build failures caused by missing group definitions.
implement it as per as the mock from Shayna:
JIRA: HMS-8784