Fix/nextjs hydration errors 12250#12251
Open
arpanroy41 wants to merge 9 commits intopatternfly:mainfrom
Open
Conversation
WalkthroughThis PR systematically refactors ID generation across ~50 components and helper utilities to use SSR-safe patterns. It replaces unsafe ID generators ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes This PR involves systematic refactoring across 50+ files with consistent but non-trivial pattern changes. While each individual file change follows predictable patterns (replacing ID generation approaches), the heterogeneity of changes (functional hooks, class wrappers, state removal, lifecycle adjustments) across diverse component types requires careful verification that ID generation semantics are preserved and SSR safety is achieved. Key risk areas include OUIA prop wiring, state removal correctness, and render-prop flow transitions. 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)
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 |
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
+76
to
+77
| <SSRSafeIds prefix="pf-" ouiaComponentType={`FormSelect${validated ? `-${validated}` : ''}`}> | ||
| {(_, generatedOuiaId) => ( |
There was a problem hiding this comment.
Keep generated OUIA IDs stable across validation state changes.
On Line 76, ouiaComponentType is tied to validated, so the generated OUIA id changes as validation changes. That makes data-ouia-component-id unstable for the same mounted control.
Proposed fix
- <SSRSafeIds prefix="pf-" ouiaComponentType={`FormSelect${validated ? `-${validated}` : ''}`}>
+ <SSRSafeIds prefix="pf-" ouiaComponentType={FormSelect.displayName}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-core/src/components/FormSelect/FormSelect.tsx` around lines 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.
Contributor
There was a problem hiding this comment.
CodeRsbbit's comment here is relevant.
The PR passes ouiaComponentType={\FormSelect${validated ?
-${validated} : ''}}toSSRSafeIds. Since validated can change at runtime,
the OUIA ID will change with it. The old code computed this once in the
constructor and stored it in state.
There was a problem hiding this comment.
@nicolethoen, thank you for confirming! You're absolutely right—this is a legitimate issue.
The problem: The current code passes a dynamic ouiaComponentType that includes the validated prop:
ouiaComponentType={`FormSelect${validated ? `-${validated}` : ''}`}Since SSRSafeIds uses ouiaComponentType as part of the key for generating the OUIA ID (via useSSRSafeId), every time validated changes (e.g., from 'default' → 'error' → 'success'), a new OUIA ID is generated. This breaks the stability contract—OUIA IDs should remain constant for a mounted component instance.
The fix: Use the stable FormSelect.displayName instead:
ouiaComponentType={FormSelect.displayName}This ensures the generated OUIA ID stays the same throughout the component's lifecycle, regardless of validation state changes. The validation state itself can still be exposed via other data attributes if needed for testing purposes, but shouldn't be part of the component type identifier.
✏️ Learnings added
Learnt from: nicolethoen
Repo: patternfly/patternfly-react PR: 12251
File: packages/react-core/src/components/FormSelect/FormSelect.tsx:76-77
Timestamp: 2026-02-26T22:33:56.823Z
Learning: In PatternFly React components using SSRSafeIds, the ouiaComponentType parameter must be a stable string (typically the component's displayName) that does not change during the component's lifecycle. Do not include dynamic props like validation state in ouiaComponentType, as this will cause the generated OUIA ID to change, breaking OUIA stability guarantees. Dynamic state should be exposed through separate data attributes if needed.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: arpanroy41
Repo: patternfly/patternfly-react PR: 12237
File: packages/react-core/src/components/Toolbar/examples/ToolbarFlexGrowAndWidths.tsx:6-6
Timestamp: 2026-02-06T19:58:49.801Z
Learning: In the PatternFly React repository, React may be used without an explicit import in TSX files because React is provided globally by the build configuration. You can reference React types (e.g., React.FunctionComponent, React.ReactNode) without importing React. Do not add 'import React from 'react'' in TSX files; rely on the global React availability.
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
+166
to
+167
| const generatedId = useSSRSafeId('pf-tooltip-'); | ||
| const id = idProp ?? generatedId; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/react-core/src/components/Tooltip/Tooltip.tsx | head -200 | tail -50Repository: patternfly/patternfly-react
Length of output: 2213
🏁 Script executed:
# Search for the cleanup logic mentioned at line 213
rg -n "removeAriaFromRefElement\|replace.*RegExp.*\\\\b.*id" packages/react-core/src/components/Tooltip/Tooltip.tsx -B2 -A2Repository: patternfly/patternfly-react
Length of output: 53
🏁 Script executed:
# Find the useSSRSafeId hook to understand what format it returns
fd useSSRSafeId packages/react-core -t ts -t tsxRepository: patternfly/patternfly-react
Length of output: 240
🏁 Script executed:
# Check what useSSRSafeId actually does
rg -n "useSSRSafeId" packages/react-core/src -B2 -A5 -t ts -t tsx | head -100Repository: patternfly/patternfly-react
Length of output: 98
🏁 Script executed:
# Read around line 213 to see the cleanup logic
sed -n '200,225p' packages/react-core/src/components/Tooltip/Tooltip.tsxRepository: patternfly/patternfly-react
Length of output: 740
🏁 Script executed:
# Search for removeAriaFromRefElement function
rg -n "removeAriaFromRefElement" packages/react-core/src/components/Tooltip/Tooltip.tsx -A10Repository: patternfly/patternfly-react
Length of output: 765
🏁 Script executed:
# Find useSSRSafeId definition
fd useSSRSafeId packages/react-core/srcRepository: patternfly/patternfly-react
Length of output: 118
🏁 Script executed:
cat packages/react-core/src/helpers/useSSRSafeId.tsRepository: patternfly/patternfly-react
Length of output: 703
🏁 Script executed:
# Search for React useId usage in the codebase to understand the ID format
rg "useId\|generatedId" packages/react-core/src/helpers/useSSRSafeId.ts -B5 -A5Repository: patternfly/patternfly-react
Length of output: 53
🏁 Script executed:
# Test the regex behavior with useId-style IDs
python <<'PY'
import re
# Simulate React useId format
id_value = "pf-tooltip-:r0:"
existing_aria = f"other-tooltip {id_value} some-id"
# Current regex pattern from line 213
pattern = rf"\b{id_value}\b"
result = existing_aria.replace(re.compile(pattern, re.MULTILINE), '', 1).strip()
print(f"ID value: {id_value}")
print(f"Input aria: {existing_aria}")
print(f"Pattern: {pattern}")
print(f"After cleanup: {result}")
print(f"Token removed: {id_value not in result}")
print()
# Let's also test with the proposed fix
def split_filter_approach(existing_aria, id_value):
return ' '.join(token for token in existing_aria.split() if token and token != id_value)
result2 = split_filter_approach(existing_aria, id_value)
print(f"With proposed fix: {result2}")
print(f"Token removed (proposed): {id_value not in result2}")
PYRepository: patternfly/patternfly-react
Length of output: 203
🏁 Script executed:
# Also test React 17 fallback format (counter-based)
python <<'PY'
import re
# React 17 fallback format (simple counter)
id_value_simple = "pf-tooltip-1"
existing_aria = f"other-tooltip {id_value_simple} some-id"
# Current regex pattern
pattern = rf"\b{id_value_simple}\b"
result = re.sub(pattern, '', existing_aria).strip()
print(f"Simple ID (React 17 fallback): {id_value_simple}")
print(f"Input aria: {existing_aria}")
print(f"After cleanup: {result}")
print(f"Token removed: {id_value_simple not in result}")
PYRepository: patternfly/patternfly-react
Length of output: 219
🏁 Script executed:
# Test the regex behavior with useId-style IDs (corrected)
python <<'PY'
import re
# Simulate React useId format
id_value = "pf-tooltip-:r0:"
existing_aria = f"other-tooltip {id_value} some-id"
# Current regex pattern from line 213 using re.sub (not replace)
pattern = rf"\b{id_value}\b"
result = re.sub(pattern, '', existing_aria).strip()
print(f"ID value: {id_value}")
print(f"Input aria: {existing_aria}")
print(f"Pattern: {pattern}")
print(f"After cleanup with \b: {result}")
print(f"Token removed: {id_value not in result}")
print()
# Let's test with escaped id
escaped_id = re.escape(id_value)
pattern_escaped = rf"\b{escaped_id}\b"
result_escaped = re.sub(pattern_escaped, '', existing_aria).strip()
print(f"Pattern with escaped ID: {pattern_escaped}")
print(f"After cleanup with escaped \b: {result_escaped}")
print(f"Token removed (escaped): {id_value not in result_escaped}")
print()
# Test the proposed fix
def split_filter_approach(existing_aria, id_value):
return ' '.join(token for token in existing_aria.split() if token and token != id_value)
result2 = split_filter_approach(existing_aria, id_value)
print(f"With proposed fix: {result2}")
print(f"Token removed (proposed): {id_value not in result2}")
PYRepository: patternfly/patternfly-react
Length of output: 476
useId-style IDs break aria cleanup due to \b word boundary mismatch.
With React 18+ using useId(), generated IDs like pf-tooltip-:r0: cause the cleanup regex at Line 213 to fail. Word boundaries (\b) don't match correctly when IDs contain colons (non-word characters), leaving stale aria-describedby / aria-labelledby attributes on the trigger element. This breaks accessibility for screen reader users.
🔧 Proposed fix
- const newAria = existingAria.replace(new RegExp(`\\b${id}\\b`, 'g'), '').trim();
+ const newAria = existingAria
+ .split(/\s+/)
+ .filter((token) => token && token !== id)
+ .join(' ');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-core/src/components/Tooltip/Tooltip.tsx` around lines 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.
Contributor
There was a problem hiding this comment.
CodeRsbbit's comment here is relevant.
The removeAriaFromRefElement function uses \b word
boundaries to strip the tooltip's ID from aria-describedby. Colons in useId()
IDs (:r0:) are non-word characters, so the regex will fail to match, leaving
stale IDs in aria attributes.
I recommend replacing the regex with split/filter/join:
old
const newAria = existingAria.replace(new RegExp(`\\b${id}\\b`, 'g'), '').trim();
New
const newAria = existingAria.split(/\s+/).filter((token) => token && token !== id).join(' ');
There was a problem hiding this comment.
@nicolethoen, thank you for confirming the issue and providing the recommended fix! You're absolutely right—the split/filter/join approach is much more robust and avoids the regex special character issues entirely.
Would you like me to implement this fix? I can update the removeAriaFromRefElement function to use the split/filter/join approach you've recommended, which will properly handle useId-style IDs with colons.
Task: Fix aria cleanup for useId-style IDs in Tooltip
- Update removeAriaFromRefElement function to use split/filter/join instead of regex with \b word boundaries
- This ensures proper cleanup of aria attributes when IDs contain special characters like colons
🧠 Learnings used
Learnt from: arpanroy41
Repo: patternfly/patternfly-react PR: 12237
File: packages/react-core/src/components/Toolbar/examples/ToolbarFlexGrowAndWidths.tsx:6-6
Timestamp: 2026-02-06T19:58:49.801Z
Learning: In the PatternFly React repository, React may be used without an explicit import in TSX files because React is provided globally by the build configuration. You can reference React types (e.g., React.FunctionComponent, React.ReactNode) without importing React. Do not add 'import React from 'react'' in TSX files; rely on the global React availability.
💡 For code changes across multiple files, use @coderabbitai plan in an issue comment
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.
Comment on lines
+166
to
+167
| const generatedId = useSSRSafeId('pf-tooltip-'); | ||
| const id = idProp ?? generatedId; |
Contributor
There was a problem hiding this comment.
CodeRsbbit's comment here is relevant.
The removeAriaFromRefElement function uses \b word
boundaries to strip the tooltip's ID from aria-describedby. Colons in useId()
IDs (:r0:) are non-word characters, so the regex will fail to match, leaving
stale IDs in aria attributes.
I recommend replacing the regex with split/filter/join:
old
const newAria = existingAria.replace(new RegExp(`\\b${id}\\b`, 'g'), '').trim();
New
const newAria = existingAria.split(/\s+/).filter((token) => token && token !== id).join(' ');
Comment on lines
+76
to
+77
| <SSRSafeIds prefix="pf-" ouiaComponentType={`FormSelect${validated ? `-${validated}` : ''}`}> | ||
| {(_, generatedOuiaId) => ( |
Contributor
There was a problem hiding this comment.
CodeRsbbit's comment here is relevant.
The PR passes ouiaComponentType={\FormSelect${validated ?
-${validated} : ''}}toSSRSafeIds. Since validated can change at runtime,
the OUIA ID will change with it. The old code computed this once in the
constructor and stored it in state.
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
3 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
Release Notes
Bug Fixes
Refactor