feat(icons): investigating pf-rh icon hotswapping #12210
Conversation
WalkthroughAdds a pfToRhIcons mapping and threads an optional rhUiIcon through icon export generation and runtime. createIcon now accepts dual icon definitions and a new svgPathData shape; rendering selects between icon and rhUiIcon. MutationObserver-based theme-detection scaffolding is present but inactive. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Component as Icon Component
participant Script as writeIcons (build)
participant Mapping as pfToRhIcons
participant Renderer as Render Method
participant Document
participant Observer as MutationObserver (scaffold)
Note over Script,Mapping: Build-time
Script->>Mapping: import pfToRhIcons
Script->>Script: compute altIcon (rhUiIcon) per icon
Script-->>Component: emit icon configs including rhUiIcon
Note over User,Component: Runtime
User->>Component: Mount (props: set?/icon/rhUiIcon)
activate Component
alt MutationObserver scaffold enabled
Component->>Observer: start observing Document classes
end
Component->>Renderer: render()
activate Renderer
Renderer->>Renderer: choose source (props.set, rhUiIcon, icon)
Renderer->>Renderer: compute viewBox & svg paths from svgPathData
Renderer-->>User: return SVG element
deactivate Renderer
deactivate Component
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
|
Preview: https://pf-react-pr-12210.surge.sh A11y report: https://pf-react-pr-12210-a11y.surge.sh |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/react-icons/src/createIcon.tsx`:
- Around line 18-22: createIcon currently expects an icon object but tests and
generateStaticSVG pass top-level SVG props, so when icon is undefined iconData
becomes undefined and renders empty SVG; update createIcon (and/or the
CreateIconProps handling) to normalize legacy callers by merging top-level
properties into the expected icon shape when props.icon is missing — e.g., if
icon is undefined, build iconData from top-level width, height, svgPath,
svgPathData, viewBox, etc., so functions like createIcon and any usage of
iconData (and generateStaticSVG) continue to work with both new and legacy call
sites.
🧹 Nitpick comments (2)
packages/react-icons/src/pfToRhIcons.js (1)
3-5: Detect missing RH UI icon mappings early.
getIconDatasilently returns null, so typos or missing RH UI icons won’t be obvious and swapping will quietly fail. Consider a build‑time check or warning for missing mappings (especially for the design‑sheet section) so mismatches are surfaced quickly.packages/react-icons/src/createIcon.tsx (1)
49-71: Reduce per‑icon MutationObserver overhead.Each icon instance attaches its own observer to
<html>, which can be costly on icon‑dense screens. Consider a shared singleton observer / global theme state, and at minimum skip the observer whensetis explicitly provided (and guard MutationObserver availability).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/react-icons/scripts/writeIcons.mjspackages/react-icons/src/createIcon.tsxpackages/react-icons/src/pfToRhIcons.js
🧰 Additional context used
🧬 Code graph analysis (2)
packages/react-icons/scripts/writeIcons.mjs (1)
packages/react-icons/src/pfToRhIcons.js (2)
pfToRhIcons(8-316)pfToRhIcons(8-316)
packages/react-icons/src/createIcon.tsx (3)
packages/react-icons/scripts/writeIcons.mjs (1)
createIcon(12-12)packages/react-icons/scripts/parseRHIcons.mjs (2)
xOffset(72-72)svgPathData(86-87)packages/react-icons/scripts/icons/fontawesomeIcons.mjs (1)
width(10-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Lint
- GitHub Check: Build
- GitHub Check: Build, test & deploy
🔇 Additional comments (1)
packages/react-icons/scripts/writeIcons.mjs (1)
20-28: Nice wiring ofrhUiIconthrough generated exports.The CJS/ESM/DTS writers and the generator all include the alternate icon data consistently.
Also applies to: 36-45, 54-61, 125-128
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react-icons/src/__tests__/createIcon.test.tsx (1)
44-47: Test accesses non-existent propertyiconDef.svgPath.
iconDefis typed asCreateIconProps, which does not have asvgPathproperty. This test will always pass incorrectly becauseiconDef.svgPathisundefined, and the assertion compares against an undefined attribute.The correct property path is
iconDef.icon?.svgPathDataor simply usesinglePathIcon.svgPathData.Fix the assertion
test('sets correct svgPath if string', () => { render(<SVGIcon />); - expect(screen.getByRole('img', { hidden: true }).querySelector('path')).toHaveAttribute('d', iconDef.svgPath); + expect(screen.getByRole('img', { hidden: true }).querySelector('path')).toHaveAttribute('d', singlePathIcon.svgPathData); });
🤖 Fix all issues with AI agents
In `@packages/react-icons/src/createIcon.tsx`:
- Around line 49-72: Multiple MutationObserver instances are created per icon in
componentDidMount, which scales poorly; replace the per-instance observer with a
module-level singleton observer (themeObserver) and a
subscribeToThemeChanges(callback) API that maintains a Set listeners; in
componentDidMount subscribe a callback that checks rhUiIcon/canUseDOM and the
this.props.set condition and calls this.setState(prev => ({ themeClassVersion:
prev.themeClassVersion + 1 })), and in componentWillUnmount unsubscribe the
callback to remove it from listeners so the singleton observer only notifies
registered icons and cleans up when none remain.
- Around line 106-112: The code in createIcon.tsx can render <path
d={undefined}> when svgPathData is undefined; update the svgPaths construction
to first check for svgPathData being null/undefined and return null (or an empty
fragment) in that case, otherwise keep the existing Array.isArray(svgPathData)
branch (mapping pathObject.className and pathObject.path with stable keys) and
the string branch; ensure the check references the svgPathData variable so no
invalid <path d={undefined}> is emitted.
♻️ Duplicate comments (2)
packages/react-icons/src/createIcon.tsx (2)
18-22: Handle legacycreateIconcallers that pass top-level icon properties.The new signature expects
{ name, icon, rhUiIcon }, but per the past review, existing call sites (tests andgenerateStaticSVGinwriteIcons.mjs) may pass top-level properties likewidth,height,svgPathData. Wheniconisundefined,iconDatabecomesundefinedat line 91, causing the SVG to render without valid dimensions or paths.Consider adding a fallback to support legacy callers:
Suggested backward-compatible normalization
export function createIcon({ name, icon, rhUiIcon = null }: CreateIconProps): React.ComponentClass<SVGIconProps> { + // Support legacy callers passing top-level icon properties + const normalizedIcon = icon ?? (arguments[0] as IconDefinition); return class SVGIcon extends Component<SVGIconProps, { themeClassVersion: number }> {Then use
normalizedIconin place oficonthroughout.
90-96: Guard against undefinediconDatato prevent runtime errors.When
iconisundefinedandshouldUseAltDatais false,iconDatabecomesundefined. The destructuring at line 91 would yieldundefinedvalues forwidthandheight, resulting in a malformedviewBoxlike"0 0 undefined undefined".Add defensive guard
const iconData = shouldUseAltData ? rhUiIcon : icon; + if (!iconData) { + // Return null or a placeholder when no icon data is available + return null; + } const { xOffset, yOffset, width, height, svgClassName, svgPathData } = iconData ?? {};
🧹 Nitpick comments (1)
packages/react-icons/src/createIcon.tsx (1)
131-196: Remove or extract commented-out experimental code.The large block of commented code clutters the file. If this CSS-based approach is being evaluated, consider moving it to a separate branch, a design document, or a clearly marked experimental file.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/react-icons/src/createIcon.tsx`:
- Around line 124-142: When set === 'unified' but rhUiIcon is null the code sets
iconData to null resulting in invalid viewBox and empty path; update the logic
in createIcon (around the iconData assignment and rendering of viewBox/svgPaths)
to fall back to the default icon when rhUiIcon is missing or short-circuit
early: compute iconData as set === 'unified' ? (rhUiIcon ?? icon) : (set !==
undefined ? icon : (rhUiIcon ?? icon)), and if iconData is still falsy return
null (or render a defined fallback) before using xOffset/width/svgPathData so
you never render viewBox="0 0 undefined undefined" or a path with d={undefined}.
♻️ Duplicate comments (1)
packages/react-icons/src/createIcon.tsx (1)
49-56: Guard against undefinedsvgPathDatato avoid<path d={undefined}>.Both the helper and render branch fall through to a
<path>whensvgPathDatais missing. This yields invalid SVG markup.✅ Suggested guard
- const svgPaths = - svgPathData && Array.isArray(svgPathData) ? ( + const svgPaths = + !svgPathData ? null : Array.isArray(svgPathData) ? ( svgPathData.map((pathObject, index) => ( <path className={pathObject.className} key={`${pathObject.path}-${index}`} d={pathObject.path} /> )) ) : ( <path d={svgPathData as string} /> );- const svgPaths = - svgPathData && Array.isArray(svgPathData) ? ( + const svgPaths = + !svgPathData ? null : Array.isArray(svgPathData) ? ( svgPathData.map((pathObject, index) => ( <path className={pathObject.className} key={`${pathObject.path}-${index}`} d={pathObject.path} /> )) ) : ( <path d={svgPathData as string} /> );Also applies to: 135-142
2 participants
What: Towards #12163
Currently, this PR uses a CSS method to swap between two svgs nested inside a wrapper svg that contains shared metadata, for icons with mapped alternate data. When an icon lacks that alternate data, or when the
setprop is specified, the previous flat svg structure is returned with the proper icon data.Commented out in
createIconis an alternate react method, using thesetprop or by detecting a global class modifier, to hotswap the svg path data / viewbox data within a single flat svg. This method will not respond to dynamic global icon set swapping via the class modifier unless the MutationObserver logic is present, which may impact performance (the icon would remain in the previous set until a user prompts a rerender of the component). However, it may be the expected case that icons are defined at build time/once in the user application and not dynamically swapped, so it may be possible to keep the react method without the MutationObserver.I would be curious for people's thoughts about the two approaches: render two inner svgs per svg & use CSS to swap visibility, or maintain the current svg rendering and use react to swap the data (ignoring any dynamic swapping case).
To test this CSS method, you will have to inspect an element (I tested with the
htmlelement), press the+button to add a new class, select the inspector stylesheet to get access to the local css stylesheet, and then paste the following block:This will allow the icons to be viewed properly and toggled with a
.pf-v6-theme-unifiedclass on thebodyelement. Without this block, both icons will be rendered simultaneously unlesssetis specified (which returns the flat svg instead).Summary by CodeRabbit
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.