Wizard: Add group value on blur, Add user, and Next actions (HMS-10148)#3944
Wizard: Add group value on blur, Add user, and Next actions (HMS-10148)#3944mgold1234 wants to merge 1 commit intoosbuild:mainfrom
Conversation
mgold1234
requested review from
avitova,
ksiekl and
regexowl
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 - I've found 1 issue, and left some high level feedback:
- The
GroupInputContextonly supportsregisterRefand never deregisters refs, so unmounted rows will leave stale entries in theSet; consider adding aderegisterRefAPI and cleaning up in theuseEffectofUserRowto avoid leaking references. - The
flushUsersGroupInputsflow currently relies on awindow-level custom event; it would be cleaner and easier to reason about ifCreateImageWizardinvoked a context method (e.g.,flushAllInputs) directly instead of using a global event bus.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `GroupInputContext` only supports `registerRef` and never deregisters refs, so unmounted rows will leave stale entries in the `Set`; consider adding a `deregisterRef` API and cleaning up in the `useEffect` of `UserRow` to avoid leaking references.
- The `flushUsersGroupInputs` flow currently relies on a `window`-level custom event; it would be cleaner and easier to reason about if `CreateImageWizard` invoked a context method (e.g., `flushAllInputs`) directly instead of using a global event bus.
## Individual Comments
### Comment 1
<location> `src/Components/CreateImageWizard/steps/Users/components/GroupInputContext.tsx:25-26` </location>
<code_context>
+}) => {
+ const refsRef = useRef<Set<LabelInputRef>>(new Set());
+
+ const registerRef = useCallback((ref: LabelInputRef | null) => {
+ if (ref) {
+ refsRef.current.add(ref);
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Refs are only ever added to the context set and never removed, which can lead to stale refs being flushed.
`refsRef` only ever grows: `registerRef` adds entries but there’s no corresponding removal when a `UserRow` unmounts. This can leave stale `LabelInputRef`s in the set, so `flushAllInputs` may call `flushInput` on unmounted components, causing unnecessary work and potentially odd behavior if the imperative handle closes over stale state.
Consider adding an `unregisterRef` (or equivalent cleanup) and calling it from `UserRow`’s cleanup, or using a callback ref pattern so the set always reflects the currently mounted inputs.
Suggested implementation:
```typescript
export const GroupInputProvider: React.FC<{ children: React.ReactNode }> = ({
children,
}) => {
const refsRef = useRef<Set<LabelInputRef>>(new Set());
const registerRef = useCallback((ref: LabelInputRef | null) => {
if (!ref) {
return;
}
refsRef.current.add(ref);
}, []);
const unregisterRef = useCallback((ref: LabelInputRef | null) => {
if (!ref) {
return;
}
refsRef.current.delete(ref);
}, []);
```
Based on the partial snippet, there are a few more places that likely need to be updated:
1. **Context value type**
Wherever the context type is defined (e.g. `type GroupInputContextValue = { ... }` or similar), add `unregisterRef: (ref: LabelInputRef | null) => void` to the type so TypeScript knows it exists.
2. **Context provider value**
In this same file, where `GroupInputContext.Provider` is returned, include `unregisterRef` in the value object, e.g.:
```ts
<GroupInputContext.Provider
value={{
registerRef,
unregisterRef,
/* other fields like flushAllInputs, etc. */
}}
>
{children}
</GroupInputContext.Provider>
```
3. **Consumers (e.g. `UserRow`)**
In `UserRow` (or whichever component registers the `LabelInputRef`):
- Destructure `unregisterRef` from `useGroupInputContext()` (or equivalent hook).
- Store the `LabelInputRef` in a local `useRef` so it can be referenced in cleanup:
```ts
const inputRef = useRef<LabelInputRef | null>(null);
```
- When registering the ref (e.g. via callback ref or `useImperativeHandle`), call `registerRef(inputRef.current)` once the ref is set.
- Use `useEffect` cleanup to call `unregisterRef(inputRef.current)` on unmount:
```ts
useEffect(() => {
if (inputRef.current) {
registerRef(inputRef.current);
}
return () => {
if (inputRef.current) {
unregisterRef(inputRef.current);
}
};
}, [registerRef, unregisterRef]);
```
These adjustments ensure that `refsRef` reflects only mounted inputs and prevents stale refs from being flushed.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/Components/CreateImageWizard/steps/Users/components/GroupInputContext.tsx
Outdated
Show resolved
Hide resolved
📝 WalkthroughWalkthroughThis PR updates the CreateImageWizard and User-related components: LabelInput gains controlled Redux input support, consolidated validation error reporting, onBlur add behavior, and new props ( 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: 2
🧹 Nitpick comments (1)
src/Components/CreateImageWizard/steps/Users/components/GroupInputContext.tsx (1)
12-18: Consider throwing an error when context is unavailable.Returning
nullwhen the context is not found could lead to silent failures if the provider is accidentally missing. Consider whether this should throw an error or at least log a warning.Alternative implementation
export const useGroupInputContext = () => { const context = useContext(GroupInputContext); if (!context) { - return null; // Return null instead of throwing - makes it optional + throw new Error('useGroupInputContext must be used within a GroupInputProvider'); } return context; };If the optional behavior is intentional, consider documenting why.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Components/CreateImageWizard/CreateImageWizard.tsxsrc/Components/CreateImageWizard/LabelInput.tsxsrc/Components/CreateImageWizard/steps/Users/components/GroupInputContext.tsxsrc/Components/CreateImageWizard/steps/Users/components/UserRow.tsxsrc/Components/CreateImageWizard/steps/Users/index.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
src/Components/CreateImageWizard/steps/Users/index.tsx (1)
src/Components/CreateImageWizard/steps/Users/components/GroupInputContext.tsx (1)
GroupInputProvider(20-52)
src/Components/CreateImageWizard/LabelInput.tsx (1)
src/store/hooks.ts (1)
useAppDispatch(6-6)
src/Components/CreateImageWizard/steps/Users/components/GroupInputContext.tsx (1)
src/Components/CreateImageWizard/LabelInput.tsx (1)
LabelInputRef(33-35)
⏰ 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). (24)
- 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-rawhide-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-9-aarch64
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: rpm-build:fedora-42-x86_64
- 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: playwright-tests
- GitHub Check: Service Tests
- GitHub Check: Red Hat Konflux / image-builder-frontend-on-pull-request
🔇 Additional comments (4)
src/Components/CreateImageWizard/CreateImageWizard.tsx (1)
621-626: LGTM - flush mechanism integrated correctly.The
beforeNextcallback properly dispatches the flush event before navigation proceeds. The implementation ensures group inputs are committed before leaving the Users step.src/Components/CreateImageWizard/steps/Users/index.tsx (1)
5-5: LGTM - clean provider integration.The
GroupInputProvideris correctly wrapped around the form, enabling the group input flush mechanism for all user rows.Also applies to: 10-21
src/Components/CreateImageWizard/steps/Users/components/UserRow.tsx (1)
73-77: LGTM - flush before adding user.The optional chaining ensures safety, and flushing pending group input before adding a new user maintains data consistency.
src/Components/CreateImageWizard/LabelInput.tsx (1)
1-227: Excellent refactor to forwardRef pattern.The component has been successfully refactored to expose the
flushInputmethod via ref, enabling the group input flush mechanism. Key improvements:
- Clean
LabelInputReftype definition- Proper use of
forwardRefanduseImperativeHandle- Internal state management for input value and errors
onBlurandonKeyDownhandlers correctly trigger item addition- Comprehensive validation with field-specific error messages
- Maintains existing functionality while adding new capabilities
The implementation is solid and follows React best practices.
src/Components/CreateImageWizard/steps/Users/components/GroupInputContext.tsx
Outdated
Show resolved
Hide resolved
src/Components/CreateImageWizard/steps/Users/components/UserRow.tsx
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #3944 +/- ##
==========================================
- Coverage 73.15% 73.12% -0.04%
==========================================
Files 197 197
Lines 7298 7344 +46
Branches 2712 2735 +23
==========================================
+ Hits 5339 5370 +31
- Misses 1716 1727 +11
- Partials 243 247 +4
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
mgold1234
force-pushed
the
fix_user_be
branch
3 times, most recently
from
feb0a56 to
52b0e6f
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)
src/Components/CreateImageWizard/LabelInput.tsxsrc/Components/CreateImageWizard/utilities/requestMapper.ts
⏰ 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). (46)
- GitHub Check: Sourcery review
- GitHub Check: Red Hat Konflux / image-builder-frontend-on-pull-request
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: testing-farm:centos-stream-10-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:centos-stream-10-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:fedora-43-x86_64:self
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: rpm-build:fedora-43-x86_64
- 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: 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: 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
🔇 Additional comments (2)
src/Components/CreateImageWizard/LabelInput.tsx (2)
66-76: Excellent trimming and normalization logic!The changes correctly:
- Trim input values and reject empty/whitespace-only strings (lines 67-69)
- Use
trimmedValueconsistently for duplicate detection (line 71), validation (line 76), and dispatching (line 122)- Prevent edge cases like treating
" admin "and"admin"as different valuesThis improves data quality and prevents confusing UX issues.
Also applies to: 122-122
134-138: Smart blur handler implementation that elegantly solves all PR objectives!The
handleBlurfunction (lines 134-138) paired withonBlurwiring (line 158) correctly adds the group value when the input loses focus. This implementation cleverly handles all three scenarios from the PR objectives:
- Manual blur (user clicks outside) →
onBlurfires- "Add user" button click → focus moves to button → input blurs →
onBlurfires- "Next" button click → focus moves to button → input blurs →
onBlurfiresBy leveraging the natural blur behavior when buttons are clicked, you avoid needing to wire up explicit handlers in parent components or expose imperative methods. The implementation maintains the existing Enter key behavior while adding the blur-based submission as requested.
Also applies to: 158-158
|
I understand the idea and agree that it might not be immediately obvious that the user needs to "add" the group after they've already typed it in the input. But maybe we should check the proposed behaviour with the UX team first? It brings a bit of inconsistency as is. Some of my thoughts - tried to click around and what felt weird is that the group is only added when it's valid. Now that makes sense from the point of validation, but when the value is invalid it just disappears on Next. If the username is empty and the group is valid, it also disappears on Next. In general adding values "out of sight" feels a bit weird. As for "confirming" group when adding a new user or on blur - maybe? But it feels like we're doing changes the user didn't necessarily asked for... maybe they started adding a group, clicked on a different tab to double check what should the group be called, but when they come back to the wizard, the partial name is already added to a group list... So I'd say let's first check with the UX team maybe? |
|
Maybe we could add some kind of guidance or a helper text to let people know that they can add a group on Enter keydown? To let people know about things without doing changes they might not want. I don't know, just more thoughts... |
regexowl
left a comment
regexowl
left a comment
There was a problem hiding this comment.
Thanks for double checking the behaviour with the UX team! ❤️
The on blur and on add a user behaviour looks great. Let's just tweak the Next button behaviour: "trigger validation when "Next" is clicked and if there are any errors to render them and block the Next button"
mgold1234
force-pushed
the
fix_user_be
branch
from
352e0f6 to
a7d6ae5
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @src/Components/CreateImageWizard/CreateImageWizard.tsx:
- Around line 288-290: The Users step is disabling Next while usersPristine is
true which prevents beforeNext from running and revealing input errors; change
the logic to match the filesystemPristine pattern: keep Next enabled when
usersPristine is true, have the beforeNext handler call setUsersPristine(false)
then validate inputs and setHasUsersInputErrors appropriately, and derive the
footer/status disabled state using both usersPristine and hasUsersInputErrors
(i.e., block navigation only after pristine is cleared and input errors are
present). Ensure the same fix is applied to the other Users footer block around
the status logic referenced (lines ~617-633) so status reflects
hasUsersInputErrors.
🧹 Nitpick comments (1)
src/Components/CreateImageWizard/steps/Users/index.tsx (1)
7-25: Prop plumbing looks consistent and keeps Users step stateless (Line 7-25).Tiny nit: if this prop type is consumed elsewhere, consider exporting
UsersStepProps(or usinginterface) to improve reuse/discoverability.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Components/CreateImageWizard/CreateImageWizard.tsxsrc/Components/CreateImageWizard/LabelInput.tsxsrc/Components/CreateImageWizard/steps/Users/components/UserInfo.tsxsrc/Components/CreateImageWizard/steps/Users/components/UserRow.tsxsrc/Components/CreateImageWizard/steps/Users/index.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
src/Components/CreateImageWizard/LabelInput.tsx (1)
src/store/hooks.ts (1)
useAppDispatch(6-6)
src/Components/CreateImageWizard/steps/Users/components/UserRow.tsx (2)
src/store/hooks.ts (1)
useAppDispatch(6-6)src/Components/CreateImageWizard/utilities/useValidation.tsx (1)
useUsersValidation(789-879)
src/Components/CreateImageWizard/steps/Users/components/UserInfo.tsx (2)
src/store/hooks.ts (1)
useAppSelector(7-7)src/store/wizardSlice.ts (1)
selectUsers(515-517)
⏰ 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). (24)
- 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: 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-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: rpm-build:centos-stream-10-x86_64
- 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:fedora-rawhide-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: playwright-tests
- GitHub Check: Service Tests
🔇 Additional comments (3)
src/Components/CreateImageWizard/steps/Users/components/UserInfo.tsx (1)
51-58: Pristine-gated alert behavior is consistent with “don’t yell until interaction” (Line 51-58).src/Components/CreateImageWizard/LabelInput.tsx (1)
73-146: Trimmed add + blur-to-add matches the PR objective and avoids whitespace duplicates (Line 73-146).src/Components/CreateImageWizard/steps/Users/components/UserRow.tsx (1)
57-64: Per-row “in-flight input error” propagation is a good hook for the wizard guard (Line 57-64, 234-235).Also applies to: 234-235
src/Components/CreateImageWizard/steps/Users/components/UserInfo.tsx
Outdated
Show resolved
Hide resolved
mgold1234
force-pushed
the
fix_user_be
branch
from
a7d6ae5 to
d1e44ae
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/Components/CreateImageWizard/CreateImageWizard.tsx (1)
617-635: Consider extracting the duplicated validation condition.The condition
usersValidation.disabledNext || hasUsersInputErrorsappears three times (lines 619, 626, and 634). Extracting it to a computed constant would improve maintainability.♻️ Suggested refactor
+ const hasUsersValidationErrors = usersValidation.disabledNext || hasUsersInputErrors; + <WizardStep name='Users' id='wizard-users' key='wizard-users' navItem={CustomStatusNavItem} status={ - !usersPristine && - (usersValidation.disabledNext || hasUsersInputErrors) + !usersPristine && hasUsersValidationErrors ? 'error' : 'default' } footer={ <CustomWizardFooter beforeNext={() => { - if (usersValidation.disabledNext || hasUsersInputErrors) { + if (hasUsersValidationErrors) { setUsersPristine(false); return false; } return true; }} disableNext={ - !usersPristine && - (usersValidation.disabledNext || hasUsersInputErrors) + !usersPristine && hasUsersValidationErrors } optional={true} isOnPremise={isOnPremise} /> } >src/Components/CreateImageWizard/steps/Users/components/UserInfo.tsx (1)
18-32: Effect dependency array may be incomplete.The effect at line 21 calls
onInputErrorsChange(line 30) but doesn't include it in the dependency array (line 32). While React'suseStatesetters are stable by guarantee, it's better practice to either:
- Include
onInputErrorsChangein the dependencies (if the callback is stable, no harm), or- Use a more targeted disable comment explaining why it's safe to omit
The current disable comment doesn't explain which specific dependency is intentionally omitted or why.
📝 Suggested improvement
Option 1 - Include the callback (safest):
React.useEffect(() => { const next: { [key: number]: boolean } = {}; // Only keep errors for indices that still exist for (let i = 0; i < users.length; i++) { if (inputErrorsRef.current[i]) { next[i] = true; } } inputErrorsRef.current = next; onInputErrorsChange?.(Object.values(inputErrorsRef.current).some(Boolean)); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [users.length]); + }, [users.length, onInputErrorsChange]);Option 2 - Document why it's safe to omit:
- // eslint-disable-next-line react-hooks/exhaustive-deps + // onInputErrorsChange (setState) is stable by React guarantee + // eslint-disable-next-line react-hooks/exhaustive-deps }, [users.length]);src/Components/CreateImageWizard/LabelInput.tsx (1)
53-61: Effect dependency may miss error message changes.Lines 59-61 use
validationErrors.lengthas a dependency, but this won't detect changes when the error message changes while the number of errors remains constant. For example, changing from one format error to another (both single errors) won't trigger the effect.Consider depending on the raw error string instead:
🔧 Proposed fix
+ const validationErrorText = stepValidation.errors[fieldName] || ''; const validationErrors = validationErrorText ? validationErrorText.split(/\s*\|\s*/).filter((e) => e.trim()) : []; // Notify parent about input error state (including "already exists" and format errors) useEffect(() => { onInputErrorChange?.(!!onStepInputErrorText || validationErrors.length > 0); - }, [onStepInputErrorText, validationErrors.length, onInputErrorChange]); + }, [onStepInputErrorText, validationErrorText, onInputErrorChange]);This ensures the parent is notified whenever the validation error string changes, not just when the count changes.
src/Components/CreateImageWizard/steps/Users/components/UserRow.tsx (1)
65-88: Clarify comment and consider simplifying the validation logic.Issue 1 - Misleading comment (lines 69-70):
The comment states "Groups and userName block navigation, userSshKey is shown for immediate feedback," but all three fields (groups,userName,userSshKey) contribute tostepValidation.disabledNextand thus block navigation. The comment should clarify that when pristine, these three errors are shown immediately while other errors (like password) are hidden until the user attempts to proceed.Issue 2 - Potentially redundant
hasGroupsErrorcheck (lines 81, 85-86):
ThehasGroupsErrorflag appears redundant in thedisabledNextcalculation (line 85-86). Groups validation errors are already included instepValidation.disabledNextfrom theuseUsersValidationhook. The only truly independent error source here ishasInputError, which tracks unsaved input inLabelInput. Consider removinghasGroupsErrorfrom line 86 unless there's a specific edge case where groups errors wouldn't be captured bystepValidation.disabledNext.📝 Suggested improvements
- // If pristine, show groups, userName, and userSshKey errors immediately - // Groups and userName block navigation, userSshKey is shown for immediate feedback + // If pristine, only show groups, userName, and userSshKey errors immediately. + // Other errors (e.g., password) are hidden until the user clicks Next. + // All errors contribute to blocking navigation via stepValidation.disabledNext. if (usersPristine) { ['groups', 'userName', 'userSshKey'].forEach((field) => { if (userErrors[field]) { errors[field] = userErrors[field]; } }); } else { // After Next clicked, show all errors Object.assign(errors, userErrors); } - const hasGroupsError = !!errors.groups; return { errors, disabledNext: - stepValidation.disabledNext || hasGroupsError || hasInputError, + stepValidation.disabledNext || hasInputError, };If there's a specific reason why
hasGroupsErroris needed separately, please add a comment explaining the edge case.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Components/CreateImageWizard/CreateImageWizard.tsxsrc/Components/CreateImageWizard/LabelInput.tsxsrc/Components/CreateImageWizard/steps/Users/components/UserInfo.tsxsrc/Components/CreateImageWizard/steps/Users/components/UserRow.tsxsrc/Components/CreateImageWizard/steps/Users/index.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
src/Components/CreateImageWizard/LabelInput.tsx (1)
src/store/hooks.ts (1)
useAppDispatch(6-6)
src/Components/CreateImageWizard/steps/Users/components/UserRow.tsx (2)
src/store/hooks.ts (1)
useAppDispatch(6-6)src/Components/CreateImageWizard/utilities/useValidation.tsx (1)
useUsersValidation(789-879)
src/Components/CreateImageWizard/steps/Users/components/UserInfo.tsx (2)
src/store/hooks.ts (1)
useAppSelector(7-7)src/store/wizardSlice.ts (1)
selectUsers(515-517)
⏰ 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:centos-stream-9-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-aarch64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: rpm-build:centos-stream-10-x86_64
- 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:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: Red Hat Konflux / image-builder-frontend-on-pull-request
- GitHub Check: Sourcery review
- GitHub Check: playwright-tests
- GitHub Check: Service Tests
🔇 Additional comments (13)
src/Components/CreateImageWizard/CreateImageWizard.tsx (2)
288-289: LGTM: State initialization is correct.The pristine state starts as
true(no interaction yet) and error state starts asfalse, which aligns with the validation pattern used for the filesystem step.
641-644: LGTM: Props correctly forwarded to UsersStep.The
usersPristinestate andonInputErrorsChangecallback are properly wired to enable the Users step to report validation errors back to the wizard.src/Components/CreateImageWizard/steps/Users/index.tsx (2)
7-10: LGTM: Type definition is clear and appropriate.The
UsersStepPropscorrectly defines the pristine state and optional error callback, matching the pattern used in the parent wizard.
12-25: LGTM: Props properly forwarded.The component correctly receives and forwards the validation props to
UserInfo, maintaining the error propagation chain.src/Components/CreateImageWizard/steps/Users/components/UserInfo.tsx (3)
11-16: LGTM: Type definition and component signature are correct.The component properly accepts the pristine state and error change callback, enabling centralized error tracking across user rows.
34-42: LGTM: Error change handler correctly updates and propagates state.The handler properly updates the per-index error map and computes the aggregate error state before notifying the parent.
73-84: LGTM: Props correctly forwarded to UserRow.The
usersPristineandonInputErrorChangeprops enable per-row error tracking and pristine-aware validation.src/Components/CreateImageWizard/LabelInput.tsx (3)
1-1: LGTM: New prop for error propagation.The
onInputErrorChangecallback enables the component to notify its parent about validation state, which is essential for the pristine-based validation flow.Also applies to: 31-31, 46-46
71-135: LGTM: Input trimming and validation improvements.The enhanced
addItemfunction correctly:
- Trims input to remove accidental whitespace
- Rejects empty values after trimming
- Checks trimmed values for duplicates (line 77)
- Validates the trimmed value (line 83)
- Returns a boolean to indicate success
- Notifies the parent of error state via
onInputErrorChangeThese changes prevent common input mistakes and enable proper error tracking.
144-148: LGTM: Blur handler addresses the core PR objective.The
onBlurhandler successfully implements the fix for issue #3943 by automatically adding non-empty input when the field loses focus. This handles the cases when:
- User clicks outside the field
- User clicks "Add user" (which blurs the input)
- User clicks "Next" (which blurs the input)
The implementation correctly uses
inputValue.trim()to avoid adding whitespace-only values.src/Components/CreateImageWizard/steps/Users/components/UserRow.tsx (3)
1-1: LGTM: New props and imports added correctly.The
usersPristineandonInputErrorChangeprops enable per-row error tracking and pristine-aware validation display.Also applies to: 42-43, 51-52
57-63: LGTM: Error state propagation is correctly implemented.The effect properly notifies the parent component whenever the input error state changes for this row, enabling the parent to aggregate errors across all rows.
237-237: LGTM: Error callback correctly wired to LabelInput.Passing
setHasInputErrordirectly as theonInputErrorChangecallback enablesLabelInputto report validation errors for unsaved input (e.g., a typed group name not yet added to the list). This is essential for blocking navigation when there's invalid pending input.
src/Components/CreateImageWizard/steps/Users/components/UserInfo.tsx
Outdated
Show resolved
Hide resolved
mgold1234
force-pushed
the
fix_user_be
branch
from
d1e44ae to
7374a51
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Components/CreateImageWizard/steps/Users/components/UserInfo.tsx (1)
76-84: Index-based keys will cause re-render issues when users are removed.Users can be removed via the
removeUseraction in the Redux store (lines 1411-1412 in wizardSlice.ts). When a user is removed, subsequent users' indices shift, but React usingindexas the key won't properly track which user is which—potentially causing form state to persist incorrectly. Users have no unique ID field (name, password, ssh_key, groups, etc. are the only properties). To fix this, consider either adding a stable unique identifier (e.g., UUID) to each user at creation, or refactoring the user management system to support proper tracking instead of index-based access.
🧹 Nitpick comments (4)
src/Components/CreateImageWizard/CreateImageWizard.tsx (1)
289-289: Consider aligning initialization pattern with filesystemPristine.The
usersPristineis initialized totrue, whereasfilesystemPristine(line 266-267) is initialized withfileSystemValidation.disabledNext. This difference means:
- Filesystem step shows errors immediately if the form starts in an invalid state
- Users step always starts pristine regardless of initial validation state
This is likely acceptable since users typically start with an empty (valid) users list, but worth noting for consistency.
src/Components/CreateImageWizard/steps/Users/components/UserInfo.tsx (1)
46-63: Consider simplifying thehasAnyErrorslogic for readability.The current logic is correct but the nested condition at lines 56-62 is somewhat hard to follow. The intent is: disable Add button if a user has errors AND (is defined OR has non-userName errors).
♻️ Optional: Clearer variable naming
- const hasAnyErrors = users.some((user, index) => { + const hasBlockingErrors = users.some((user, index) => { const userErrors = stepValidation.errors[index]; if (!userErrors || Object.keys(userErrors).length === 0) { return false; } - // If user is defined but has errors (including missing userName), disable Add button - // If user is completely empty and has "Required value" error, allow Add button - return ( - isUserDefined(user) || - Object.keys(userErrors).some( - (field) => - field !== 'userName' || userErrors[field] !== 'Required value', - ) - ); + const hasOnlyEmptyUserNameError = + Object.keys(userErrors).length === 1 && + userErrors.userName === 'Required value'; + + // Allow Add button only if user is completely empty with just the "Required value" error + return isUserDefined(user) || !hasOnlyEmptyUserNameError; });src/Components/CreateImageWizard/LabelInput.tsx (2)
55-58: Type narrowing foruseReduxStatecould be clearer.The
useReduxStatevariable is truthy when both props are provided, but TypeScript may not narrowonInputValueChangeinside the condition later. This works but could be slightly improved.♻️ Optional: More explicit type check
- const useReduxState = currentInputValue !== undefined && onInputValueChange; + const useReduxState = + currentInputValue !== undefined && onInputValueChange !== undefined;
67-70: Missing dependency inuseEffect-validationErrorsshould be included or memoized.The
useEffectdepends onvalidationErrorTextbut usesvalidationErrors.length. SincevalidationErrorsis derived on each render, this could cause stale closure issues or unnecessary re-runs. The logic works correctly here sincevalidationErrorsis derived fromvalidationErrorText, but the dependency array should reflect what's actually being used.♻️ Recommended: Align dependency with derived value
useEffect(() => { onInputErrorChange?.(!!onStepInputErrorText || validationErrors.length > 0); - }, [onStepInputErrorText, validationErrorText, onInputErrorChange]); + }, [onStepInputErrorText, validationErrors.length, onInputErrorChange]);Or memoize
validationErrors:const validationErrors = React.useMemo( () => validationErrorText ? validationErrorText.split(/\s*\|\s*/).filter((e) => e.trim()) : [], [validationErrorText] );
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/Components/CreateImageWizard/CreateImageWizard.tsxsrc/Components/CreateImageWizard/LabelInput.tsxsrc/Components/CreateImageWizard/steps/Users/components/UserInfo.tsxsrc/Components/CreateImageWizard/steps/Users/components/UserRow.tsxsrc/Components/CreateImageWizard/steps/Users/utilities/isUserDefined.tssrc/Components/CreateImageWizard/utilities/requestMapper.tssrc/Components/CreateImageWizard/utilities/useValidation.tsxsrc/store/wizardSlice.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Components/CreateImageWizard/steps/Users/components/UserRow.tsx
🧰 Additional context used
🧬 Code graph analysis (5)
src/Components/CreateImageWizard/LabelInput.tsx (1)
src/store/hooks.ts (1)
useAppDispatch(6-6)
src/Components/CreateImageWizard/steps/Users/utilities/isUserDefined.ts (1)
src/store/wizardSlice.ts (1)
UserWithAdditionalInfo(61-66)
src/Components/CreateImageWizard/steps/Users/components/UserInfo.tsx (4)
src/store/hooks.ts (1)
useAppSelector(7-7)src/store/wizardSlice.ts (1)
selectUsers(535-537)src/Components/CreateImageWizard/steps/Users/components/UserRow.tsx (1)
UsersContext(39-39)src/Components/CreateImageWizard/steps/Users/utilities/isUserDefined.ts (1)
isUserDefined(11-18)
src/Components/CreateImageWizard/CreateImageWizard.tsx (1)
src/Components/CreateImageWizard/steps/Users/components/UserRow.tsx (1)
UsersContext(39-39)
src/Components/CreateImageWizard/utilities/useValidation.tsx (1)
src/Components/CreateImageWizard/validators.ts (1)
isUserGroupValid(100-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). (21)
- 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: 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: Service Tests
- GitHub Check: playwright-tests
🔇 Additional comments (18)
src/Components/CreateImageWizard/utilities/requestMapper.ts (1)
351-360: LGTM!The initialization of
currentGroupInput: ''for users loaded from blueprints correctly aligns with the new per-user field inwizardSlice.ts, ensuring consistent state shape.src/Components/CreateImageWizard/steps/Users/utilities/isUserDefined.ts (1)
1-18: LGTM!The utility correctly identifies when a user has started filling in any field, including the new
currentGroupInputfor tracking in-progress group input. The optional chaining and trim ensure safe handling of undefined or whitespace-only values.src/Components/CreateImageWizard/CreateImageWizard.tsx (1)
617-639: LGTM!The Users step correctly implements the pristine pattern:
- Status shows error only after user interaction clears pristine state
beforeNextblocks navigation and reveals errors when validation failsUsersContext.Providercorrectly passes the pristine state for child components to consumesrc/Components/CreateImageWizard/utilities/useValidation.tsx (2)
803-809: LGTM!The change to always validate
userNamefor every user in the list is correct. If a user row exists, it must have a valid name regardless of whether other fields are filled.
839-878: LGTM!The validation for
currentGroupInputcorrectly handles three key scenarios:
- Invalid format in the interim input
- Duplicate detection against existing groups
- Prevention of group matching the username
This ensures users get immediate feedback on their typing before committing the group value, aligning with the PR's objective to prevent data loss on blur/Add user/Next actions.
src/store/wizardSlice.ts (4)
61-66: LGTM!The type extension correctly adds
currentGroupInput: stringtoUserWithAdditionalInfo, enabling per-user tracking of interim group input state.
93-96: LGTM!The payload type follows the established pattern for user-indexed operations.
1398-1410: LGTM!The
addUserreducer correctly initializescurrentGroupInput: ''for new users, ensuring consistent state shape.
1470-1476: LGTM!The
setCurrentGroupInputByIndexreducer correctly updates the per-user group input state, following the same pattern as other user setters likesetUserNameByIndex.src/Components/CreateImageWizard/steps/Users/components/UserInfo.tsx (4)
5-10: LGTM on imports.The imports are correctly structured -
UsersContextfromUserRowandisUserDefinedutility are properly imported to support the new validation logic.
14-28: LGTM on context usage and default user initialization.Good addition of
usersPristinefrom context andcurrentGroupInput: ''to the default user object, aligning with the Redux store structure introduced in this PR.
36-44: Logic forhasDefinedUserWithoutNameis correct.This correctly identifies users who have started filling out fields (groups, password, SSH key, or group input) but are missing a username - exactly what's needed to show the validation alert proactively.
67-68: Alert display condition logic is sound.The alert shows when:
- Step is no longer pristine AND any user is missing a username, OR
- Any defined user is missing a username (shows immediately without waiting for "dirty" state)
This provides good UX by alerting users proactively when they've started defining a user but forgot the username.
src/Components/CreateImageWizard/LabelInput.tsx (5)
31-35: LGTM on new props interface.The new props cleanly extend the component to support either local state or Redux-based state management, making it flexible for different use cases.
84-94: LGTM onaddItemrefactor with trimming and duplicate check.Good improvements:
- Returns boolean for success/failure status
- Uses
trimmedValueconsistently- Notifies parent about errors synchronously
143-151: LGTM on successful add flow.Correctly dispatches the trimmed value, clears both Redux and local state appropriately, and notifies parent that errors are cleared.
161-165: Core fix for the PR objective - LGTM.The
handleBlurcorrectly triggersaddItemwhen there's input to add. This addresses the main issue where group values were only added on Enter.
177-184: Potential UX concern:onBlurfires on every focus loss.Per the PR comments, the UX team should be consulted. When the input loses focus (e.g., user clicks elsewhere, switches tabs), the current value is auto-added. If the value is invalid, it will show an error and clear the input. If valid, it gets added immediately. This might surprise users who intended to continue editing.
Consider whether this behavior aligns with UX expectations, especially for partially typed inputs.
As noted in the PR discussion, this behavior change may need UX team approval. The current implementation:
- Adds valid input on blur (which may surprise users)
- Shows error and clears invalid input on blur (data loss concern)
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
mgold1234
force-pushed
the
fix_user_be
branch
from
7374a51 to
037f798
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Components/CreateImageWizard/utilities/useValidation.tsx`:
- Around line 839-847: The optional chain on currentGroupInput is unnecessary
because UserWithAdditionalInfo.currentGroupInput is a required string; update
the validation block to access and trim currentGroupInput directly (use
users[index].currentGroupInput.trim()), and remove the optional chaining in the
derived booleans (currentInputAlreadyInGroups and currentInputMatchesUsername)
while keeping the same truthy checks (e.g., currentInput &&
!isUserGroupValid(currentInput), currentInput &&
users[index].groups.includes(currentInput), currentInput && currentInput ===
users[index].name) so eslint no-longer flags an unnecessary optional chain on
currentGroupInput.
🧹 Nitpick comments (3)
src/store/wizardSlice.ts (1)
1479-1485: Consider adding bounds checking for index access.The reducer directly accesses
state.users[action.payload.index]without verifying that the index is valid. If an invalid index is passed, this could cause a runtime error.🛡️ Proposed defensive check
setCurrentGroupInputByIndex: ( state, action: PayloadAction<UserCurrentGroupInputPayload>, ) => { + if (action.payload.index < 0 || action.payload.index >= state.users.length) { + return; + } state.users[action.payload.index].currentGroupInput = action.payload.input; },src/Components/CreateImageWizard/CreateImageWizard.tsx (1)
617-635: Inconsistency with filesystem step pattern:disableNextnot gated by pristine.The filesystem step (lines 550-551) uses
!filesystemPristine && fileSystemValidation.disabledNextfordisableNext, allowing users to click Next even with errors until they've attempted once. The Users step here keepsdisableNext={usersValidation.disabledNext}without the pristine check.This creates inconsistent UX: on filesystem, Next is clickable until you try; on Users, Next may be disabled immediately.
♻️ Proposed fix for consistency
footer={ <CustomWizardFooter beforeNext={() => { if (usersValidation.disabledNext) { setUsersPristine(false); return false; } return true; }} - disableNext={usersValidation.disabledNext} + disableNext={ + !usersPristine && usersValidation.disabledNext + } optional={true} isOnPremise={isOnPremise} /> }src/Components/CreateImageWizard/LabelInput.tsx (1)
31-35: Consider gating blur-to-add behavior behind an opt-in prop for explicit control.The
onBlurhandler unconditionally triggers item addition across allLabelInputconsumers (ports, services, NTP servers, kernel arguments, and groups). While this provides consistent UX across fields, adding an explicitaddOnBlurprop would make the behavior intentional and allow future flexibility if needed.♻️ Suggested refactor (opt‑in blur behavior)
type LabelInputProps = { ariaLabel: string; placeholder: string; validator: (value: string) => boolean; requiredList?: string[] | undefined; requiredCategoryName?: string; list: string[] | undefined; item: string; addAction: (value: string) => UnknownAction; removeAction: (value: string) => UnknownAction; stepValidation: StepValidation; fieldName: string; onInputErrorChange?: (hasError: boolean) => void; // For using Redux state instead of local state (groups in Users step) currentInputValue?: string; onInputValueChange?: (value: string) => UnknownAction; + addOnBlur?: boolean; }; const LabelInput = ({ ariaLabel, placeholder, validator, list, requiredList, requiredCategoryName, item, addAction, removeAction, stepValidation, fieldName, onInputErrorChange, currentInputValue, onInputValueChange, + addOnBlur = false, }: LabelInputProps) => { @@ const handleBlur = () => { + if (!addOnBlur) return; if (inputValue.trim()) { addItem(inputValue); } };
mgold1234
force-pushed
the
fix_user_be
branch
from
a7a2d83 to
e96d61f
Compare
|
/jira-epic HMS-9973 |
schutzbot
changed the title
regexowl
left a comment
regexowl
left a comment
There was a problem hiding this comment.
Ah, I see. I've started reviewing, but there seems to be older users step than we have currently. It's really weird that the PR doesn't warn about conflicts, but there was a bigger change to the way the users table is rendered.
Before (and what I see in this PR):
And after (what is currently on stage after SPUR changes):
Could you please rebase so the code is up-to-date with main? I'll continue with the review after the rebase.
src/Components/CreateImageWizard/steps/Users/utilities/isUserDefined.ts
Outdated
Show resolved
Hide resolved
src/Components/CreateImageWizard/steps/Users/utilities/isUserDefined.ts
Outdated
Show resolved
Hide resolved
src/Components/CreateImageWizard/steps/Users/components/UserInfo.tsx
Outdated
Show resolved
Hide resolved
mgold1234
force-pushed
the
fix_user_be
branch
2 times, most recently
from
4142a3a to
9196335
Compare
|
/retest |
1 similar comment
|
/retest |
|
Noticed some weird behaviour. When there's more groups the "Add user" and "Next" button don't "work" any more unless the new group is added first. So realistically - the user would have to click the Next or Add button twice to do the action they wanted to do: Screencast_20260212_100920.webmCan you also please squash the commits so the |
mgold1234
force-pushed
the
fix_user_be
branch
3 times, most recently
from
aad0044 to
32c7ebd
Compare
lucasgarfield
left a comment
lucasgarfield
left a comment
There was a problem hiding this comment.
The bug fix is needed — group values shouldn't be silently dropped on Next. But the approach has problems.
The core issue: this PR coordinates input commitment through DOM events (document.activeElement.blur() + onMouseDown preventDefault) instead of explicit state management. This creates a race condition where beforeNext reads stale validation state, and breaks keyboard accessibility.
What needs to change:
- Replace DOM manipulation with explicit input commitment in
beforeNext - Remove
onMouseDown preventDefault(accessibility regression) - Fix the controlled/uncontrolled mode switching in
LabelInput - Remove
useEffectthat syncs derived error state to parent - Add tests for the new blur behavior
The ?? refactoring and input trimming are fine. Details inline.
src/Components/CreateImageWizard/steps/Users/components/UserInfo.tsx
Outdated
Show resolved
Hide resolved
mgold1234
force-pushed
the
fix_user_be
branch
3 times, most recently
from
abe7e45 to
3ced858
Compare
regexowl
left a comment
regexowl
left a comment
There was a problem hiding this comment.
Ran through the code and added some comments.
Also noticed two possible bugs:
- Invalid groups get silently dropped on "Add user" and "Next"
- Duplicate groups get silently dropped on "Add user" and "Next"
src/Components/CreateImageWizard/steps/Users/components/GroupRow.tsx
Outdated
Show resolved
Hide resolved
mgold1234
force-pushed
the
fix_user_be
branch
6 times, most recently
from
1657e88 to
4d711cc
Compare
This commit fixes a bug where group field value was only added to the list on Enter key press. Now the value is added to the list also when: - Blurring the field (onBlur) - Clicking "Add user" button - Clicking "Next" button Store pending group input in Redux (pendingGroupInput) and commit it explicitly via commitAllPendingGroupInputs on Enter, Add User, and Next. Use relatedTarget in handleBlur to skip addItem when focus moves to a button, avoiding DOM shifts that cause double-click issues.
|
this will be address in a simple pr -#4225 |
3 participants
This commit fixes a bug where group field value was only added to the list on Enter key press. Now the value is added to the list also when:
Fixes: #3943
JIRA: HMS-10148