feat(icons): investigating pf-rh icon hotswapping #12210
Conversation
WalkthroughAdds a pfToRhIcons mapping and threads an optional rhUiIcon through icon export generation and the runtime createIcon component. The component now accepts dual icon definitions, observes document theme-class changes, and selects which icon to render (icon vs rhUiIcon) based on theme state or explicit prop. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Component as Icon Component
participant Observer as MutationObserver
participant Document
participant State as Component State
participant Renderer as Render Method
User->>Component: Mount component (set prop = undefined)
activate Component
Component->>Observer: Create and start observing document element
Component->>State: Initialize themeClassVersion = 0
deactivate Component
User->>Document: Apply/Remove theme class
activate Document
Document->>Observer: Trigger mutation event
deactivate Document
activate Observer
alt set prop not provided
Observer->>State: Increment themeClassVersion
State->>Component: Trigger re-render
else set prop explicitly set
Observer->>Component: Ignore mutation
end
deactivate Observer
activate Component
Component->>Renderer: Call render()
activate Renderer
Renderer->>Renderer: Determine unified-theme state from themeClassVersion or set prop
Renderer->>Renderer: Select icon source: `rhUiIcon` vs `icon`
Renderer->>Renderer: Compute viewBox and offsets
Renderer->>Renderer: Build SVG paths from string or path-object array
Renderer-->>User: Return SVG with selected icon
deactivate Renderer
deactivate Component
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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.
| export interface CreateIconProps extends IconDefinition { | ||
| name?: string; | ||
| icon?: IconDefinition; | ||
| rhUiIcon?: IconDefinition | null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the createIcon.tsx file to see the full implementation
wc -l packages/react-icons/src/createIcon.tsxRepository: patternfly/patternfly-react
Length of output: 114
🏁 Script executed:
# Read the createIcon.tsx file to understand the actual implementation
cat -n packages/react-icons/src/createIcon.tsxRepository: patternfly/patternfly-react
Length of output: 7994
🏁 Script executed:
# Check if writeIcons.mjs exists and examine the call site mentioned
find . -name "writeIcons.mjs" -type fRepository: patternfly/patternfly-react
Length of output: 116
🏁 Script executed:
# Check the writeIcons.mjs file around lines 80-87
wc -l packages/react-icons/scripts/writeIcons.mjsRepository: patternfly/patternfly-react
Length of output: 118
🏁 Script executed:
# Read the relevant section of writeIcons.mjs
cat -n packages/react-icons/scripts/writeIcons.mjs | head -100 | tail -40Repository: patternfly/patternfly-react
Length of output: 1522
🏁 Script executed:
# Search for all createIcon calls in the codebase
rg -n 'createIcon\s*\(' --type=ts --type=tsx --type=js --type=mjs -A 3Repository: patternfly/patternfly-react
Length of output: 98
🏁 Script executed:
# Try again with corrected file types
rg -n 'createIcon\s*\(' --type=ts --type=js --type=mjs -A 3Repository: patternfly/patternfly-react
Length of output: 98
🏁 Script executed:
# Also search for any exports of icon objects to understand the pattern
rg -n 'export.*Icon\s*=\s*createIcon' --type=ts --type=js --type=mjs -B 2 -A 5Repository: patternfly/patternfly-react
Length of output: 98
🏁 Script executed:
# Search without specific file type flags
rg -n 'createIcon\s*\(' -A 3Repository: patternfly/patternfly-react
Length of output: 1833
🏁 Script executed:
# Also check the actual icon generation files to see how they call createIcon
find . -path ./node_modules -prune -o -name "*.ts" -o -name "*.js" | xargs grep -l "createIcon" | head -10Repository: patternfly/patternfly-react
Length of output: 145
🏁 Script executed:
# Let's look at a sample icon file to understand the calling pattern
find packages/react-icons/src/icons -name "*.ts" -type f | head -3 | xargs catRepository: patternfly/patternfly-react
Length of output: 136
🏁 Script executed:
# Let's look at the test file to understand the proper usage
cat -n packages/react-icons/src/__tests__/createIcon.test.tsx | head -50Repository: patternfly/patternfly-react
Length of output: 1941
🏁 Script executed:
# Search for iconDef and iconDefWithArrayPath definitions in the test
rg -n 'const.*iconDef' packages/react-icons/src/__tests__/ -A 10Repository: patternfly/patternfly-react
Length of output: 2449
🏁 Script executed:
# Let's also check the generated icon files to see the pattern
find packages/react-icons/src -name "*.ts" | head -5Repository: patternfly/patternfly-react
Length of output: 104
🏁 Script executed:
# Let's verify the exact property name issue - tests use svgPath but interface uses svgPathData
rg -n 'svgPath' packages/react-icons/src/__tests__/createIcon.test.tsxRepository: patternfly/patternfly-react
Length of output: 743
🏁 Script executed:
# Check if there's any mapping from svgPath to svgPathData
rg -n 'svgPath' packages/react-icons/src/createIcon.tsxRepository: patternfly/patternfly-react
Length of output: 811
🏁 Script executed:
# Let's see the full picture of how writeIcons.mjs creates icons
cat -n packages/react-icons/scripts/writeIcons.mjs | sed -n '18,50p'Repository: patternfly/patternfly-react
Length of output: 1293
🏁 Script executed:
# Check if the generated icon files use icon parameter or top-level properties
rg -n 'Icon.*Config' packages/react-icons/scripts/writeIcons.mjs -B 5 -A 5Repository: patternfly/patternfly-react
Length of output: 650
🏁 Script executed:
# Check what the actual icon structure looks like in generated files
cat -n packages/react-icons/scripts/writeIcons.mjs | sed -n '100,160p'Repository: patternfly/patternfly-react
Length of output: 2356
🏁 Script executed:
# Verify the actual property names used - is it svgPath or svgPathData?
rg -n '"svgPath' packages/react-icons/scripts/writeIcons.mjsRepository: patternfly/patternfly-react
Length of output: 53
🏁 Script executed:
# Find the generateIcons function to see what the actual icon structure is
rg -n 'function generateIcons' packages/react-icons/scripts/writeIcons.mjs -A 20Repository: patternfly/patternfly-react
Length of output: 53
🏁 Script executed:
# Search for where icons are loaded/parsed
rg -n 'import.*icon' packages/react-icons/scripts/writeIcons.mjs -iRepository: patternfly/patternfly-react
Length of output: 422
🏁 Script executed:
# Let's look at the beginning of writeIcons.mjs to understand the data flow
cat -n packages/react-icons/scripts/writeIcons.mjs | head -80Repository: patternfly/patternfly-react
Length of output: 3331
🏁 Script executed:
# Check if there's a generateIcons.mjs file
cat packages/react-icons/scripts/generateIcons.mjs | head -50Repository: patternfly/patternfly-react
Length of output: 747
🏁 Script executed:
# Now let's understand the actual problem - check what properties the icon object has
rg -n 'svgPathData' packages/react-icons/scripts/ -B 2 -A 2Repository: patternfly/patternfly-react
Length of output: 50384
🏁 Script executed:
# Check the actual test to see if svgPath is the correct property name
cat -n packages/react-icons/src/__tests__/createIcon.test.tsx | sed -n '1,60p'Repository: patternfly/patternfly-react
Length of output: 2347
🏁 Script executed:
# Check the IconDefinition interface and SVGPathObject definition
rg -n 'interface IconDefinition|interface SVGPathObject' packages/react-icons/src/createIcon.tsx -A 5Repository: patternfly/patternfly-react
Length of output: 317
🏁 Script executed:
# Verify what properties tests are trying to use vs what the interface expects
rg -n 'svgPath' packages/react-icons/src/__tests__/createIcon.test.tsxRepository: patternfly/patternfly-react
Length of output: 743
🏁 Script executed:
# Check if there's any type mapping or compatibility layer
rg -n 'svgPath\|svgPathData' packages/react-icons/src/createIcon.tsxRepository: patternfly/patternfly-react
Length of output: 53
🏁 Script executed:
# Check the exact test to see if it actually passes despite the mismatch
cat -n packages/react-icons/src/__tests__/createIcon.test.tsx | grep -A 20 "test('sets correct svgPath if string'"Repository: patternfly/patternfly-react
Length of output: 1107
🏁 Script executed:
# Look at what happens when tests call createIcon with top-level svgPath
# The interface allows it via CreateIconProps extends IconDefinition
# But let's see if there's any actual handling of top-level properties
ast-grep --pattern 'const iconData = $_'Repository: patternfly/patternfly-react
Length of output: 172
🏁 Script executed:
# Check if there's any fallback or compatibility handling for svgPath
rg -n 'svgPath' packages/react-icons/src/ -A 2 -B 2Repository: patternfly/patternfly-react
Length of output: 6210
🏁 Script executed:
# Check if tests are actually running or if there's a TypeScript build issue
find packages/react-icons -name "tsconfig*.json" -o -name "jest.config.*"Repository: patternfly/patternfly-react
Length of output: 144
🏁 Script executed:
# Look at the complete CreateIconProps interface definition
cat -n packages/react-icons/src/createIcon.tsx | sed -n '18,22p'Repository: patternfly/patternfly-react
Length of output: 243
🏁 Script executed:
# Confirm the exact problem: tests use svgPath but interface expects svgPathData
# and implementation only reads from 'icon' parameter
rg -n 'svgPath\b' packages/react-icons/src/__tests__/createIcon.test.tsx | head -5Repository: patternfly/patternfly-react
Length of output: 324
Handle legacy createIcon callers that pass top-level icon properties.
The tests and generateStaticSVG call createIcon() with top-level width, height, svgPath/svgPathData properties, but the implementation expects an icon parameter object containing those properties. When icon is undefined, iconData becomes undefined, resulting in an empty SVG render.
This affects:
- Test suite (packages/react-icons/src/tests/createIcon.test.tsx lines 22-23)
- Static SVG generation (packages/react-icons/scripts/writeIcons.mjs lines 80-87)
Either normalize legacy input within createIcon, update all call sites to pass { icon: { width, height, svgPathData, ... } }, or maintain backward compatibility by reading top-level properties as a fallback.
🤖 Prompt for AI Agents
In `@packages/react-icons/src/createIcon.tsx` around lines 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.
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.
| componentDidMount() { | ||
| if (rhUiIcon !== null && canUseDOM) { | ||
| this.observer = new MutationObserver((mutations) => { | ||
| for (const mutation of mutations) { | ||
| if (mutation.type === 'attributes' && mutation.attributeName === 'class') { | ||
| const target = mutation.target as HTMLElement; | ||
| const hadClass = (mutation.oldValue || '').includes('pf-v6-theme-unified'); | ||
| const hasClass = target.classList.contains('pf-v6-theme-unified'); | ||
|
|
||
| if (hadClass !== hasClass && this.props.set === undefined) { | ||
| this.setState((prevState) => ({ | ||
| themeClassVersion: prevState.themeClassVersion + 1 | ||
| })); | ||
| } | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| this.observer.observe(document.documentElement, { | ||
| attributes: true, | ||
| attributeFilter: ['class'], | ||
| attributeOldValue: true | ||
| }); | ||
| } |
There was a problem hiding this comment.
MutationObserver per icon instance may degrade performance at scale.
Each icon instance creates its own MutationObserver watching document.documentElement. With hundreds of icons on a page, this creates hundreds of observers that all fire on every class change, each triggering a setState and potential re-render.
Consider a shared singleton observer pattern:
Singleton observer approach
// Module-level singleton
let themeObserver: MutationObserver | null = null;
const listeners = new Set<() => void>();
function subscribeToThemeChanges(callback: () => void): () => void {
if (canUseDOM && !themeObserver) {
themeObserver = new MutationObserver((mutations) => {
for (const mutation of mutations) {
if (mutation.type === 'attributes' && mutation.attributeName === 'class') {
const hadClass = (mutation.oldValue || '').includes('pf-v6-theme-unified');
const hasClass = document.documentElement.classList.contains('pf-v6-theme-unified');
if (hadClass !== hasClass) {
listeners.forEach((cb) => cb());
}
}
}
});
themeObserver.observe(document.documentElement, {
attributes: true,
attributeFilter: ['class'],
attributeOldValue: true
});
}
listeners.add(callback);
return () => listeners.delete(callback);
}Then in the component, subscribe and unsubscribe via componentDidMount/componentWillUnmount.
🤖 Prompt for AI Agents
In `@packages/react-icons/src/createIcon.tsx` around lines 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.
| const svgPaths = Array.isArray(svgPathData) ? ( | ||
| svgPathData.map((pathObject, index) => ( | ||
| <path className={pathObject.className} key={`${pathObject.path}-${index}`} d={pathObject.path} /> | ||
| )) | ||
| ) : ( | ||
| <path d={svgPathData as string} /> | ||
| ); |
There was a problem hiding this comment.
Handle undefined svgPathData gracefully.
If iconData.svgPathData is undefined, the ternary falls through to render <path d={undefined} />, which produces invalid SVG markup.
Add fallback
- const svgPaths = 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} />
);🤖 Prompt for AI Agents
In `@packages/react-icons/src/createIcon.tsx` around lines 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.
2 participants
What: Towards #12163
Generated a mapping object for existing PatternFly icons to sync with RH-UI icons based on the design team's google sheet, and added another section of mappings vai Cursor looking for icons whose naming schemes matched (but these will need to be verified). We will want to do a full audit mapping our icons to RH icons.
Currently this uses a react method of swapping svg data via
set="unified"prop, or by a MutationObserver watching thehtmlclassList for a global css class "pf-v6-theme-unified" (which can be anything). I'm not sure of the overall impact of MutationObserver, but without it icons would not update if a global setting was triggered until they re-rendered. Which could maybe be possible from the app-side but wouldn't be guaranteed.I was tinkering with using CSS to try to show/hide the different icons, but the previous method I was trying out worked for
pathobjects and the svgPathData, and didn't account for the icons differing viewbox data (commented out in the PR for viewing). We would likely instead need to have the icon component render two full separate svgs with different classes applied to control the visibility rather than just the different paths. I'm not sure if that's better or worse than the impact of MutationObserver and would love feedback.Summary by CodeRabbit
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.