Fix/nextjs hydration errors 12250#12251
Open
arpanroy41 wants to merge 12 commits intopatternfly:mainfrom
Open
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughRefactors ID and OUIA generation across the codebase to SSR-safe patterns by adding Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure the |
Collaborator
|
Preview: https://pf-react-pr-12251.surge.sh A11y report: https://pf-react-pr-12251-a11y.surge.sh |
…ultiple components - Updated AboutModal, CalendarMonth, CardHeader, Checkbox, DataListCheck, DrawerPanelContent, DualListSelector, DualListSelectorListItem, DualListSelectorListWrapper, DualListSelectorPane, ExpandableSection, FormGroup, InternalFormFieldGroup, JumpLinks, and MenuItem components to use useSSRSafeId for generating unique IDs instead of GenerateId. - This change improves consistency and simplifies ID generation across components.
…omponents - Updated Checkbox, ExpandableSection, FormSelect, Menu, MenuToggle, MenuToggleCheckbox, Modal, Nav, NavExpandable, and Progress components to utilize SSRSafeIds for generating unique IDs. - This change enhances ID management and ensures consistency across components.
The deprecated Table class component in react-table still requires getDefaultOUIAId which cannot be replaced with hooks. Re-added as a simplified legacy function marked @deprecated. Co-authored-by: Cursor <cursoragent@cursor.com>
arpanroy41
force-pushed
the
fix/nextjs-hydration-errors-12250
branch
from
b69f6fb to
b39dbde
Compare
- improve interface definitions by removing unnecessary line breaks and enhancing readability. - This change maintains consistency in code style across the codebase.
Reformatted with Prettier 3.8.1 to match CI and updated snapshot
- Updated snapshots for CodeEditor, AboutModalBoxCloseButton, AlertActionCloseButton, AlertIcon, ClipboardCopyButton, and others to reflect recent changes in SVG icons and structure. - Ensured consistency in icon rendering across components by replacing placeholder paths with actual SVG elements.
- Replaced the import of React's useId with a direct import from 'react' in the useSSRSafeId helper. - This change simplifies the code and ensures compatibility with the latest React features.
…sign - Added a resize event listener to update the viewport width in the ToolbarLabelGroup component. - Ensured proper cleanup of the event listener in componentWillUnmount. - Updated Cypress tests for the Switch component to reflect changes in element visibility and interaction.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
packages/react-core/src/components/Toolbar/ToolbarLabelGroupContent.tsx (1)
46-48: Avoid redundantsetStateduring resize storms.Line 47 sets state on every resize event, even when the value is unchanged. A quick equality guard reduces unnecessary rerenders.
♻️ Proposed refactor
handleResize = () => { - this.setState({ viewportWidth: window.innerWidth }); + const nextViewportWidth = window.innerWidth; + if (nextViewportWidth !== this.state.viewportWidth) { + this.setState({ viewportWidth: nextViewportWidth }); + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-core/src/components/Toolbar/ToolbarLabelGroupContent.tsx` around lines 46 - 48, The resize handler handleResize currently calls this.setState({ viewportWidth: window.innerWidth }) on every resize event causing redundant renders; update handleResize to first compare window.innerWidth to this.state.viewportWidth and only call this.setState when the value differs (i.e., guard against equality) to prevent unnecessary rerenders during resize storms.packages/react-core/src/components/DualListSelector/DualListSelectorListItem.tsx (1)
50-51: Missing trailing hyphen in prefix for consistency.The prefix
'dual-list-selector-list-item'is missing a trailing hyphen compared to similar components in this PR (e.g., the deprecated version uses'dual-list-selector-list-item-'). This results in generated IDs likedual-list-selector-list-item:r0:instead ofdual-list-selector-list-item-:r0:.💡 Suggested fix for consistency
- const generatedId = useSSRSafeId('dual-list-selector-list-item'); + const generatedId = useSSRSafeId('dual-list-selector-list-item-');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-core/src/components/DualListSelector/DualListSelectorListItem.tsx` around lines 50 - 51, The generated ID prefix used by useSSRSafeId in DualListSelectorListItem is missing a trailing hyphen; update the prefix string passed to useSSRSafeId (currently 'dual-list-selector-list-item') to include the trailing hyphen (e.g., 'dual-list-selector-list-item-') so that generatedId (and the resolved id variable which falls back to generatedId via idProp ?? generatedId) matches the other components' ID format.packages/react-core/src/components/Popover/Popover.tsx (1)
279-280: Consider using a descriptive prefix for easier debugging.The
useSSRSafeId()call uses the default'pf-'prefix. Other components in this PR use more descriptive prefixes (e.g.,'dual-list-selector-pane-','pf-progress-'). A prefix like'pf-popover-'would make generated IDs more identifiable during debugging.💡 Suggested change
- const generatedId = useSSRSafeId(); + const generatedId = useSSRSafeId('pf-popover-');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-core/src/components/Popover/Popover.tsx` around lines 279 - 280, The generated ID uses useSSRSafeId() with the default 'pf-' prefix; update the call in Popover (where generatedId and uniqueId are defined) to pass a descriptive prefix such as 'pf-popover-' so generatedId (and therefore uniqueId when id is not provided) is easily identifiable during debugging; adjust the useSSRSafeId invocation in the Popover component to include that prefix string.packages/react-core/src/components/JumpLinks/__tests__/JumpLinks.test.tsx (1)
140-142: Strengthen the new assertions to validate ID linkage, not just truthiness.A truthy
aria-labelledbycan still point to a missing element. Consider asserting the referenced node exists.💡 Suggested test hardening
const navigation = screen.getByRole('navigation', { name: /Toggle jump links/i }); expect(navigation).toHaveAttribute('aria-labelledby'); - expect(navigation.getAttribute('aria-labelledby')).toBeTruthy(); + const labelledById = navigation.getAttribute('aria-labelledby'); + expect(labelledById).toBeTruthy(); + expect(document.getElementById(labelledById!)).toBeInTheDocument();const navigation = screen.getByRole('navigation', { name: /Jump to section/i }); expect(navigation).toHaveAttribute('aria-labelledby'); - expect(navigation.getAttribute('aria-labelledby')).toBeTruthy(); + const labelledById = navigation.getAttribute('aria-labelledby'); + expect(labelledById).toBeTruthy(); + expect(document.getElementById(labelledById!)).toBeInTheDocument();Also applies to: 151-153
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-core/src/components/JumpLinks/__tests__/JumpLinks.test.tsx` around lines 140 - 142, Replace the loose truthiness checks for aria-labelledby with assertions that the ID actually resolves to an existing element: read the attribute from the navigation element (navigation.getAttribute('aria-labelledby') -> id), assert id is a non-empty string, then assert document.getElementById(id) is not null (and optionally that it equals the expected label node or has expected text). Apply the same stronger assertion pattern to the other occurrence in the same test (the block around the second expect at 151-153).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react-core/src/components/Card/CardHeader.tsx`:
- Around line 170-176: The clickable link branch in CardHeader.tsx (inside the
isClickableLinkCard handling that returns baseProps with selectableActions)
doesn't set rel for external links; update the object returned where
selectableActions.isExternalLink is used to also include rel: 'noopener
noreferrer' when target: '_blank' is set (i.e., when
selectableActions.isExternalLink is true) so external links are hardened against
opener access while keeping existing spreads like baseProps and aria/tabIndex
behavior.
In `@packages/react-core/src/components/Checkbox/Checkbox.tsx`:
- Around line 120-159: The input may receive checked={null} because
checkedProps.checked is normalized after inputRendered is created; move the
normalization so checkedProps.checked is set (e.g., checkedProps.checked =
checkedProps.checked === null ? false : checkedProps.checked) before building
inputRendered (the JSX that uses checkedProps), ensuring the ref indeterminate
logic that reads isChecked still works; update places referencing checkedProps,
inputRendered, isChecked, and WrapperComponent accordingly so the input never
receives null for the checked prop.
In
`@packages/react-core/src/components/DualListSelector/DualListSelectorPane.tsx`:
- Around line 68-72: The inner ternary is redundant: in DualListSelectorPane
remove the conditional expression "{searchInput ? searchInput : <SearchInput
isDisabled={isDisabled} />}" and simply render "searchInput" inside the div with
className={css(styles.dualListSelectorToolsFilter)} (or, if you intended to show
the default SearchInput when searchInput is falsy, change the outer condition to
always render the div and use the ternary there). Update references to
searchInput and SearchInput accordingly and keep isDisabled passed to the
default SearchInput if you choose the always-render approach.
In `@packages/react-core/src/components/FormSelect/FormSelect.tsx`:
- Around line 76-77: The generated OUIA id for FormSelect is changing because
ouiaComponentType passed into SSRSafeIds includes the mutable validated value;
update the usage in FormSelect so SSRSafeIds receives a stable ouiaComponentType
(e.g., "FormSelect" only) instead of `FormSelect${validated ? `-${validated}` :
''}` to preserve generatedOuiaId across validation changes, and if you need to
expose validation in OUIA, attach validated to a separate stable prop or data
attribute rather than altering ouiaComponentType.
In `@packages/react-core/src/components/Switch/Switch.tsx`:
- Around line 126-130: The Switch component currently renders <CheckIcon />
unconditionally in the unlabeled branch, ignoring the hasCheckIcon prop; update
the unlabeled rendering path inside the Switch component to only render the
CheckIcon when hasCheckIcon is true (same condition used in the labeled branch),
e.g. gate the element rendered inside the node with className
css(styles.switchToggleIcon) on props.hasCheckIcon so that switchToggle and
switchToggleIcon markup remains but the icon is omitted when hasCheckIcon is
false.
In `@packages/react-core/src/components/Tooltip/Tooltip.tsx`:
- Around line 166-167: The cleanup that removes the tooltip id from trigger
element aria attributes fails for useId-style ids (e.g., pf-tooltip-:r0:)
because it uses a \b-based regex; update the cleanup in the Tooltip component
(the effect that uses generatedId / id to set/remove
aria-describedby/aria-labelledby) to remove the id token safely by either
escaping the id for a RegExp (use an escape function for special chars) or,
better, by splitting the attribute value on whitespace and filtering out exact
matches of id (then rejoin and remove the attribute if empty); ensure this logic
runs both for aria-describedby and aria-labelledby and references the same id
variable (id or generatedId) used when adding the attribute.
In `@packages/react-core/src/helpers/useSSRSafeId.ts`:
- Around line 15-17: The counter-based fallback in useSSRSafeId can cause SSR
hydration mismatches because the module-level counter resets between server and
client; update useSSRSafeId to detect when the fallback path is taken (e.g.,
when useId is unavailable) and when running on the server (typeof window ===
'undefined') and emit a dev-only warning (process.env.NODE_ENV !== 'production')
that documents the limitation and advises using React 18+ or explicit IDs; also
add a short note to the component library docs/README referencing useSSRSafeId
and the SSR limitation so consumers are aware.
---
Nitpick comments:
In
`@packages/react-core/src/components/DualListSelector/DualListSelectorListItem.tsx`:
- Around line 50-51: The generated ID prefix used by useSSRSafeId in
DualListSelectorListItem is missing a trailing hyphen; update the prefix string
passed to useSSRSafeId (currently 'dual-list-selector-list-item') to include the
trailing hyphen (e.g., 'dual-list-selector-list-item-') so that generatedId (and
the resolved id variable which falls back to generatedId via idProp ??
generatedId) matches the other components' ID format.
In `@packages/react-core/src/components/JumpLinks/__tests__/JumpLinks.test.tsx`:
- Around line 140-142: Replace the loose truthiness checks for aria-labelledby
with assertions that the ID actually resolves to an existing element: read the
attribute from the navigation element
(navigation.getAttribute('aria-labelledby') -> id), assert id is a non-empty
string, then assert document.getElementById(id) is not null (and optionally that
it equals the expected label node or has expected text). Apply the same stronger
assertion pattern to the other occurrence in the same test (the block around the
second expect at 151-153).
In `@packages/react-core/src/components/Popover/Popover.tsx`:
- Around line 279-280: The generated ID uses useSSRSafeId() with the default
'pf-' prefix; update the call in Popover (where generatedId and uniqueId are
defined) to pass a descriptive prefix such as 'pf-popover-' so generatedId (and
therefore uniqueId when id is not provided) is easily identifiable during
debugging; adjust the useSSRSafeId invocation in the Popover component to
include that prefix string.
In `@packages/react-core/src/components/Toolbar/ToolbarLabelGroupContent.tsx`:
- Around line 46-48: The resize handler handleResize currently calls
this.setState({ viewportWidth: window.innerWidth }) on every resize event
causing redundant renders; update handleResize to first compare
window.innerWidth to this.state.viewportWidth and only call this.setState when
the value differs (i.e., guard against equality) to prevent unnecessary
rerenders during resize storms.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (78)
packages/react-code-editor/src/components/CodeEditor/__test__/__snapshots__/CodeEditor.test.tsx.snapis excluded by!**/*.snappackages/react-code-editor/src/components/CodeEditor/__test__/__snapshots__/CodeEditorControl.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/AboutModal/__tests__/__snapshots__/AboutModalBoxCloseButton.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/AboutModal/__tests__/__snapshots__/AboutModalBoxHeader.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/Alert/__tests__/Generated/__snapshots__/AlertActionCloseButton.test.tsx.snapis excluded by!**/*.snap,!**/generated/**packages/react-core/src/components/Alert/__tests__/Generated/__snapshots__/AlertActionLink.test.tsx.snapis excluded by!**/*.snap,!**/generated/**packages/react-core/src/components/Breadcrumb/__tests__/__snapshots__/Breadcrumb.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/Button/__tests__/__snapshots__/Button.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/Card/__tests__/__snapshots__/Card.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/Card/__tests__/__snapshots__/CardHeader.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/ClipboardCopy/__tests__/__snapshots__/ClipboardCopy.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/ClipboardCopy/__tests__/__snapshots__/ClipboardCopyButton.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/ClipboardCopy/__tests__/__snapshots__/ClipboardCopyToggle.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/Compass/__tests__/__snapshots__/CompassNavHome.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/Compass/__tests__/__snapshots__/CompassNavSearch.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/DataList/__tests__/Generated/__snapshots__/DataListCheck.test.tsx.snapis excluded by!**/*.snap,!**/generated/**packages/react-core/src/components/DataList/__tests__/Generated/__snapshots__/DataListToggle.test.tsx.snapis excluded by!**/*.snap,!**/generated/**packages/react-core/src/components/DataList/__tests__/__snapshots__/DataListToggle.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/DatePicker/__tests__/__snapshots__/DatePicker.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/Drawer/__tests__/Generated/__snapshots__/DrawerPanelContent.test.tsx.snapis excluded by!**/*.snap,!**/generated/**packages/react-core/src/components/Drawer/__tests__/__snapshots__/Drawer.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/EmptyState/__tests__/__snapshots__/EmptyState.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/ExpandableSection/__tests__/__snapshots__/ExpandableSection.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/FileUpload/__tests__/__snapshots__/FileUpload.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/FileUpload/__tests__/__snapshots__/FileUploadField.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/Form/__tests__/__snapshots__/FormFieldGroup.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/FormSelect/__tests__/__snapshots__/FormSelect.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/JumpLinks/__tests__/__snapshots__/JumpLinks.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/Label/__tests__/__snapshots__/Label.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/Label/__tests__/__snapshots__/LabelGroup.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/LoginPage/__tests__/__snapshots__/LoginForm.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/LoginPage/__tests__/__snapshots__/LoginMainHeader.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/LoginPage/__tests__/__snapshots__/LoginPage.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/Menu/__tests__/__snapshots__/Menu.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/MenuToggle/__tests__/__snapshots__/MenuToggle.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/Modal/__tests__/__snapshots__/ModalContent.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/MultipleFileUpload/__tests__/__snapshots__/MultipleFileUploadButton.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/MultipleFileUpload/__tests__/__snapshots__/MultipleFileUploadMain.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/MultipleFileUpload/__tests__/__snapshots__/MultipleFileUploadStatus.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/MultipleFileUpload/__tests__/__snapshots__/MultipleFileUploadStatusItem.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/Nav/__tests__/Generated/__snapshots__/NavExpandable.test.tsx.snapis excluded by!**/*.snap,!**/generated/**packages/react-core/src/components/Nav/__tests__/Generated/__snapshots__/NavItem.test.tsx.snapis excluded by!**/*.snap,!**/generated/**packages/react-core/src/components/Nav/__tests__/__snapshots__/Nav.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/NotificationBadge/__tests__/__snapshots__/NotificationBadge.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/NumberInput/__tests__/__snapshots__/NumberInput.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/OverflowMenu/__tests__/Generated/__snapshots__/OverflowMenuDropdownItem.test.tsx.snapis excluded by!**/*.snap,!**/generated/**packages/react-core/src/components/Page/__tests__/__snapshots__/Page.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/Pagination/__tests__/Generated/__snapshots__/PaginationNavigation.test.tsx.snapis excluded by!**/*.snap,!**/generated/**packages/react-core/src/components/Pagination/__tests__/Generated/__snapshots__/PaginationOptionsMenu.test.tsx.snapis excluded by!**/*.snap,!**/generated/**packages/react-core/src/components/Pagination/__tests__/__snapshots__/Pagination.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/Popover/__tests__/Generated/__snapshots__/PopoverCloseButton.test.tsx.snapis excluded by!**/*.snap,!**/generated/**packages/react-core/src/components/Radio/__tests__/__snapshots__/Radio.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/SearchInput/__tests__/__snapshots__/SearchInput.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/SkipToContent/__tests__/__snapshots__/SkipToContent.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/Slider/__tests__/__snapshots__/Slider.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/Switch/__tests__/__snapshots__/Switch.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/Tabs/__tests__/__snapshots__/Tab.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/Tabs/__tests__/__snapshots__/Tabs.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/TextInput/__tests__/__snapshots__/TextInput.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/Toolbar/__tests__/__snapshots__/Toolbar.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/deprecated/components/Chip/__tests__/__snapshots__/Chip.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/deprecated/components/DualListSelector/__tests__/__snapshots__/DualListSelector.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/deprecated/components/Modal/__tests__/__snapshots__/ModalContent.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/deprecated/components/Wizard/__tests__/Generated/__snapshots__/WizardFooterInternal.test.tsx.snapis excluded by!**/*.snap,!**/generated/**packages/react-core/src/deprecated/components/Wizard/__tests__/Generated/__snapshots__/WizardNav.test.tsx.snapis excluded by!**/*.snap,!**/generated/**packages/react-core/src/deprecated/components/Wizard/__tests__/Generated/__snapshots__/WizardNavItem.test.tsx.snapis excluded by!**/*.snap,!**/generated/**packages/react-core/src/deprecated/components/Wizard/__tests__/__snapshots__/Wizard.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/helpers/GenerateId/__tests__/__snapshots__/GenerateId.test.tsx.snapis excluded by!**/*.snappackages/react-drag-drop/src/components/DragDrop/__tests__/__snapshots__/DragDrop.test.tsx.snapis excluded by!**/*.snappackages/react-table/src/components/Table/__tests__/__snapshots__/RowWrapper.test.tsx.snapis excluded by!**/*.snappackages/react-table/src/components/Table/__tests__/__snapshots__/Table.test.tsx.snapis excluded by!**/*.snappackages/react-table/src/components/Table/__tests__/__snapshots__/Tr.test.tsx.snapis excluded by!**/*.snappackages/react-table/src/deprecated/components/Table/__tests__/__snapshots__/Table.test.tsx.snapis excluded by!**/*.snappackages/react-templates/src/components/Dropdown/__tests__/__snapshots__/SimpleDropdown.test.tsx.snapis excluded by!**/*.snappackages/react-templates/src/components/Select/__tests__/__snapshots__/CheckboxSelectSnapshots.test.tsx.snapis excluded by!**/*.snappackages/react-templates/src/components/Select/__tests__/__snapshots__/MultiTypeaheadSelect.test.tsx.snapis excluded by!**/*.snappackages/react-templates/src/components/Select/__tests__/__snapshots__/SimpleSelect.test.tsx.snapis excluded by!**/*.snappackages/react-templates/src/components/Select/__tests__/__snapshots__/TypeaheadSelect.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (51)
packages/react-core/src/components/AboutModal/AboutModal.tsxpackages/react-core/src/components/CalendarMonth/CalendarMonth.tsxpackages/react-core/src/components/Card/CardHeader.tsxpackages/react-core/src/components/Checkbox/Checkbox.tsxpackages/react-core/src/components/DataList/DataListCheck.tsxpackages/react-core/src/components/Drawer/DrawerPanelContent.tsxpackages/react-core/src/components/DualListSelector/DualListSelector.tsxpackages/react-core/src/components/DualListSelector/DualListSelectorListItem.tsxpackages/react-core/src/components/DualListSelector/DualListSelectorListWrapper.tsxpackages/react-core/src/components/DualListSelector/DualListSelectorPane.tsxpackages/react-core/src/components/ExpandableSection/ExpandableSection.tsxpackages/react-core/src/components/Form/FormGroup.tsxpackages/react-core/src/components/Form/InternalFormFieldGroup.tsxpackages/react-core/src/components/FormSelect/FormSelect.tsxpackages/react-core/src/components/JumpLinks/JumpLinks.tsxpackages/react-core/src/components/JumpLinks/__tests__/JumpLinks.test.tsxpackages/react-core/src/components/Menu/Menu.tsxpackages/react-core/src/components/Menu/MenuItem.tsxpackages/react-core/src/components/MenuToggle/MenuToggle.tsxpackages/react-core/src/components/MenuToggle/MenuToggleCheckbox.tsxpackages/react-core/src/components/Modal/Modal.tsxpackages/react-core/src/components/MultipleFileUpload/MultipleFileUploadStatus.tsxpackages/react-core/src/components/Nav/Nav.tsxpackages/react-core/src/components/Nav/NavExpandable.tsxpackages/react-core/src/components/Nav/NavGroup.tsxpackages/react-core/src/components/Popover/Popover.tsxpackages/react-core/src/components/Progress/Progress.tsxpackages/react-core/src/components/Radio/Radio.tsxpackages/react-core/src/components/SearchInput/AdvancedSearchMenu.tsxpackages/react-core/src/components/Select/Select.tsxpackages/react-core/src/components/Switch/Switch.tsxpackages/react-core/src/components/Tabs/Tabs.tsxpackages/react-core/src/components/TextInput/TextInput.tsxpackages/react-core/src/components/TimePicker/TimePicker.tsxpackages/react-core/src/components/Toolbar/Toolbar.tsxpackages/react-core/src/components/Toolbar/ToolbarLabelGroupContent.tsxpackages/react-core/src/components/Toolbar/ToolbarToggleGroup.tsxpackages/react-core/src/components/Tooltip/Tooltip.tsxpackages/react-core/src/components/TreeView/TreeViewListItem.tsxpackages/react-core/src/components/TreeView/__tests__/TreeViewListItem.test.tsxpackages/react-core/src/deprecated/components/Chip/Chip.tsxpackages/react-core/src/deprecated/components/DualListSelector/DualListSelectorListItem.tsxpackages/react-core/src/deprecated/components/DualListSelector/DualListSelectorListWrapper.tsxpackages/react-core/src/deprecated/components/DualListSelector/DualListSelectorPane.tsxpackages/react-core/src/deprecated/components/Modal/Modal.tsxpackages/react-core/src/helpers/GenerateId/GenerateId.tspackages/react-core/src/helpers/OUIA/ouia.tspackages/react-core/src/helpers/SSRSafeIds/SSRSafeIds.tsxpackages/react-core/src/helpers/index.tspackages/react-core/src/helpers/useSSRSafeId.tspackages/react-integration/cypress/integration/switch.spec.ts
Comment on lines
+170
to
176
| if (isClickableLinkCard) { | ||
| return { | ||
| ...baseProps, | ||
| href: selectableActions.to, | ||
| ...(isCardDisabled && { tabIndex: -1, 'aria-disabled': true }), | ||
| ...(selectableActions.isExternalLink && { target: '_blank' }) | ||
| }; |
There was a problem hiding this comment.
Harden external-link clickable actions with rel when using _blank.
Opening links in a new tab without rel="noopener noreferrer" leaves the opener vulnerable.
🔐 Proposed fix
if (isClickableLinkCard) {
return {
...baseProps,
href: selectableActions.to,
...(isCardDisabled && { tabIndex: -1, 'aria-disabled': true }),
- ...(selectableActions.isExternalLink && { target: '_blank' })
+ ...(selectableActions.isExternalLink && {
+ target: '_blank',
+ rel: selectableActions.selectableActionProps?.rel ?? 'noopener noreferrer'
+ })
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (isClickableLinkCard) { | |
| return { | |
| ...baseProps, | |
| href: selectableActions.to, | |
| ...(isCardDisabled && { tabIndex: -1, 'aria-disabled': true }), | |
| ...(selectableActions.isExternalLink && { target: '_blank' }) | |
| }; | |
| if (isClickableLinkCard) { | |
| return { | |
| ...baseProps, | |
| href: selectableActions.to, | |
| ...(isCardDisabled && { tabIndex: -1, 'aria-disabled': true }), | |
| ...(selectableActions.isExternalLink && { | |
| target: '_blank', | |
| rel: selectableActions.selectableActionProps?.rel ?? 'noopener noreferrer' | |
| }) | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-core/src/components/Card/CardHeader.tsx` around lines 170 -
176, The clickable link branch in CardHeader.tsx (inside the isClickableLinkCard
handling that returns baseProps with selectableActions) doesn't set rel for
external links; update the object returned where
selectableActions.isExternalLink is used to also include rel: 'noopener
noreferrer' when target: '_blank' is set (i.e., when
selectableActions.isExternalLink is true) so external links are hardened against
opener access while keeping existing spreads like baseProps and aria/tabIndex
behavior.
Comment on lines
+120
to
+159
| const inputRendered = ( | ||
| <input | ||
| {...props} | ||
| className={css(styles.checkInput, inputClassName)} | ||
| type="checkbox" | ||
| onChange={this.handleChange} | ||
| aria-invalid={!isValid} | ||
| aria-label={ariaLabel} | ||
| aria-describedby={ariaDescribedByValue} | ||
| disabled={isDisabled} | ||
| required={isRequired} | ||
| ref={(elem) => { | ||
| elem && (elem.indeterminate = isChecked === null); | ||
| }} | ||
| {...checkedProps} | ||
| {...getOUIAProps(Checkbox.displayName, ouiaId !== undefined ? ouiaId : generatedOuiaId, ouiaSafe)} | ||
| /> | ||
| ); | ||
|
|
||
| const wrapWithLabel = (isLabelWrapped && !component) || component === 'label'; | ||
| const wrapWithLabel = (isLabelWrapped && !component) || component === 'label'; | ||
|
|
||
| const Label = wrapWithLabel ? 'span' : 'label'; | ||
| const labelRendered = label ? ( | ||
| <Label | ||
| className={css(styles.checkLabel, isDisabled && styles.modifiers.disabled)} | ||
| htmlFor={!wrapWithLabel ? props.id : undefined} | ||
| > | ||
| {label} | ||
| {isRequired && ( | ||
| <span className={css(styles.checkLabelRequired)} aria-hidden="true"> | ||
| {ASTERISK} | ||
| </span> | ||
| )} | ||
| </Label> | ||
| ) : null; | ||
| const Label = wrapWithLabel ? 'span' : 'label'; | ||
| const labelRendered = label ? ( | ||
| <Label | ||
| className={css(styles.checkLabel, isDisabled && styles.modifiers.disabled)} | ||
| htmlFor={!wrapWithLabel ? props.id : undefined} | ||
| > | ||
| {label} | ||
| {isRequired && ( | ||
| <span className={css(styles.checkLabelRequired)} aria-hidden="true"> | ||
| {ASTERISK} | ||
| </span> | ||
| )} | ||
| </Label> | ||
| ) : null; | ||
|
|
||
| const Component = component ?? (wrapWithLabel ? 'label' : 'div'); | ||
| const WrapperComponent = component ?? (wrapWithLabel ? 'label' : 'div'); | ||
|
|
||
| checkedProps.checked = checkedProps.checked === null ? false : checkedProps.checked; | ||
| return ( | ||
| <Component | ||
| className={css(styles.check, !label && styles.modifiers.standalone, className)} | ||
| htmlFor={wrapWithLabel ? props.id : undefined} | ||
| > | ||
| {labelPosition === 'start' ? ( | ||
| <> | ||
| {labelRendered} | ||
| {inputRendered} | ||
| </> | ||
| ) : ( | ||
| <> | ||
| {inputRendered} | ||
| {labelRendered} | ||
| </> | ||
| )} | ||
| {description && ( | ||
| <span id={this.state.descriptionId} className={css(styles.checkDescription)}> | ||
| {description} | ||
| </span> | ||
| )} | ||
| {body && <span className={css(styles.checkBody)}>{body}</span>} | ||
| </Component> | ||
| checkedProps.checked = checkedProps.checked === null ? false : checkedProps.checked; | ||
| return ( |
There was a problem hiding this comment.
Normalize checked before creating the input element.
On Line 158, checkedProps.checked is coerced after inputRendered is already built on Line 120, so isChecked === null can still pass checked={null} into <input>.
Proposed fix
- const inputRendered = (
+ const normalizedCheckedProps = {
+ ...checkedProps,
+ ...(checkedProps.checked === null ? { checked: false } : {})
+ };
+
+ const inputRendered = (
<input
{...props}
className={css(styles.checkInput, inputClassName)}
type="checkbox"
onChange={this.handleChange}
aria-invalid={!isValid}
aria-label={ariaLabel}
aria-describedby={ariaDescribedByValue}
disabled={isDisabled}
required={isRequired}
ref={(elem) => {
elem && (elem.indeterminate = isChecked === null);
}}
- {...checkedProps}
+ {...normalizedCheckedProps}
{...getOUIAProps(Checkbox.displayName, ouiaId !== undefined ? ouiaId : generatedOuiaId, ouiaSafe)}
/>
);
@@
- checkedProps.checked = checkedProps.checked === null ? false : checkedProps.checked;
return (📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const inputRendered = ( | |
| <input | |
| {...props} | |
| className={css(styles.checkInput, inputClassName)} | |
| type="checkbox" | |
| onChange={this.handleChange} | |
| aria-invalid={!isValid} | |
| aria-label={ariaLabel} | |
| aria-describedby={ariaDescribedByValue} | |
| disabled={isDisabled} | |
| required={isRequired} | |
| ref={(elem) => { | |
| elem && (elem.indeterminate = isChecked === null); | |
| }} | |
| {...checkedProps} | |
| {...getOUIAProps(Checkbox.displayName, ouiaId !== undefined ? ouiaId : generatedOuiaId, ouiaSafe)} | |
| /> | |
| ); | |
| const wrapWithLabel = (isLabelWrapped && !component) || component === 'label'; | |
| const wrapWithLabel = (isLabelWrapped && !component) || component === 'label'; | |
| const Label = wrapWithLabel ? 'span' : 'label'; | |
| const labelRendered = label ? ( | |
| <Label | |
| className={css(styles.checkLabel, isDisabled && styles.modifiers.disabled)} | |
| htmlFor={!wrapWithLabel ? props.id : undefined} | |
| > | |
| {label} | |
| {isRequired && ( | |
| <span className={css(styles.checkLabelRequired)} aria-hidden="true"> | |
| {ASTERISK} | |
| </span> | |
| )} | |
| </Label> | |
| ) : null; | |
| const Label = wrapWithLabel ? 'span' : 'label'; | |
| const labelRendered = label ? ( | |
| <Label | |
| className={css(styles.checkLabel, isDisabled && styles.modifiers.disabled)} | |
| htmlFor={!wrapWithLabel ? props.id : undefined} | |
| > | |
| {label} | |
| {isRequired && ( | |
| <span className={css(styles.checkLabelRequired)} aria-hidden="true"> | |
| {ASTERISK} | |
| </span> | |
| )} | |
| </Label> | |
| ) : null; | |
| const Component = component ?? (wrapWithLabel ? 'label' : 'div'); | |
| const WrapperComponent = component ?? (wrapWithLabel ? 'label' : 'div'); | |
| checkedProps.checked = checkedProps.checked === null ? false : checkedProps.checked; | |
| return ( | |
| <Component | |
| className={css(styles.check, !label && styles.modifiers.standalone, className)} | |
| htmlFor={wrapWithLabel ? props.id : undefined} | |
| > | |
| {labelPosition === 'start' ? ( | |
| <> | |
| {labelRendered} | |
| {inputRendered} | |
| </> | |
| ) : ( | |
| <> | |
| {inputRendered} | |
| {labelRendered} | |
| </> | |
| )} | |
| {description && ( | |
| <span id={this.state.descriptionId} className={css(styles.checkDescription)}> | |
| {description} | |
| </span> | |
| )} | |
| {body && <span className={css(styles.checkBody)}>{body}</span>} | |
| </Component> | |
| checkedProps.checked = checkedProps.checked === null ? false : checkedProps.checked; | |
| return ( | |
| const normalizedCheckedProps = { | |
| ...checkedProps, | |
| ...(checkedProps.checked === null ? { checked: false } : {}) | |
| }; | |
| const inputRendered = ( | |
| <input | |
| {...props} | |
| className={css(styles.checkInput, inputClassName)} | |
| type="checkbox" | |
| onChange={this.handleChange} | |
| aria-invalid={!isValid} | |
| aria-label={ariaLabel} | |
| aria-describedby={ariaDescribedByValue} | |
| disabled={isDisabled} | |
| required={isRequired} | |
| ref={(elem) => { | |
| elem && (elem.indeterminate = isChecked === null); | |
| }} | |
| {...normalizedCheckedProps} | |
| {...getOUIAProps(Checkbox.displayName, ouiaId !== undefined ? ouiaId : generatedOuiaId, ouiaSafe)} | |
| /> | |
| ); | |
| const wrapWithLabel = (isLabelWrapped && !component) || component === 'label'; | |
| const Label = wrapWithLabel ? 'span' : 'label'; | |
| const labelRendered = label ? ( | |
| <Label | |
| className={css(styles.checkLabel, isDisabled && styles.modifiers.disabled)} | |
| htmlFor={!wrapWithLabel ? props.id : undefined} | |
| > | |
| {label} | |
| {isRequired && ( | |
| <span className={css(styles.checkLabelRequired)} aria-hidden="true"> | |
| {ASTERISK} | |
| </span> | |
| )} | |
| </Label> | |
| ) : null; | |
| const WrapperComponent = component ?? (wrapWithLabel ? 'label' : 'div'); | |
| return ( |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-core/src/components/Checkbox/Checkbox.tsx` around lines 120 -
159, The input may receive checked={null} because checkedProps.checked is
normalized after inputRendered is created; move the normalization so
checkedProps.checked is set (e.g., checkedProps.checked = checkedProps.checked
=== null ? false : checkedProps.checked) before building inputRendered (the JSX
that uses checkedProps), ensuring the ref indeterminate logic that reads
isChecked still works; update places referencing checkedProps, inputRendered,
isChecked, and WrapperComponent accordingly so the input never receives null for
the checked prop.
Comment on lines
+68
to
+72
| {searchInput && ( | ||
| <div className={css(styles.dualListSelectorToolsFilter)}> | ||
| {searchInput ? searchInput : <SearchInput isDisabled={isDisabled} />} | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
Redundant conditional on line 70.
The inner ternary searchInput ? searchInput : <SearchInput isDisabled={isDisabled} /> is redundant because the outer condition on line 68 already checks for searchInput. If searchInput is truthy, it will always be rendered; the fallback <SearchInput /> can never execute.
Proposed fix
{searchInput && (
<div className={css(styles.dualListSelectorToolsFilter)}>
- {searchInput ? searchInput : <SearchInput isDisabled={isDisabled} />}
+ {searchInput}
</div>
)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-core/src/components/DualListSelector/DualListSelectorPane.tsx`
around lines 68 - 72, The inner ternary is redundant: in DualListSelectorPane
remove the conditional expression "{searchInput ? searchInput : <SearchInput
isDisabled={isDisabled} />}" and simply render "searchInput" inside the div with
className={css(styles.dualListSelectorToolsFilter)} (or, if you intended to show
the default SearchInput when searchInput is falsy, change the outer condition to
always render the div and use the ternary there). Update references to
searchInput and SearchInput accordingly and keep isDisabled passed to the
default SearchInput if you choose the always-render approach.
Comment on lines
+126
to
+130
| ) : ( | ||
| <span className={css(styles.switchToggle)}> | ||
| <div className={css(styles.switchToggleIcon)}> | ||
| <CheckIcon /> | ||
| </div> |
There was a problem hiding this comment.
Respect hasCheckIcon for unlabeled switches.
Line 126-Line 130 render CheckIcon unconditionally when label is omitted, so hasCheckIcon={false} is ignored in that path.
Proposed fix
- ) : (
- <span className={css(styles.switchToggle)}>
- <div className={css(styles.switchToggleIcon)}>
- <CheckIcon />
- </div>
- </span>
- )}
+ ) : (
+ <span className={css(styles.switchToggle)}>
+ {hasCheckIcon && (
+ <span className={css(styles.switchToggleIcon)}>
+ <CheckIcon />
+ </span>
+ )}
+ </span>
+ )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-core/src/components/Switch/Switch.tsx` around lines 126 - 130,
The Switch component currently renders <CheckIcon /> unconditionally in the
unlabeled branch, ignoring the hasCheckIcon prop; update the unlabeled rendering
path inside the Switch component to only render the CheckIcon when hasCheckIcon
is true (same condition used in the labeled branch), e.g. gate the element
rendered inside the node with className css(styles.switchToggleIcon) on
props.hasCheckIcon so that switchToggle and switchToggleIcon markup remains but
the icon is omitted when hasCheckIcon is false.
Comment on lines
+15
to
+17
| : function useSSRSafeId(prefix = 'pf-'): string { | ||
| const [id] = useState(() => `${prefix}${++counter}`); | ||
| return id; |
There was a problem hiding this comment.
React 17 fallback may still cause hydration mismatches.
The counter-based fallback for React 17 uses useState to stabilize the ID per-component, but the module-level counter will reset between server and client rendering. If this code runs on the server (SSR), IDs generated there won't match the client-side IDs since the counter starts fresh on both.
This fallback improves consistency within client-only rendering but doesn't fully solve SSR hydration for React 17. Consider documenting this limitation or logging a warning when the fallback is used in an SSR context.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-core/src/helpers/useSSRSafeId.ts` around lines 15 - 17, The
counter-based fallback in useSSRSafeId can cause SSR hydration mismatches
because the module-level counter resets between server and client; update
useSSRSafeId to detect when the fallback path is taken (e.g., when useId is
unavailable) and when running on the server (typeof window === 'undefined') and
emit a dev-only warning (process.env.NODE_ENV !== 'production') that documents
the limitation and advises using React 18+ or explicit IDs; also add a short
note to the component library docs/README referencing useSSRSafeId and the SSR
limitation so consumers are aware.
nicolethoen
reviewed
Feb 26, 2026
Contributor
nicolethoen
left a comment
nicolethoen
left a comment
There was a problem hiding this comment.
Overall, this could result in snapshots being updated for some consumers, but I think they will change for the better.
I only have two questions related to some regressions flagged by coderabbit.
|
hi @arpanroy41 @dlabaj @nicolethoen thanks for working on this fix. This issue is currently blocking us from upgrading Next.js from 14 to 15 in our project. Next.js 14 has a vulnerability related to React Server Components, so we attempted the upgrade, but Next.js 15 introduces hydration errors with PatternFly. Do you have an estimate of when this fix might be merged or released? It would help us plan our upgrade. Thanks! |
Contributor
|
we are happy to merge this once all review comments have been addressed. @arpanroy41 will you be able to address my comments? |
…oved accessibility - Changed the ouiaComponentType prop in FormSelect to utilize the component's displayName instead of a concatenated string with validation status.
- Updated the logic for modifying the existing aria attribute to use split and filter methods, ensuring that the specified ID is removed correctly.
Contributor
Author
|
@nicolethoen I have resolved the 2 points you have mentioned. Thank you for your help! |
- Modified the ouiaComponentId in FormSelect snapshots to remove the "default" and validation status, ensuring a uniform format across all instances.
Contributor
|
@arpanroy41 Looks like there's a conflict, but other then that it looks good to merge. |
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
5 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.
Replaced the counter-based ID generation with React's useId() hook, which produces deterministic IDs that are consistent between server and client rendering.
New utilities introduced:
useSSRSafeId(prefix?) — A hook that wraps React.useId() (React 18+) with an optional prefix. Falls back to a useState-based counter for React 17 compatibility.
SSRSafeIds — A render-prop component that exposes useSSRSafeId to class-component patterns, generating both a component ID and an optional OUIA ID.
Refactored utilities:
GenerateId — Now delegates to useSSRSafeId internally. API unchanged; isRandom prop is deprecated (no-op).
useOUIAId / useOUIAProps — Now use useSSRSafeId for generated OUIA IDs instead of the global counter.
ID format change
Generated IDs change from sequential integers to React useId() tokens:
Before:
pf-random-id-0, OUIA-Generated-Tabs-3After:
pf-random-id-:r0:, OUIA-Generated-Tabs-:r1:This is a non-breaking change — these IDs are auto-generated internals, not user-facing API. Any consumer passing an explicit id prop is unaffected.
Summary by CodeRabbit
Bug Fixes
Refactor
Tests