ACM-33628-UI-text-overflow-in-AppSet-Review-step-hides-action-buttons-behind-YAML-pane#6112
Conversation
📝 WalkthroughWalkthroughThis PR refactors the review section's description-list layout to support dual inline and stacked rendering modes for pen controls, with corresponding CSS grid structure updates and responsive width adjustments across three files. ChangesDescription-List Layout Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/packages/react-form-wizard/src/review/ReviewStepNavigation.tsx (1)
471-494:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftBoth inline and stacked clusters render simultaneously; duplicate IDs and double-mounted stateful components.
Lines 485–492 render
beforePenControlsandrenderEditButtons()twice—once in the inline cluster and once in the stacked cluster. Although CSS@containerqueries hide one viadisplay: none, both remain in the DOM:
- Duplicate DOM IDs & form associations: Any
id,for=,aria-describedby,aria-controls, or<label htmlFor>insidebeforePenControlsnow appears twice, breaking HTML validity and assistive tech associations.- Double-mounted components: When
beforePenControlsis a valid React element (line 455–458),cloneElementis called twice, creating two independent component instances. If the child is stateful, this violates the single-instance expectation.- Duplicate buttons:
renderEditButtons()generates two sets of buttons with identicalaria-labelattributes. Screen readers ignore the hidden set, but event listeners, selectors, and analytics still count both.Render a single cluster and toggle its visual position via CSS (e.g., grid row/column re-ordering within the same
@containerquery), or conditionally render based on container size to keep only one cluster in the DOM.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/packages/react-form-wizard/src/review/ReviewStepNavigation.tsx` around lines 471 - 494, The inline and stacked control clusters are both rendered (beforePenControlsInline / beforePenControlsStacked and renderEditButtons()) which duplicates IDs and mounts components twice; change ReviewStepNavigation to render a single control cluster (one span containing the cloned beforePenControls and the result of renderEditButtons()) and rely on CSS/@container queries to visually position it rather than outputting two DOM copies; update the JSX that currently emits both the inline and stacked spans to emit a single container (reuse DescriptionListDescription or the outer div) and remove the duplicate cloneElement/renderEditButtons calls so stateful children and IDs exist only once.
🧹 Nitpick comments (5)
frontend/packages/react-form-wizard/src/review/ReviewStep.css (2)
123-123: 💤 Low valueCoupling to a private PatternFly custom property.
--pf-v6-c-description-list__group--RowGapis an internal PatternFly token; private custom properties (double-underscore namespaced per BEM block) are not part of the documented theming surface and can disappear or be renamed across minor PatternFly releases. Consider a fallback (var(--pf-v6-c-description-list__group--RowGap, var(--pf-t--global--spacer--sm))) so the layout doesn't lose its row gap if the variable is dropped on upgrade.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/packages/react-form-wizard/src/review/ReviewStep.css` at line 123, The CSS currently uses a private PatternFly custom property for row-gap (row-gap: var(--pf-v6-c-description-list__group--RowGap)); update the declaration in ReviewStep.css to provide a safe fallback so layout won’t break if that internal token is removed—use the original token with a documented fallback such as var(--pf-v6-c-description-list__group--RowGap, var(--pf-t--global--spacer--sm)) so the row-gap falls back to a stable global spacer; locate the row-gap declaration in ReviewStep.css to make this change.
117-125: 💤 Low valueSubgrid relies on the PatternFly DL group being a grid container.
grid-template-columns: subgridrequires the parent (.pf-v6-c-description-list__group) to itself bedisplay: gridwith the term/description columns defined. PatternFly v6 horizontalDescriptionListdoes this today, but the selector silently degrades (subgrid → none) if PatternFly ever switches to flex/block, which would collapse the term and description into a single column.Worth confirming
pf-v6-c-description-list__groupis still a grid in@patternfly/react-core@6.4.3and that horizontalDescriptionListis what wraps these rows in the wizard.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/packages/react-form-wizard/src/review/ReviewStep.css` around lines 117 - 125, The CSS uses grid-template-columns: subgrid on .wizard-review-pen-hover-zone--dl-group-row which requires the parent .pf-v6-c-description-list__group to be display: grid; confirm PatternFly `@patternfly/react-core`@6.4.3 still makes .pf-v6-c-description-list__group a grid for horizontal DescriptionList, and add a defensive fallback: if the parent is not grid, force a compatible grid on the parent or replace subgrid with an explicit column template that matches the PatternFly term/description columns (reference .wizard-review-pen-hover-zone--dl-group-row and .pf-v6-c-description-list__group when making the change) so the term/description layout won’t collapse if PatternFly switches to flex/block.frontend/packages/react-form-wizard/src/review/ReviewStepNavigation.tsx (3)
418-451: 💤 Low value
renderEditButtonsis invoked up to twice per render in the DL path — confirm intent.The helper is called at line 467 (unused in the DL branch since
controlsis only rendered in theelseat line 498) and again at lines 486 and 492. In the DL path that's two invocations producing two pairs of buttons in the DOM, each carrying the sameariaLabel/arrowAriaLabel. This is consistent with the inline/stacked CSS strategy but compounds the duplication concern raised in the parent comment. If you switch to a single-cluster CSS-positioning approach, this helper only needs to be called once.Minor: since
renderEditButtonsis only consumed inside this component and not passed downward, a plain inline JSX block (oruseMemoreturning the element) would be more idiomatic thanuseCallbackreturning a fragment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/packages/react-form-wizard/src/review/ReviewStepNavigation.tsx` around lines 418 - 451, renderEditButtons is being invoked multiple times causing duplicate button pairs in the DL path; replace the useCallback that returns JSX with a useMemo (or an inline JSX constant) that returns the button fragment so the element is created once, then use that single memoized element wherever renderEditButtons was called; ensure the memo depends on ariaLabel, arrowAriaLabel, onArrowClick, onPenClick, and onPenIconClick and that the event handlers still call e.stopPropagation() and select the handler (onPenIconClick ?? onPenClick).
464-469: 💤 Low value
controlsis dead code in the DL branch.
controlsis built unconditionally but only consumed in the non-DLelsebranch at line 498. In the DL branch the inline/stacked clusters are constructed inline andcontrolsis discarded. Either inline this expression into theelsebranch, or move it inside auseMemo/conditional so it isn't allocated whendescriptionListTerm != null.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/packages/react-form-wizard/src/review/ReviewStepNavigation.tsx` around lines 464 - 469, The const controls (which builds <span className="wizard-review-pen-controls"> using beforePenControls and renderEditButtons()) is allocated unconditionally but only used when descriptionListTerm is null; move that expression into the non-DL branch where it's consumed (the else branch that renders the inline/stacked clusters) so it's only created when needed, or wrap it in a useMemo that returns null when descriptionListTerm != null and lists beforePenControls and renderEditButtons (and any other relevant props/state) as dependencies to avoid unnecessary allocation.
453-462: 💤 Low value
cloneElementrequiresbeforePenControlsto be a single React element.
isValidElementcorrectly gates the clone branch, but the fallback[beforePenControls, beforePenControls]returns the same node reference twice. If a caller ever passes an array or Fragment of elements, React will warn about missing/duplicate keys and the inline/stacked rows will collide. Consider wrapping each branch in a keyed Fragment, e.g.:- return [beforePenControls, beforePenControls] as const + return [ + <Fragment key="wizard-review-pen-bc-inline">{beforePenControls}</Fragment>, + <Fragment key="wizard-review-pen-bc-stacked">{beforePenControls}</Fragment>, + ] as constAlso worth wrapping this IIFE in
useMemokeyed onbeforePenControlsto avoid re-cloning on every render of the hover zone.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/packages/react-form-wizard/src/review/ReviewStepNavigation.tsx` around lines 453 - 462, The current IIFE that produces beforePenControlsInline and beforePenControlsStacked can clone a single element safely but returns the same node reference for non-element inputs, causing key collisions if callers pass arrays/Fragments; update the logic (affecting beforePenControlsInline, beforePenControlsStacked, beforePenControls, isValidElement, cloneElement) to: 1) wrap each branch result in a keyed Fragment when the input is not a single element (so inline and stacked each get a distinct keyed wrapper) and 2) memoize the whole computation with useMemo keyed on beforePenControls to avoid re-cloning on every render.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/packages/react-form-wizard/src/review/utils.ts`:
- Around line 127-134: The COMPACT vs WIDE breakpoint maps
(REVIEW_HORIZONTAL_TERM_WIDTH_COMPACT and REVIEW_HORIZONTAL_TERM_WIDTH_WIDE)
only differ at '2xl', so the selector horizontalTermWidthModifierForInputRun
(which switches by maxLen < 64) is ineffective below 2xl; either expand the WIDE
values for smaller breakpoints (default, sm, md, lg, xl) to the intended wider
ch sizes so short labels get more room at all sizes, or simplify the logic by
keeping both maps identical and only apply a '2xl-only' widening in
horizontalTermWidthModifierForInputRun (or remove the maxLen threshold). Update
the relevant constant(s) and/or the horizontalTermWidthModifierForInputRun
function accordingly to reflect the intended behavior.
---
Outside diff comments:
In `@frontend/packages/react-form-wizard/src/review/ReviewStepNavigation.tsx`:
- Around line 471-494: The inline and stacked control clusters are both rendered
(beforePenControlsInline / beforePenControlsStacked and renderEditButtons())
which duplicates IDs and mounts components twice; change ReviewStepNavigation to
render a single control cluster (one span containing the cloned
beforePenControls and the result of renderEditButtons()) and rely on
CSS/@container queries to visually position it rather than outputting two DOM
copies; update the JSX that currently emits both the inline and stacked spans to
emit a single container (reuse DescriptionListDescription or the outer div) and
remove the duplicate cloneElement/renderEditButtons calls so stateful children
and IDs exist only once.
---
Nitpick comments:
In `@frontend/packages/react-form-wizard/src/review/ReviewStep.css`:
- Line 123: The CSS currently uses a private PatternFly custom property for
row-gap (row-gap: var(--pf-v6-c-description-list__group--RowGap)); update the
declaration in ReviewStep.css to provide a safe fallback so layout won’t break
if that internal token is removed—use the original token with a documented
fallback such as var(--pf-v6-c-description-list__group--RowGap,
var(--pf-t--global--spacer--sm)) so the row-gap falls back to a stable global
spacer; locate the row-gap declaration in ReviewStep.css to make this change.
- Around line 117-125: The CSS uses grid-template-columns: subgrid on
.wizard-review-pen-hover-zone--dl-group-row which requires the parent
.pf-v6-c-description-list__group to be display: grid; confirm PatternFly
`@patternfly/react-core`@6.4.3 still makes .pf-v6-c-description-list__group a grid
for horizontal DescriptionList, and add a defensive fallback: if the parent is
not grid, force a compatible grid on the parent or replace subgrid with an
explicit column template that matches the PatternFly term/description columns
(reference .wizard-review-pen-hover-zone--dl-group-row and
.pf-v6-c-description-list__group when making the change) so the term/description
layout won’t collapse if PatternFly switches to flex/block.
In `@frontend/packages/react-form-wizard/src/review/ReviewStepNavigation.tsx`:
- Around line 418-451: renderEditButtons is being invoked multiple times causing
duplicate button pairs in the DL path; replace the useCallback that returns JSX
with a useMemo (or an inline JSX constant) that returns the button fragment so
the element is created once, then use that single memoized element wherever
renderEditButtons was called; ensure the memo depends on ariaLabel,
arrowAriaLabel, onArrowClick, onPenClick, and onPenIconClick and that the event
handlers still call e.stopPropagation() and select the handler (onPenIconClick
?? onPenClick).
- Around line 464-469: The const controls (which builds <span
className="wizard-review-pen-controls"> using beforePenControls and
renderEditButtons()) is allocated unconditionally but only used when
descriptionListTerm is null; move that expression into the non-DL branch where
it's consumed (the else branch that renders the inline/stacked clusters) so it's
only created when needed, or wrap it in a useMemo that returns null when
descriptionListTerm != null and lists beforePenControls and renderEditButtons
(and any other relevant props/state) as dependencies to avoid unnecessary
allocation.
- Around line 453-462: The current IIFE that produces beforePenControlsInline
and beforePenControlsStacked can clone a single element safely but returns the
same node reference for non-element inputs, causing key collisions if callers
pass arrays/Fragments; update the logic (affecting beforePenControlsInline,
beforePenControlsStacked, beforePenControls, isValidElement, cloneElement) to:
1) wrap each branch result in a keyed Fragment when the input is not a single
element (so inline and stacked each get a distinct keyed wrapper) and 2) memoize
the whole computation with useMemo keyed on beforePenControls to avoid
re-cloning on every render.
🪄 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: Enterprise
Run ID: 19d2bcda-088c-4a47-85ed-c4ffecd0fb7b
📒 Files selected for processing (3)
frontend/packages/react-form-wizard/src/review/ReviewStep.cssfrontend/packages/react-form-wizard/src/review/ReviewStepNavigation.tsxfrontend/packages/react-form-wizard/src/review/utils.ts
| const REVIEW_HORIZONTAL_TERM_WIDTH_WIDE: HorizontalTermWidthModifier = { | ||
| default: '24ch', | ||
| sm: '30ch', | ||
| md: '40ch', | ||
| lg: '56ch', | ||
| xl: '60ch', | ||
| default: '12ch', | ||
| sm: '15ch', | ||
| md: '20ch', | ||
| lg: '28ch', | ||
| xl: '30ch', | ||
| '2xl': '70ch', | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the file in question
find . -name "utils.ts" -path "*/react-form-wizard/src/review/*"Repository: stolostron/console
Length of output: 120
🏁 Script executed:
# Once found, let's read the relevant sections
cat -n frontend/packages/react-form-wizard/src/review/utils.ts | head -150Repository: stolostron/console
Length of output: 6880
🏁 Script executed:
# Search for both WIDE and COMPACT constants
rg "REVIEW_HORIZONTAL_TERM_WIDTH" frontend/packages/react-form-wizard/src/review/utils.ts -A 8Repository: stolostron/console
Length of output: 809
🏁 Script executed:
# Also search for the selector function
rg "horizontalTermWidthModifierForInputRun" frontend/packages/react-form-wizard/src/review/utils.ts -B 2 -A 10Repository: stolostron/console
Length of output: 523
COMPACT and WIDE constants are identical except at the 2xl breakpoint (35ch vs 70ch).
The selector horizontalTermWidthModifierForInputRun chooses between them based on maxLen < 64, but this distinction only affects term width at the 2xl breakpoint. For all smaller breakpoints (default through xl), both maps return identical values, making the label-length check ineffective below 2xl.
Confirm this matches the intended overflow fix; if the intent was to widen short labels at all sizes, the WIDE values at smaller breakpoints need adjustment. If the intent was a 2xl-only widening, consider whether the COMPACT/WIDE split and the 64-char threshold remain necessary.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/packages/react-form-wizard/src/review/utils.ts` around lines 127 -
134, The COMPACT vs WIDE breakpoint maps (REVIEW_HORIZONTAL_TERM_WIDTH_COMPACT
and REVIEW_HORIZONTAL_TERM_WIDTH_WIDE) only differ at '2xl', so the selector
horizontalTermWidthModifierForInputRun (which switches by maxLen < 64) is
ineffective below 2xl; either expand the WIDE values for smaller breakpoints
(default, sm, md, lg, xl) to the intended wider ch sizes so short labels get
more room at all sizes, or simplify the logic by keeping both maps identical and
only apply a '2xl-only' widening in horizontalTermWidthModifierForInputRun (or
remove the maxLen threshold). Update the relevant constant(s) and/or the
horizontalTermWidthModifierForInputRun function accordingly to reflect the
intended behavior.
|
/retest |
1 similar comment
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fxiang1, 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 |
2 participants
📝 Summary
Ticket Summary (Title):
UI text overflow in AppSet Review step hides action buttons behind YAML pane
Ticket Link:
https://redhat.atlassian.net/browse/ACM-33628
Type of Change:
✅ Checklist
General
ACM-12340 Fix bug with...)If Feature
If Bugfix
🗒️ Notes for Reviewers
Summary by CodeRabbit
Release Notes
Style
Refactor