ACM-30606 Wizard review collapse, link, highlight#5957
Open
jeswanke wants to merge 4 commits intostolostron:mainfrom
Open
ACM-30606 Wizard review collapse, link, highlight#5957jeswanke wants to merge 4 commits intostolostron:mainfrom
jeswanke wants to merge 4 commits intostolostron:mainfrom
Conversation
Signed-off-by: John Swanke <jswanke@redhat.com> Signed-off-by: John Swanke <jswanke@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jeswanke The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughAdds a review-step subsystem (input registration, DOM-tree building, review UI, edit/navigation handlers, persisted expand/collapse state, and editor-path highlighting) and removes DisplayMode.Details rendering across many input components; also injects wizard IDs and wires editor highlight propagation into SyncEditor consumers. Changes
Sequence Diagram(s)sequenceDiagram
participant Input as Input Component
participant Reg as StepInputsRegistry
participant Sync as ReviewDomTreeSync
participant Step as Step/container (buildTree)
participant Review as ReviewStep UI
Input->>Reg: register(inputId, meta{path,label,value,type})
Reg-->>Sync: bump review version
Sync->>Step: reviewDomTreeVersion changed
Step->>Step: buildTree(container, inputMap, {stepId,label})
Step->>Reg: setStepTree(stepId, tree)
Reg-->>Review: updated step trees available
Review->>Review: render sections/collapses/badges
User->>Review: click pen (navigate)
Review->>Step: compute target domId & highlight path
Step->>Input: scroll/focus input and apply DOM highlight
Step->>Review: setHighlightEditorPath(path) via context
sequenceDiagram
participant ReviewZone as ReviewPenHoverZone
participant User as User
participant Nav as useReviewEditHandler
participant Scroll as scrollReviewEditTargetIntoView
participant Editor as SyncEditor
User->>ReviewZone: hover -> reveal pen (delayed)
User->>ReviewZone: click pen (intent: navigate)
ReviewZone->>Nav: onPenClick(node,'navigate')
Nav->>Nav: determine stepId & domId & editor path
Nav->>Scroll: scrollReviewEditTargetIntoView(domId)
Scroll->>Scroll: focus/select target, add highlight classes
Nav->>Editor: setHighlightEditorPath(path) (via context)
Editor->>Editor: rangeForHighlightPath -> decorate range in Monaco
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
frontend/packages/react-form-wizard/src/inputs/WizStringsInput.tsx (2)
102-114:⚠️ Potential issue | 🟠 MajorSame mutation issue in
StringsMapInput.
values.push('')andvalues.splice()mutate the array directly. Apply the same immutable pattern here.🐛 Suggested fix
const onNewKey = () => { - values.push('') - let newValue = values + const newValues = [...values, ''] + let newValue: string[] | any = newValues if (props.unmap) newValue = props.unmap(values) setValue(newValue) } const onDeleteKey = (index: number) => { - values.splice(index, 1) - let newValue = values + const newValues = values.filter((_, i) => i !== index) + let newValue: string[] | any = newValues if (props.unmap) newValue = props.unmap(values) setValue(newValue) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/react-form-wizard/src/inputs/WizStringsInput.tsx` around lines 102 - 114, onNewKey and onDeleteKey in WizStringsInput currently mutate the values array (using values.push and values.splice); change them to use immutable updates: for onNewKey create a new array (e.g. const updated = [...values, '']) and for onDeleteKey create a new array without the removed index (e.g. const updated = values.filter((_, i) => i !== index) or use slices/concat), then if props.unmap apply props.unmap(updated) and call setValue(with the resulting new array).
20-28:⚠️ Potential issue | 🟠 MajorPotential state issue: mutating array before
setValue.
values.push('')andvalues.splice()mutate the array in place. Sincevaluesis derived fromvalue, you're mutating the existing state reference before callingsetValue. React may not detect the change if the reference is unchanged.🐛 Suggested fix using spread/filter
const onNewKey = () => { - values.push('') - setValue(values) + setValue([...values, '']) } const onDeleteKey = (index: number) => { - values.splice(index, 1) - setValue(values) + setValue(values.filter((_, i) => i !== index)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/react-form-wizard/src/inputs/WizStringsInput.tsx` around lines 20 - 28, onNewKey and onDeleteKey currently mutate the existing values array (values.push and values.splice) which can keep the same reference and prevent React from noticing updates; change them to create new arrays before calling setValue (e.g., in onNewKey return a new array with the added empty string and call setValue(newArray); in onDeleteKey create a filtered or sliced copy that omits the index and call setValue(copy)) and avoid mutating values in place.frontend/packages/react-form-wizard/src/inputs/WizTableSelect.tsx (1)
57-75:⚠️ Potential issue | 🔴 CriticalBug:
pagedItemsused instead ofiteminonSelect.Line 63 uses
pagedItems(the entire page array) instead ofitem(the single selected item) whenprops.itemToValueis falsy. This will add all paged items instead of just the selected one.🐛 Fix
setValue([ ...(props.itemToValue ? selectedItems.map(props.itemToValue) : selectedItems), - props.itemToValue ? props.itemToValue(item) : pagedItems, + props.itemToValue ? props.itemToValue(item) : item, ])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/react-form-wizard/src/inputs/WizTableSelect.tsx` around lines 57 - 75, In the onSelect callback for WizTableSelect, fix the branch that appends a newly selected item when props.itemToValue is falsy: replace the incorrect use of pagedItems with the single item variable so setValue appends item (not the whole pagedItems array); update the code path that currently builds the new value from selectedItems and pagedItems to instead append item, keeping the existing behavior when props.itemToValue is truthy (use props.itemToValue(item)). Ensure you modify the onSelect function (references: onSelect, selectedItems, setValue, props.itemToValue, pagedItems, item) accordingly.frontend/packages/react-form-wizard/src/inputs/WizTimeRange.tsx (1)
39-48:⚠️ Potential issue | 🟠 MajorFix
onChangecallback to receive time string, not event object.PatternFly's
TimePicker.onChangesignature is(event, time, hour?, minute?, seconds?, isValid?) => void. The current callback at line 42 destructures only the first parameter (value) which is the event object, not the time string. This causesset(item, path, value)to store a React event instead of the time value.Pass the second parameter instead:
onChange={(_event, time) => { set(item, path, time) update() }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/react-form-wizard/src/inputs/WizTimeRange.tsx` around lines 39 - 48, The TimePicker onChange handler currently treats the first parameter as the time value, but PatternFly's TimePicker.onChange signature provides the time string as the second parameter; update the onChange callback for the TimePicker component (identified by id/ key using id and the TimePicker JSX) to accept (event, time) and call set(item, path, time) followed by update() so you store the actual time string instead of the React event object.
🧹 Nitpick comments (8)
frontend/src/components/SyncEditor/SyncEditor.css (1)
75-78: Consider using PatternFly color tokens and verify highlight color contrast.The
rgba(255, 190, 140, 0.38)highlight uses a custom color instead of design tokens. For accessibility, verify this provides sufficient contrast for users with color vision deficiencies when displayed over syntax-highlighted text. The!importantoverride is appropriate for Monaco editor decorations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/SyncEditor/SyncEditor.css` around lines 75 - 78, The .syncEditorYamlHighlight CSS class uses a hardcoded rgba highlight; replace that with a PatternFly color token (e.g., use a CSS variable like var(--pf-global--palette--gold-200) or an appropriate --pf-global--info/warning token) while keeping the !important override for Monaco decorations, and then verify the resulting highlight over your syntax colors meets WCAG contrast (adjust token choice or opacity as needed to reach accessible contrast ratios for text).frontend/packages/react-form-wizard/src/inputs/WizKeyValue.tsx (1)
17-67: Consider extracting the repeated reducer logic.The
pairs.reduce(...)pattern converting pairs to an object is duplicated four times. A helper function would reduce repetition, though this is optional.♻️ Suggested helper extraction
+const pairsToObject = (pairs: { key: string; value: string }[]) => + pairs.reduce<Record<string, string>>((result, pair) => { + result[pair.key] = pair.value + return result + }, {}) + const onKeyChange = (index: number, newKey: string) => { pairs[index].key = newKey - setValue( - pairs.reduce( - (result, pair) => { - result[pair.key] = pair.value - return result - }, - {} as Record<string, string> - ) - ) + setValue(pairsToObject(pairs)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/react-form-wizard/src/inputs/WizKeyValue.tsx` around lines 17 - 67, Extract the repeated pairs-to-record reducer into a small helper (e.g., function pairsToRecord(pairs: {key:string,value:string}[]): Record<string,string>) and replace the inline pairs.reduce(...) calls inside onKeyChange, onValueChange, onNewKey, and onDeleteKey with a single call to that helper; ensure you still update pairs (assignment, push, splice) before calling pairsToRecord and then pass its return to setValue to preserve existing behavior.frontend/packages/react-form-wizard/src/Wizard.tsx (1)
173-181: Avoid persisting to a shared fallback bucket.When both
idandreviewStorageKeyare omitted, every wizard resolves towizard-default. That makes collapse state bleed across unrelated wizards. Consider disabling persistence until a stable scope is provided.Also applies to: 197-197
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/react-form-wizard/src/Wizard.tsx` around lines 173 - 181, The function defaultReviewStorageKeyFromId currently falls back to "wizard-default" when id is empty causing unrelated wizards to share storage; change it so when the sanitized id is empty it returns an empty string (or null/undefined depending on the surrounding API) to disable persistence instead of returning a shared "wizard-default". Update defaultReviewStorageKeyFromId (and the other resolver used around the reviewStorageKey resolution at the other site) to check if suffix === '' and return a non-persisting sentinel (empty string/null/undefined) rather than `wizard-default`, keeping MAX_REVIEW_STORAGE_KEY_LEN behavior for non-empty keys.frontend/packages/react-form-wizard/src/review/ReviewStep.tsx (2)
282-293: Silent catch on localStorage write.The empty catch block silently swallows all errors. Consider logging in development mode for debugging purposes while still suppressing in production.
Optional: Add dev-mode logging
} catch { - /* ignore quota / private mode */ + /* ignore quota / private mode */ + if (process.env.NODE_ENV === 'development') { + console.debug('Failed to write review expandable state to localStorage') + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/react-form-wizard/src/review/ReviewStep.tsx` around lines 282 - 293, The catch block in writeReviewExpandableStorage silently swallows errors from localStorage.setItem; update the function to catch the error and, when in development (e.g., check process.env.NODE_ENV !== 'production' or a similar runtime flag), log a descriptive message including the error and the reviewExpandableStorageKey(reviewStorageKey) value and the sections/lastToolbar payload for debugging, while continuing to suppress/ignore the error in production to avoid leaking exceptions to users.
719-724: Controlled/uncontrolled pattern is correct but has redundant check.Line 721 uses
props.isExpanded!with non-null assertion. TheisControlledcheck guaranteesisExpanded !== undefined, so the assertion is technically safe but could be cleaner.Minor: Use nullish coalescing for clarity
- const isExpanded = isControlled ? props.isExpanded! : localExpanded + const isExpanded = isControlled ? props.isExpanded : localExpandedTypeScript should narrow this correctly after the
isControlledcheck.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/react-form-wizard/src/review/ReviewStep.tsx` around lines 719 - 724, Replace the non-null assertion on props.isExpanded with a clean narrowing expression: compute isControlled as before, then set isExpanded using either the controlled prop or local state without using "!" — e.g. const isExpanded = isControlled ? props.isExpanded : localExpanded or const isExpanded = props.isExpanded ?? localExpanded; likewise remove any "!" usage for props.onExpandedChange in onToggle and rely on the isControlled check to call props.onExpandedChange(expanded).frontend/packages/react-form-wizard/src/review/ReviewStepContexts.tsx (2)
108-108:get()returns ref, not map directly.The
getmethod returnsinputMapRef(theMutableRefObject) rather thaninputMapRef.current. This means consumers must access.currentto get the actual Map. The type signature() => MutableRefObject<Map<...>>is correct but slightly unusual - typically a getter would return the value directly.Consider returning the map directly if ref stability isn't needed by consumers
- const get = useCallback(() => inputMapRef, []) + const get = useCallback(() => inputMapRef.current, [])And update the type:
- get: () => MutableRefObject<Map<string, InputReviewStepMeta>> + get: () => Map<string, InputReviewStepMeta>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/react-form-wizard/src/review/ReviewStepContexts.tsx` at line 108, The get function currently returns the ref object (inputMapRef) instead of the Map value; change the useCallback returned value in get to return inputMapRef.current so consumers receive the Map directly, and update the get function's type signature accordingly (or, if you need to preserve returning a ref for stability, rename the function to reflect that behavior). Ensure references to get elsewhere expect a Map (not a MutableRefObject) and adjust imports/usages if necessary.
177-177:getStepscreates new sorted array on each call.Since
getStepsis wrapped inuseCallbackwith empty deps but creates a new sorted array every invocation, frequent calls could allocate unnecessarily. If this is called in render paths, consider memoizing the sorted result whenversionchanges.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/react-form-wizard/src/review/ReviewStepContexts.tsx` at line 177, getSteps currently builds and sorts a new array on every call (defined as const getSteps = useCallback(...)), causing unnecessary allocations; replace this with a memoized sorted array and return a stable accessor. Specifically, compute the sorted steps once using useMemo keyed on the appropriate change signal (e.g., version or an explicit stepsVersion) and mapRef (or a ref-backed mutation counter), then have getSteps return that memoized array (or remove getSteps and expose the memoized steps directly). Update references to getSteps accordingly and keep the sort logic (Array.from(mapRef.current.values()).sort((a,b)=>a.id.localeCompare(b.id))) when building the memo so the result only recomputes when version/stepsVersion changes.frontend/packages/react-form-wizard/src/review/utils.ts (1)
45-94: Recursive DOM traversal without depth limit.
buildReviewSubtreerecursively walks the entire DOM subtree. For deeply nested wizard forms, this could cause performance issues or (in extreme cases) stack overflow. Consider adding a depth limit or iterative approach if wizard forms can become deeply nested.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/react-form-wizard/src/review/utils.ts` around lines 45 - 94, buildReviewSubtree currently recurses the DOM with no depth limit which risks deep-stack or performance issues; modify buildReviewSubtree to accept a maxDepth (e.g., add a parameter maxDepth: number = 100) and short-circuit recursion when depth <= 0, passing depth-1 to recursive calls (or alternatively implement an iterative stack-based traversal inside buildReviewSubtree that tracks current depth and skips pushing children once maxDepth is reached); update all internal calls that call buildReviewSubtree (including the initial entry point) to provide the chosen maxDepth and ensure behavior for nodes skipped by depth limiting is defined (e.g., treat them as having no children).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/packages/react-form-wizard/src/contexts/StringContext.tsx`:
- Around line 34-35: The new required fields reviewExpandAllTooltip and
reviewCollapseAllTooltip on the exported interface WizardStrings are
source-breaking for external consumers; change those two properties to optional
(e.g., reviewExpandAllTooltip?: string and reviewCollapseAllTooltip?: string) in
the WizardStrings interface in StringContext.tsx and ensure any internal usage
continues to rely on ReviewStepToolbar's existing defaults rather than assuming
the properties are present.
In `@frontend/packages/react-form-wizard/src/inputs/WizArrayInput.tsx`:
- Around line 468-480: The effect in useLayoutEffect (WizArrayInput.tsx)
registers inputs via stepInputsRegistry.register but does not bumpReviewDomTree
on cleanup, leaving stale entries when the last array item is removed; update
the cleanup returned by the effect to call stepInputsRegistry.unregister(id) and
then call bumpReviewDomTree?.() so the reviewDomTreeVersion (used by Step.tsx)
is incremented on unregister as well, ensuring the review tree is rebuilt
immediately after item removal.
- Around line 181-201: The mapping in WizArrayInput uses key={index} which
breaks state when items are sorted/removed; change the key on the ArrayInputItem
to a stable identifier (e.g., use value.id or value._id from the mapped
`value`), and if items may not have an id, assign a persistent uid to each item
when they are added (store it on the item object or in component state) and use
that uid as the key instead of index so ArrayInputItem,
moveUp/moveDown/removeItem and their expanded/collapsed state remain stable
across reorders.
- Around line 218-227: The MenuToggle currently nests a Button (see Dropdown
usage with MenuToggle and Button, PlusCircleIcon and props.placeholder), which
violates PatternFly accessibility rules; replace the nested Button by moving the
icon and label into MenuToggle's own props/children: remove the Button element
inside the MenuToggle in the WizArrayInput Dropdown, pass icon={<PlusCircleIcon
/>} to MenuToggle and keep {props.placeholder} as its child, preserving
ref={toggleRef}, onClick={onToggle}, variant="plainText" and the existing
isOpen/open and onOpenChange/setOpen behavior.
In `@frontend/packages/react-form-wizard/src/inputs/WizAsyncSelect.tsx`:
- Around line 84-125: The SyncButton in WizAsyncSelect currently calls sync(),
which early-returns unless displayMode === DisplayMode.Step, causing the control
to be inert in review mode; update sync() (or its caller) so the async load path
can run in non-step displays: either add a non-step summary branch that
fetches/options-populates when displayMode !== DisplayMode.Step, or remove/relax
the early return in sync() so asyncCallback is honored in review mode; ensure
SyncButton's onClick (and any gating around props.asyncCallback and loading)
triggers the same fetch/update logic used in step mode and that value/options
state (used by PfSelect, SelectListOptions, handleSetOptions) are updated
accordingly.
In `@frontend/packages/react-form-wizard/src/inputs/WizFormGroup.tsx`:
- Around line 18-19: WizFormGroup currently builds registrationPath via
buildReviewInputRegistrationPath but doesn't apply the same props.id suffix
logic used by useInput (see Input.ts useInput and convertId), causing
FormGroup.fieldId to mismatch rendered control ids (e.g., WizCheckbox) and
breaking a11y; update WizFormGroup to derive its id the same way as useInput
(apply props.id suffix to registrationPath or accept/propagate the final id from
child controls) so FormGroup.fieldId equals the actual control id, and also
guard any (window as any).Cypress checks with typeof window !== 'undefined' to
avoid SSR/runtime errors. Ensure you modify references to registrationPath,
convertId, and the Cypress check accordingly so both WizFormGroup and child
inputs produce identical ids.
In `@frontend/packages/react-form-wizard/src/review/ReviewStep.tsx`:
- Around line 697-700: The comment for reviewArrayInstanceMarginLeft is
inaccurate: it claims each nested ARRAY_INPUT adds 16px while the implementation
returns 32 + 2 * arrayInputNesting (i.e., 2px per nesting). Update the comment
above function reviewArrayInstanceMarginLeft (and mention arrayInputNesting) to
state that the top-level array section uses 32px and each nested ARRAY_INPUT
adds 2px, so the function returns 32 + 2 * arrayInputNesting; keep the logic
unchanged.
In `@frontend/packages/react-form-wizard/src/review/ReviewStepNavigation.tsx`:
- Line 342: The code is unsafely casting a KeyboardEvent to a ReactMouseEvent
when calling onPenClick; update the handlers so you don't force-cast events:
either add a separate keyboard handler (e.g., onPenKeyPress) that calls the same
logic for keyboard activation, or change onPenClick's signature to accept a
union type (React.MouseEvent<HTMLElement> | React.KeyboardEvent<HTMLElement>)
and branch on event type inside onPenClick to use mouse-specific properties only
when available; locate usages of onPenClick and the call site that currently
does onPenClick(e as unknown as ReactMouseEvent<HTMLElement>) and replace with
the appropriate typed handler or a safe call that does not cast events.
- Around line 124-132: The variables easeOutFallbackId and autoDismissId are
declared after they're first used, creating a temporal-dead-zone/hoisting risk;
move the declarations for let easeOutFallbackId: ReturnType<typeof setTimeout> |
undefined and let autoDismissId: ReturnType<typeof setTimeout> | undefined to
before any use (before the setTimeout call and before the dismiss() function
references) in ReviewStepNavigation.tsx so dismiss(), the setTimeout that uses
autoDismissId, and any fallback logic reference already-declared variables;
ensure references to REVIEW_EDIT_TARGET_HIGHLIGHT_AUTO_DISMISS_MS remain
unchanged.
- Around line 401-410: The non-dl-term branch rendering <Comp
className={zoneClassName} ...> is missing the keyboard handler; add the same
onKeyDown prop that the dl-term variant uses (i.e., wire the component to the
same onKeyDown handler used elsewhere in this file) and ensure the element is
keyboard-focusable (add tabIndex={0} if not already present) so onZoneClick can
be invoked via keyboard; keep existing props (style, onMouseEnter/onMouseLeave,
onClick) and use the same handler name used in the dl-term variant for
consistency.
In `@frontend/src/components/SyncEditor/decorate.ts`:
- Around line 258-275: The loop inside tryKind() returns on the first iteration
so only mappings[kind][0] is considered; change the logic to iterate through all
items in arr, compute fromPaths for each (using the same pathKey/pathItemKey
logic and mappingLeaf lookup), and only return mappingLeafToRange(monaco,
fromPaths) when a valid fromPaths (or a successful mappingLeafToRange result) is
found; if none found after the loop, return undefined (or
mappingLeafToRange(monaco, undefined) if that matches existing behavior). Ensure
you update the return location (move it outside the for loop) and keep the same
helpers/variables (arr, pathKey, pathArrayKey, pathItemKey, fromPaths, paths,
mappingLeafToRange).
---
Outside diff comments:
In `@frontend/packages/react-form-wizard/src/inputs/WizStringsInput.tsx`:
- Around line 102-114: onNewKey and onDeleteKey in WizStringsInput currently
mutate the values array (using values.push and values.splice); change them to
use immutable updates: for onNewKey create a new array (e.g. const updated =
[...values, '']) and for onDeleteKey create a new array without the removed
index (e.g. const updated = values.filter((_, i) => i !== index) or use
slices/concat), then if props.unmap apply props.unmap(updated) and call
setValue(with the resulting new array).
- Around line 20-28: onNewKey and onDeleteKey currently mutate the existing
values array (values.push and values.splice) which can keep the same reference
and prevent React from noticing updates; change them to create new arrays before
calling setValue (e.g., in onNewKey return a new array with the added empty
string and call setValue(newArray); in onDeleteKey create a filtered or sliced
copy that omits the index and call setValue(copy)) and avoid mutating values in
place.
In `@frontend/packages/react-form-wizard/src/inputs/WizTableSelect.tsx`:
- Around line 57-75: In the onSelect callback for WizTableSelect, fix the branch
that appends a newly selected item when props.itemToValue is falsy: replace the
incorrect use of pagedItems with the single item variable so setValue appends
item (not the whole pagedItems array); update the code path that currently
builds the new value from selectedItems and pagedItems to instead append item,
keeping the existing behavior when props.itemToValue is truthy (use
props.itemToValue(item)). Ensure you modify the onSelect function (references:
onSelect, selectedItems, setValue, props.itemToValue, pagedItems, item)
accordingly.
In `@frontend/packages/react-form-wizard/src/inputs/WizTimeRange.tsx`:
- Around line 39-48: The TimePicker onChange handler currently treats the first
parameter as the time value, but PatternFly's TimePicker.onChange signature
provides the time string as the second parameter; update the onChange callback
for the TimePicker component (identified by id/ key using id and the TimePicker
JSX) to accept (event, time) and call set(item, path, time) followed by update()
so you store the actual time string instead of the React event object.
---
Nitpick comments:
In `@frontend/packages/react-form-wizard/src/inputs/WizKeyValue.tsx`:
- Around line 17-67: Extract the repeated pairs-to-record reducer into a small
helper (e.g., function pairsToRecord(pairs: {key:string,value:string}[]):
Record<string,string>) and replace the inline pairs.reduce(...) calls inside
onKeyChange, onValueChange, onNewKey, and onDeleteKey with a single call to that
helper; ensure you still update pairs (assignment, push, splice) before calling
pairsToRecord and then pass its return to setValue to preserve existing
behavior.
In `@frontend/packages/react-form-wizard/src/review/ReviewStep.tsx`:
- Around line 282-293: The catch block in writeReviewExpandableStorage silently
swallows errors from localStorage.setItem; update the function to catch the
error and, when in development (e.g., check process.env.NODE_ENV !==
'production' or a similar runtime flag), log a descriptive message including the
error and the reviewExpandableStorageKey(reviewStorageKey) value and the
sections/lastToolbar payload for debugging, while continuing to suppress/ignore
the error in production to avoid leaking exceptions to users.
- Around line 719-724: Replace the non-null assertion on props.isExpanded with a
clean narrowing expression: compute isControlled as before, then set isExpanded
using either the controlled prop or local state without using "!" — e.g. const
isExpanded = isControlled ? props.isExpanded : localExpanded or const isExpanded
= props.isExpanded ?? localExpanded; likewise remove any "!" usage for
props.onExpandedChange in onToggle and rely on the isControlled check to call
props.onExpandedChange(expanded).
In `@frontend/packages/react-form-wizard/src/review/ReviewStepContexts.tsx`:
- Line 108: The get function currently returns the ref object (inputMapRef)
instead of the Map value; change the useCallback returned value in get to return
inputMapRef.current so consumers receive the Map directly, and update the get
function's type signature accordingly (or, if you need to preserve returning a
ref for stability, rename the function to reflect that behavior). Ensure
references to get elsewhere expect a Map (not a MutableRefObject) and adjust
imports/usages if necessary.
- Line 177: getSteps currently builds and sorts a new array on every call
(defined as const getSteps = useCallback(...)), causing unnecessary allocations;
replace this with a memoized sorted array and return a stable accessor.
Specifically, compute the sorted steps once using useMemo keyed on the
appropriate change signal (e.g., version or an explicit stepsVersion) and mapRef
(or a ref-backed mutation counter), then have getSteps return that memoized
array (or remove getSteps and expose the memoized steps directly). Update
references to getSteps accordingly and keep the sort logic
(Array.from(mapRef.current.values()).sort((a,b)=>a.id.localeCompare(b.id))) when
building the memo so the result only recomputes when version/stepsVersion
changes.
In `@frontend/packages/react-form-wizard/src/review/utils.ts`:
- Around line 45-94: buildReviewSubtree currently recurses the DOM with no depth
limit which risks deep-stack or performance issues; modify buildReviewSubtree to
accept a maxDepth (e.g., add a parameter maxDepth: number = 100) and
short-circuit recursion when depth <= 0, passing depth-1 to recursive calls (or
alternatively implement an iterative stack-based traversal inside
buildReviewSubtree that tracks current depth and skips pushing children once
maxDepth is reached); update all internal calls that call buildReviewSubtree
(including the initial entry point) to provide the chosen maxDepth and ensure
behavior for nodes skipped by depth limiting is defined (e.g., treat them as
having no children).
In `@frontend/packages/react-form-wizard/src/Wizard.tsx`:
- Around line 173-181: The function defaultReviewStorageKeyFromId currently
falls back to "wizard-default" when id is empty causing unrelated wizards to
share storage; change it so when the sanitized id is empty it returns an empty
string (or null/undefined depending on the surrounding API) to disable
persistence instead of returning a shared "wizard-default". Update
defaultReviewStorageKeyFromId (and the other resolver used around the
reviewStorageKey resolution at the other site) to check if suffix === '' and
return a non-persisting sentinel (empty string/null/undefined) rather than
`wizard-default`, keeping MAX_REVIEW_STORAGE_KEY_LEN behavior for non-empty
keys.
In `@frontend/src/components/SyncEditor/SyncEditor.css`:
- Around line 75-78: The .syncEditorYamlHighlight CSS class uses a hardcoded
rgba highlight; replace that with a PatternFly color token (e.g., use a CSS
variable like var(--pf-global--palette--gold-200) or an appropriate
--pf-global--info/warning token) while keeping the !important override for
Monaco decorations, and then verify the resulting highlight over your syntax
colors meets WCAG contrast (adjust token choice or opacity as needed to reach
accessible contrast ratios for text).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2904fef2-b472-4ec7-9843-efd6df82aec4
⛔ Files ignored due to path filters (1)
frontend/public/locales/en/translation.jsonis excluded by!frontend/public/locales/**
📒 Files selected for processing (65)
frontend/packages/react-form-wizard/cypress/e2e/ansible.cy.tsfrontend/packages/react-form-wizard/cypress/e2e/input.cy.tsfrontend/packages/react-form-wizard/src/Step.tsxfrontend/packages/react-form-wizard/src/Wizard.tsxfrontend/packages/react-form-wizard/src/contexts/HighlightEditorPathContext.tsxfrontend/packages/react-form-wizard/src/contexts/ReviewDomTreeSyncContext.tsxfrontend/packages/react-form-wizard/src/contexts/StringContext.tsxfrontend/packages/react-form-wizard/src/index.tsfrontend/packages/react-form-wizard/src/inputs/Input.tsfrontend/packages/react-form-wizard/src/inputs/WizArrayInput.tsxfrontend/packages/react-form-wizard/src/inputs/WizAsyncSelect.tsxfrontend/packages/react-form-wizard/src/inputs/WizCheckbox.tsxfrontend/packages/react-form-wizard/src/inputs/WizFormGroup.tsxfrontend/packages/react-form-wizard/src/inputs/WizHidden.tsxfrontend/packages/react-form-wizard/src/inputs/WizKeyValue.tsxfrontend/packages/react-form-wizard/src/inputs/WizMultiSelect.tsxfrontend/packages/react-form-wizard/src/inputs/WizNumberInput.tsxfrontend/packages/react-form-wizard/src/inputs/WizRadio.tsxfrontend/packages/react-form-wizard/src/inputs/WizSelect.tsxfrontend/packages/react-form-wizard/src/inputs/WizSingleSelect.tsxfrontend/packages/react-form-wizard/src/inputs/WizStringsInput.tsxfrontend/packages/react-form-wizard/src/inputs/WizSwitch.tsxfrontend/packages/react-form-wizard/src/inputs/WizTableSelect.tsxfrontend/packages/react-form-wizard/src/inputs/WizTextArea.tsxfrontend/packages/react-form-wizard/src/inputs/WizTextInput.tsxfrontend/packages/react-form-wizard/src/inputs/WizTiles.tsxfrontend/packages/react-form-wizard/src/inputs/WizTimeRange.tsxfrontend/packages/react-form-wizard/src/review/ReviewStep.cssfrontend/packages/react-form-wizard/src/review/ReviewStep.tsxfrontend/packages/react-form-wizard/src/review/ReviewStepContexts.tsxfrontend/packages/react-form-wizard/src/review/ReviewStepNavigation.tsxfrontend/packages/react-form-wizard/src/review/ReviewStepToolbar.tsxfrontend/packages/react-form-wizard/src/review/utils.tsfrontend/packages/react-form-wizard/wizards/Ansible/AnsibleWizard.tsxfrontend/packages/react-form-wizard/wizards/AppWizard/AppWizard.tsxfrontend/packages/react-form-wizard/wizards/Application/ApplicationWizard.tsxfrontend/packages/react-form-wizard/wizards/Argo/ArgoWizard.tsxfrontend/packages/react-form-wizard/wizards/Cluster/ClusterForm.tsxfrontend/packages/react-form-wizard/wizards/Cluster/Provider.tsxfrontend/packages/react-form-wizard/wizards/Credentials/CredentialsWizard.tsxfrontend/packages/react-form-wizard/wizards/Home/HomeWizard.tsxfrontend/packages/react-form-wizard/wizards/Hypershift/AmazonHypershiftWizard.tsxfrontend/packages/react-form-wizard/wizards/Inputs/InputsWizard.tsxfrontend/packages/react-form-wizard/wizards/Placement/PlacementWizard.tsxfrontend/packages/react-form-wizard/wizards/Policy/PolicyWizard.tsxfrontend/packages/react-form-wizard/wizards/PolicyAutomation/PolicyAutomationWizard.tsxfrontend/packages/react-form-wizard/wizards/PolicySet/PolicySetWizard.tsxfrontend/packages/react-form-wizard/wizards/ROSA/RosaWizard.tsxfrontend/src/components/SyncEditor/SyncEditor.cssfrontend/src/components/SyncEditor/SyncEditor.tsxfrontend/src/components/SyncEditor/decorate.tsfrontend/src/lib/wizardStrings.tsfrontend/src/routes/Applications/CreateArgoApplication/CreatePullApplicationSet.tsxfrontend/src/routes/Applications/CreateArgoApplication/CreatePushApplicationSet.tsxfrontend/src/routes/Applications/CreateArgoApplication/EditArgoApplicationSet.tsxfrontend/src/routes/Governance/policies/CreatePolicy.tsxfrontend/src/routes/Governance/policies/CreatePolicyAutomation.tsxfrontend/src/routes/Governance/policies/EditPolicy.tsxfrontend/src/routes/Governance/policies/EditPolicyAutomation.tsxfrontend/src/routes/Governance/policy-sets/CreatePolicySet.tsxfrontend/src/routes/Governance/policy-sets/EditPolicySet.tsxfrontend/src/routes/Infrastructure/Clusters/ManagedClusters/ImportCluster/ImportCluster.tsxfrontend/src/wizards/Argo/ArgoWizard.test.tsxfrontend/src/wizards/Argo/CreateArgoResources.tsxfrontend/src/wizards/Argo/MultipleSourcesSelector.tsx
💤 Files with no reviewable changes (1)
- frontend/packages/react-form-wizard/src/inputs/WizHidden.tsx
There was a problem hiding this comment.
♻️ Duplicate comments (2)
frontend/packages/react-form-wizard/src/review/ReviewStepNavigation.tsx (2)
335-345:⚠️ Potential issue | 🟡 MinorUnsafe type cast from KeyboardEvent to MouseEvent.
The cast
e as unknown as ReactMouseEvent<HTMLElement>forces a keyboard event into a mouse event type. IfonPenClickuses mouse-specific properties, this will fail at runtime.Use union type for the handler
- onPenClick: (e: ReactMouseEvent<HTMLElement>) => void + onPenClick: (e: ReactMouseEvent<HTMLElement> | ReactKeyboardEvent<HTMLElement>) => voidThen update the call site:
- onPenClick(e as unknown as ReactMouseEvent<HTMLElement>) + onPenClick(e)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/react-form-wizard/src/review/ReviewStepNavigation.tsx` around lines 335 - 345, The current onZoneKeyDown in ReviewStepNavigation.tsx unsafely casts a KeyboardEvent to a MouseEvent when calling onPenClick; instead change the onPenClick handler type (prop and any usages) to accept a union event type React.MouseEvent<HTMLElement> | React.KeyboardEvent<HTMLElement> (or add a new onPenActivate union-typed wrapper) and remove the cast in onZoneKeyDown so you call onPenClick(e) directly; update the onPenClick signature and any implementors to handle both event kinds (or create a small adapter function that extracts shared data and delegates) and keep the function name references onZoneKeyDown and onPenClick consistent.
400-411:⚠️ Potential issue | 🟡 MinorMissing keyboard handler and accessibility attributes on non-dl-term variant.
The
descriptionListTerm == nullbranch renders<Comp>withonClickbut noonKeyDown,role, ortabIndex. This creates keyboard accessibility inconsistency with the dl-term variant.Add keyboard handler and accessibility attributes
<Comp className={zoneClassName} style={style} onMouseEnter={onEnter} onMouseLeave={onLeave} onClick={onZoneClick} + onKeyDown={onZoneKeyDown} + role="button" + tabIndex={0} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/react-form-wizard/src/review/ReviewStepNavigation.tsx` around lines 400 - 411, The non-dl-term branch rendering <Comp> (where descriptionListTerm == null) currently only has onClick, causing keyboard accessibility gaps; update that branch to mirror the dl-term variant by adding an onKeyDown handler that invokes onZoneClick when Enter or Space is pressed, and add role="button" and tabIndex={0} to <Comp> so it is focusable and announced as interactive; keep existing props (className={zoneClassName}, style, onMouseEnter={onEnter}, onMouseLeave={onLeave}) and ensure the onKeyDown references the same onZoneClick logic used by the click handler.
🧹 Nitpick comments (2)
frontend/packages/react-form-wizard/src/inputs/Input.ts (1)
161-164: Transform detection assumes specific signature.The
inputValueToPathValue(true, false)call assumes all transform functions accept these arguments meaningfully. If a transform expects different types or has side effects, this probing call could behave unexpectedly.Consider documenting this contract or adding a separate metadata flag for transformed inputs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/react-form-wizard/src/inputs/Input.ts` around lines 161 - 164, The code probes props.inputValueToPathValue by calling inputValueToPathValue(true, false), which assumes a specific signature and can cause unexpected behavior; instead add and check an explicit metadata flag (e.g., props.isTransformed or props.hasInputValueTransform) and only append the transformed JSON to registrationPath when that flag is true, or if you must probe, first verify the function's arity via props.inputValueToPathValue.length >= 2 before calling it; update the logic around registrationPath and the use of props.inputValueToPathValue accordingly (referencing props.inputValueToPathValue, registrationPath, transformed).frontend/packages/react-form-wizard/src/review/ReviewStepNavigation.tsx (1)
121-132: Variable declarations should precede usage.
autoDismissIdis assigned on line 124 but declared on line 132. Same foreaseOutFallbackIdused indismiss(). While JS hoisting makes this work, it harms readability.Move declarations before first use
+ let easeOutFallbackId: ReturnType<typeof setTimeout> | undefined + let autoDismissId: ReturnType<typeof setTimeout> | undefined + requestAnimationFrame(() => { requestAnimationFrame(() => { highlightEl.classList.add(REVIEW_EDIT_TARGET_HIGHLIGHT_VISIBLE_CLASS) autoDismissId = setTimeout(() => { autoDismissId = undefined dismiss() }, REVIEW_EDIT_TARGET_HIGHLIGHT_AUTO_DISMISS_MS) }) }) - - let easeOutFallbackId: ReturnType<typeof setTimeout> | undefined - let autoDismissId: ReturnType<typeof setTimeout> | undefined🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/react-form-wizard/src/review/ReviewStepNavigation.tsx` around lines 121 - 132, Move the declarations for autoDismissId and easeOutFallbackId so they appear before their first use: declare "let autoDismissId: ReturnType<typeof setTimeout> | undefined" and "let easeOutFallbackId: ReturnType<typeof setTimeout> | undefined" above the requestAnimationFrame block (before you assign autoDismissId inside that callback and before dismiss() may reference easeOutFallbackId); this preserves the existing types and behavior but improves readability and avoids relying on hoisting in ReviewStepNavigation (references: autoDismissId, easeOutFallbackId, dismiss(), highlightEl).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@frontend/packages/react-form-wizard/src/review/ReviewStepNavigation.tsx`:
- Around line 335-345: The current onZoneKeyDown in ReviewStepNavigation.tsx
unsafely casts a KeyboardEvent to a MouseEvent when calling onPenClick; instead
change the onPenClick handler type (prop and any usages) to accept a union event
type React.MouseEvent<HTMLElement> | React.KeyboardEvent<HTMLElement> (or add a
new onPenActivate union-typed wrapper) and remove the cast in onZoneKeyDown so
you call onPenClick(e) directly; update the onPenClick signature and any
implementors to handle both event kinds (or create a small adapter function that
extracts shared data and delegates) and keep the function name references
onZoneKeyDown and onPenClick consistent.
- Around line 400-411: The non-dl-term branch rendering <Comp> (where
descriptionListTerm == null) currently only has onClick, causing keyboard
accessibility gaps; update that branch to mirror the dl-term variant by adding
an onKeyDown handler that invokes onZoneClick when Enter or Space is pressed,
and add role="button" and tabIndex={0} to <Comp> so it is focusable and
announced as interactive; keep existing props (className={zoneClassName}, style,
onMouseEnter={onEnter}, onMouseLeave={onLeave}) and ensure the onKeyDown
references the same onZoneClick logic used by the click handler.
---
Nitpick comments:
In `@frontend/packages/react-form-wizard/src/inputs/Input.ts`:
- Around line 161-164: The code probes props.inputValueToPathValue by calling
inputValueToPathValue(true, false), which assumes a specific signature and can
cause unexpected behavior; instead add and check an explicit metadata flag
(e.g., props.isTransformed or props.hasInputValueTransform) and only append the
transformed JSON to registrationPath when that flag is true, or if you must
probe, first verify the function's arity via props.inputValueToPathValue.length
>= 2 before calling it; update the logic around registrationPath and the use of
props.inputValueToPathValue accordingly (referencing
props.inputValueToPathValue, registrationPath, transformed).
In `@frontend/packages/react-form-wizard/src/review/ReviewStepNavigation.tsx`:
- Around line 121-132: Move the declarations for autoDismissId and
easeOutFallbackId so they appear before their first use: declare "let
autoDismissId: ReturnType<typeof setTimeout> | undefined" and "let
easeOutFallbackId: ReturnType<typeof setTimeout> | undefined" above the
requestAnimationFrame block (before you assign autoDismissId inside that
callback and before dismiss() may reference easeOutFallbackId); this preserves
the existing types and behavior but improves readability and avoids relying on
hoisting in ReviewStepNavigation (references: autoDismissId, easeOutFallbackId,
dismiss(), highlightEl).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a9e64f97-291a-47e7-bac6-7aacf321b452
📒 Files selected for processing (7)
frontend/packages/react-form-wizard/src/Step.tsxfrontend/packages/react-form-wizard/src/Wizard.tsxfrontend/packages/react-form-wizard/src/index.tsfrontend/packages/react-form-wizard/src/inputs/Input.tsfrontend/packages/react-form-wizard/src/inputs/WizArrayInput.tsxfrontend/packages/react-form-wizard/src/review/ReviewStepContexts.tsxfrontend/packages/react-form-wizard/src/review/ReviewStepNavigation.tsx
✅ Files skipped from review due to trivial changes (2)
- frontend/packages/react-form-wizard/src/index.ts
- frontend/packages/react-form-wizard/src/inputs/WizArrayInput.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/packages/react-form-wizard/src/Wizard.tsx
- frontend/packages/react-form-wizard/src/Step.tsx
Signed-off-by: John Swanke <jswanke@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
frontend/packages/react-form-wizard/src/inputs/WizArrayInput.tsx (1)
181-185:⚠️ Potential issue | 🟠 MajorUse a stable key for reorderable array items.
This list still uses
key={index}. After move/remove, React will preserveexpandedstate and the review-registration subtree for the slot, not the actual item.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/react-form-wizard/src/inputs/WizArrayInput.tsx` around lines 181 - 185, The array mapping in WizArrayInput uses key={index}, which breaks stable identity on reorder/remove; change it to use a stable unique key per item (e.g., key={item.id} or key={`${id}-${item._uid}`}) when rendering ArrayInputItem. Ensure the array items in values have a persistent unique identifier (add an _uid or id when initializing/adding items inside WizArrayInput or the addItem handler) and use that property instead of the index in the values.map that returns ArrayInputItem so React preserves the correct subtree for each logical item.
🧹 Nitpick comments (1)
frontend/packages/react-form-wizard/src/inputs/WizArrayInput.tsx (1)
207-208: Make these ids instance-specific.Line 208 hardcodes
add-button, and Line 353 only scopes the header id byindex. Rendering more than one array input on the same step will produce duplicate DOM ids.Possible fix
- <Button - id="add-button" + <Button + id={`${id}-add-button`} variant="link" size="sm" aria-label={actionAriaLabel} onClick={() => addItem(props.newValue ?? {})} icon={<PlusCircleIcon />} >- id: `nested-field-group1-titleText-id-${index}`, + id: `${id}-title`,Also applies to: 353-353
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/react-form-wizard/src/inputs/WizArrayInput.tsx` around lines 207 - 208, The hardcoded DOM ids in WizArrayInput (e.g., Button id "add-button" and the header id that is only scoped by index) can collide when multiple array inputs render; update WizArrayInput to generate or accept an instance-specific identifier (use React.useId(), a prop like instanceId/name, or a generated uid) and append it to the Button id and the header id (e.g., "add-button-{instanceId}" and "header-{instanceId}-{index}") so all ids are unique per component instance and per item index; update any references or aria attributes that point to those ids accordingly (Button id, header id, and any label/aria-labelledby usage).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/packages/react-form-wizard/src/inputs/WizArrayInput.tsx`:
- Around line 389-404: The hidden DOM node is mounting the same ReactNode twice
via collapsedContent and can duplicate stateful side effects; instead add a
dedicated measurement API (e.g., a collapsedLabelText string prop or a
getCollapsedLabel(): string callback) and use that for measuring with
collapsedContentMeasureRef rather than rendering collapsedContent again. Update
the WizArrayInput props to accept collapsedLabelText or getCollapsedLabel,
change the measurement block to render only the plain string returned by that
prop (or skip rendering if absent), and remove the hidden duplicate render of
collapsedContent so stateful children are mounted only once.
- Around line 272-276: Array item review registrations are storing the DOM id
(variable id) instead of the logical review registration path, which breaks path
matching in test/Cypress where id is parentId-N; update
ArrayInputItemReviewRegistration to use registrationPath (computed by
buildReviewInputRegistrationPath) for its path field so it consistently matches
useInput's registration logic (see registrationPath, id,
ArrayInputItemReviewRegistration, buildReviewInputRegistrationPath, and
useInput), and apply the same change to the other occurrences mentioned (the
blocks around the other two spots).
---
Duplicate comments:
In `@frontend/packages/react-form-wizard/src/inputs/WizArrayInput.tsx`:
- Around line 181-185: The array mapping in WizArrayInput uses key={index},
which breaks stable identity on reorder/remove; change it to use a stable unique
key per item (e.g., key={item.id} or key={`${id}-${item._uid}`}) when rendering
ArrayInputItem. Ensure the array items in values have a persistent unique
identifier (add an _uid or id when initializing/adding items inside
WizArrayInput or the addItem handler) and use that property instead of the index
in the values.map that returns ArrayInputItem so React preserves the correct
subtree for each logical item.
---
Nitpick comments:
In `@frontend/packages/react-form-wizard/src/inputs/WizArrayInput.tsx`:
- Around line 207-208: The hardcoded DOM ids in WizArrayInput (e.g., Button id
"add-button" and the header id that is only scoped by index) can collide when
multiple array inputs render; update WizArrayInput to generate or accept an
instance-specific identifier (use React.useId(), a prop like instanceId/name, or
a generated uid) and append it to the Button id and the header id (e.g.,
"add-button-{instanceId}" and "header-{instanceId}-{index}") so all ids are
unique per component instance and per item index; update any references or aria
attributes that point to those ids accordingly (Button id, header id, and any
label/aria-labelledby usage).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 43fb266f-ea01-4190-9f31-1fa41bba64e3
📒 Files selected for processing (4)
frontend/packages/react-form-wizard/src/inputs/WizArrayInput.tsxfrontend/packages/react-form-wizard/src/inputs/WizFormGroup.tsxfrontend/packages/react-form-wizard/src/review/ReviewStep.tsxfrontend/src/components/SyncEditor/decorate.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/packages/react-form-wizard/src/inputs/WizFormGroup.tsx
- frontend/src/components/SyncEditor/decorate.ts
- frontend/packages/react-form-wizard/src/review/ReviewStep.tsx
Comment on lines
+389
to
+404
| {typeof props.collapsedContent !== 'string' && ( | ||
| <div | ||
| ref={collapsedContentMeasureRef} | ||
| aria-hidden | ||
| style={{ | ||
| position: 'absolute', | ||
| width: 0, | ||
| height: 0, | ||
| overflow: 'hidden', | ||
| clip: 'rect(0,0,0,0)', | ||
| whiteSpace: 'nowrap', | ||
| }} | ||
| > | ||
| {collapsedContent} | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
Avoid mounting collapsedContent twice for label measurement.
collapsedContent is typed as arbitrary ReactNode, so this hidden copy mounts the same subtree a second time. If callers pass anything stateful here, its effects, ids, and any review-related side effects will run twice. A dedicated label prop/callback would be safer than measuring a duplicate render.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/packages/react-form-wizard/src/inputs/WizArrayInput.tsx` around
lines 389 - 404, The hidden DOM node is mounting the same ReactNode twice via
collapsedContent and can duplicate stateful side effects; instead add a
dedicated measurement API (e.g., a collapsedLabelText string prop or a
getCollapsedLabel(): string callback) and use that for measuring with
collapsedContentMeasureRef rather than rendering collapsedContent again. Update
the WizArrayInput props to accept collapsedLabelText or getCollapsedLabel,
change the measurement block to render only the plain string returned by that
prop (or skip rendering if absent), and remove the hidden duplicate render of
collapsedContent so stateful children are mounted only once.
Contributor
Author
|
/retest images |
Contributor
Author
|
/test images |
Signed-off-by: John Swanke <jswanke@redhat.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
frontend/packages/react-form-wizard/src/inputs/WizArrayInput.tsx (2)
500-514:getArrayInstanceLabelsilently capitalizes first character.Line 511 uppercases the first char of measured text. This may be intentional for display consistency, but could surprise callers expecting verbatim text. Consider documenting this behavior or making it opt-in.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/react-form-wizard/src/inputs/WizArrayInput.tsx` around lines 500 - 514, The function getArrayInstanceLabel silently mutates measured text by uppercasing the first character; change it to make capitalization explicit by adding an optional parameter (e.g., capitalize: boolean = false) to getArrayInstanceLabel and only perform text.charAt(0).toUpperCase() + text.slice(1) when that flag is true (leave behavior for string-collapsedContent via get(item, collapsedContent) unchanged), and update all call sites to pass true where capitalization is desired (or document the new parameter in the function JSDoc if you prefer opting-in via documentation instead).
143-148: Consider reusingprependItemKindToRegistrationPathfromInput.ts.This kind-prefix logic duplicates the utility in
Input.ts:259-266. Extracting a shared helper would consolidate the pattern.Example consolidation
- if (item && typeof item === 'object' && 'kind' in item) { - const kind = (item as { kind: unknown }).kind - if (kind != null && String(kind) !== '') { - next.push(String(kind)) - } - } + // reuse prependItemKindToRegistrationPath or extract kind extraction helper + const kindPrefix = getItemKind(item) + if (kindPrefix) next.push(kindPrefix)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/react-form-wizard/src/inputs/WizArrayInput.tsx` around lines 143 - 148, The kind-prefix logic in WizArrayInput.tsx duplicates the helper in Input.ts; replace the inline block that checks item.kind (the code around the if (item && typeof item === 'object' && 'kind' in item) branch) by calling the shared helper prependItemKindToRegistrationPath from Input.ts (import it if necessary) so that kind extraction and String(kind) handling is centralized; update imports and remove the duplicated logic to reuse prependItemKindToRegistrationPath for building the next registration path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/packages/react-form-wizard/src/inputs/WizArrayInput.tsx`:
- Around line 500-514: The function getArrayInstanceLabel silently mutates
measured text by uppercasing the first character; change it to make
capitalization explicit by adding an optional parameter (e.g., capitalize:
boolean = false) to getArrayInstanceLabel and only perform
text.charAt(0).toUpperCase() + text.slice(1) when that flag is true (leave
behavior for string-collapsedContent via get(item, collapsedContent) unchanged),
and update all call sites to pass true where capitalization is desired (or
document the new parameter in the function JSDoc if you prefer opting-in via
documentation instead).
- Around line 143-148: The kind-prefix logic in WizArrayInput.tsx duplicates the
helper in Input.ts; replace the inline block that checks item.kind (the code
around the if (item && typeof item === 'object' && 'kind' in item) branch) by
calling the shared helper prependItemKindToRegistrationPath from Input.ts
(import it if necessary) so that kind extraction and String(kind) handling is
centralized; update imports and remove the duplicated logic to reuse
prependItemKindToRegistrationPath for building the next registration path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a5c1f5ae-ffff-4106-a8f3-eb69be31a3cb
📒 Files selected for processing (1)
frontend/packages/react-form-wizard/src/inputs/WizArrayInput.tsx
|
@jeswanke: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
fxiang1
requested changes
Apr 10, 2026
| stepInputsRegistry.unregister(id) | ||
| bumpReviewDomTree?.() | ||
| } | ||
| }, [stepInputsRegistry, index, collapsedContent, measureRef, value, id, item, bumpReviewDomTree]) |
Contributor
There was a problem hiding this comment.
I think index can be removed from here since it's not used in the useLayoutEffect.
Contributor
|
@jeswanke Overall I think this PR looks great! I just commented on a minor issue. One question, I noticed you added this feature to the cluster import wizard but not the cluster create wizard, will you create another work item to add this to other wizards? |
Contributor
|
@jeswanke I just realized on the Credential wizard we already have a design to not hide the action button on the review step. Maybe we can change the back link and highlight button to display like this? ie. not require the user to hover over to show the buttons and align to the right. 
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📝 Summary
Here is a description of the changes:
https://docs.google.com/document/d/1U2a_kY7IfbKwtBkybbJOp4qIO6bzzM0VZTOJzQY3dB8/edit?tab=t.0
Here is a video of the changes
https://redhat-internal.slack.com/archives/C02684L8JMV/p1775683819289319
Ticket Summary (Title):
Make wizard review step expandable/support back link/support yaml highlight
Ticket Link:
https://redhat.atlassian.net/browse/ACM-30806
Type of Change:
✅ Checklist
General
ACM-12340 Fix bug with...)If Feature
If Bugfix
🗒️ Notes for Reviewers
Summary by CodeRabbit
New Features
UI/UX Improvements
Tests