fix(createIcon): Fix broken API in createIcon.tsx#12336
Open
tlabaj wants to merge 2 commits intopatternfly:mainfrom
Open
fix(createIcon): Fix broken API in createIcon.tsx#12336tlabaj wants to merge 2 commits intopatternfly:mainfrom
tlabaj wants to merge 2 commits intopatternfly:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughcreateIcon now accepts both legacy flat and new nested icon shapes, splits and extends icon typings, adds runtime normalization to prefer Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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-12336.surge.sh A11y report: https://pf-react-pr-12336-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: 2
🤖 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-icons/src/createIcon.tsx`:
- Around line 86-99: normalizeCreateIconArg currently allows nested
CreateIconProps to return with icon undefined which later causes invalid SVG
output; update normalizeCreateIconArg (the isNestedCreateIconProps branch) to
validate that p.icon is present (not null/undefined) and throw a clear Error
(e.g., "createIcon: missing default 'icon' for nested props 'name'") if it's
missing before calling normalizeIconDefinition; still normalize rhUiIcon as
before using normalizeIconDefinition when non-null. Ensure the thrown message
references the name (p.name) so callers can identify the bad input.
- Around line 17-30: The public IconDefinition union causes consumers to need
type-narrowing; replace the union by exporting a single interface that extends
IconDefinitionBase and includes both svgPathData and the deprecated svgPath
(marked in JSDoc), then update the exported IconDefinition type to that single
interface; specifically, remove IconDefinitionWithSvgPath |
IconDefinitionWithSvgPathData as the union, keep IconDefinitionWithSvgPathData
and IconDefinitionWithSvgPath (or collapse them) but ensure IconDefinition
refers to the unified interface so helpers can access svgPathData directly and
existing internals like resolveSvgPathData() continue to work without changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d1c895fb-106c-4779-b20e-c9d2064e6bd2
📒 Files selected for processing (2)
packages/react-icons/src/__tests__/createIcon.test.tsxpackages/react-icons/src/createIcon.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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-icons/src/__tests__/createIcon.test.tsx`:
- Around line 1-2: Remove the default React import and the accompanying ESLint
suppression in createIcon.test.tsx: delete the line importing React (the default
identifier "React") and the comment "// eslint-disable-next-line
no-restricted-imports -- test file excluded from package tsconfig; default
import satisfies TS/JSX", since the project uses the automatic JSX runtime and
default React imports are disallowed by the no-restricted-imports rule.
- Around line 198-216: The test 'set="rh-ui" with no rhUiIcon mapping falls back
to default and warns' currently restores the jest.spyOn mock
(warnSpy.mockRestore()) only at the end, which can leak a mocked console.warn if
an assertion throws; wrap the test's execution (render, expects) in a
try-finally and call warnSpy.mockRestore() in the finally block so the spy
created for console.warn is always restored; locate the warnSpy declaration in
this test (const warnSpy = jest.spyOn(console, 'warn').mockImplementation(...))
and move the restore into a finally guarding the render/expect code for
IconNoRhMapping created via createIcon.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cd163c56-8281-441d-960a-7bf1888bbda2
📒 Files selected for processing (2)
packages/react-icons/src/__tests__/createIcon.test.tsxpackages/react-icons/src/createIcon.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-icons/src/createIcon.tsx
tlabaj
requested review from
a team,
kmcfaul and
wise-king-sullyman
and removed request for
a team
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react-icons/src/createIcon.tsx (1)
199-203: Simplify branch condition and tightenNormalizedCreateIconProps.iconto non-optional.Two small, related cleanups in this block:
- Line 199:
(set === undefined && rhUiIcon === null) || set !== undefinedreduces toset !== undefined || rhUiIcon === null.- Line 200-201:
set !== undefined && set === 'rh-ui'— the first conjunct is redundant;set === 'rh-ui'already impliesset !== undefined.NormalizedCreateIconProps.icon(line 86) is typed optional, butnormalizeCreateIconArgalways assigns it (nested path throws when null at line 97-101; legacy path always callsnormalizeIconDefinition). Making it non-optional lets you drop theiconData ?? ({} as Partial<…>)fallback on line 203 and the| undefinedon line 200, keeping the runtime invariant reflected in the types.♻️ Proposed refactor
On line 86:
- icon?: IconDefinitionWithSvgPathData; + icon: IconDefinitionWithSvgPathData;On lines 199-203:
- if ((set === undefined && rhUiIcon === null) || set !== undefined) { - const iconData: IconDefinitionWithSvgPathData | undefined = - set !== undefined && set === 'rh-ui' && rhUiIcon !== null ? rhUiIcon : icon; - const { xOffset, yOffset, width, height, svgPathData, svgClassName } = - iconData ?? ({} as Partial<IconDefinitionWithSvgPathData>); + if (set !== undefined || rhUiIcon === null) { + const iconData: IconDefinitionWithSvgPathData = + set === 'rh-ui' && rhUiIcon !== null ? rhUiIcon : icon; + const { xOffset, yOffset, width, height, svgPathData, svgClassName } = iconData;You can then drop the
icon && …guard on line 250 as well (still safe, just redundant).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-icons/src/createIcon.tsx` around lines 199 - 203, Simplify the conditional and tighten the icon type: change the branch `(set === undefined && rhUiIcon === null) || set !== undefined` to `set !== undefined || rhUiIcon === null`, replace `set !== undefined && set === 'rh-ui'` with `set === 'rh-ui'`, and make NormalizedCreateIconProps.icon non-optional in normalizeCreateIconArg so callers always get an IconDefinition; then remove the `| undefined` from the iconData declaration and drop the fallback `iconData ?? ({} as Partial<...>)` as well as the redundant `icon && …` guard elsewhere (e.g., the check around svg rendering).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/react-icons/src/createIcon.tsx`:
- Around line 199-203: Simplify the conditional and tighten the icon type:
change the branch `(set === undefined && rhUiIcon === null) || set !==
undefined` to `set !== undefined || rhUiIcon === null`, replace `set !==
undefined && set === 'rh-ui'` with `set === 'rh-ui'`, and make
NormalizedCreateIconProps.icon non-optional in normalizeCreateIconArg so callers
always get an IconDefinition; then remove the `| undefined` from the iconData
declaration and drop the fallback `iconData ?? ({} as Partial<...>)` as well as
the redundant `icon && …` guard elsewhere (e.g., the check around svg
rendering).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c738d752-fb52-452d-97ef-f346e6a210a6
📒 Files selected for processing (2)
packages/react-icons/src/__tests__/createIcon.test.tsxpackages/react-icons/src/createIcon.tsx
There was a problem hiding this comment.
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/createIcon.tsx (1)
198-207:⚠️ Potential issue | 🟡 Minor
console.warnfires on every render; also simplify redundant conditions.
- The warning at Lines 200-202 is inside
render(), so every re-render of a parent using<MyIcon set="rh-ui" />against an icon with norhUiIconre-logs the same message — easy to flood the console. Consider gating it with astaticdev-only flag on the class (or moving the check tocomponentDidMount) so it logs at most once per component.- The guard at Line 205 —
(set === undefined && rhUiIcon === null) || set !== undefined— reduces toset !== undefined || rhUiIcon === null.- At Line 207,
set !== undefined && set === 'rh-ui'is equivalent to justset === 'rh-ui'.Proposed readability tweaks (warn dedup left to your discretion)
- if ((set === undefined && rhUiIcon === null) || set !== undefined) { + if (set !== undefined || rhUiIcon === null) { const iconData: IconDefinitionWithSvgPathData | undefined = - set !== undefined && set === 'rh-ui' && rhUiIcon !== null ? rhUiIcon : icon; + set === 'rh-ui' && rhUiIcon !== null ? rhUiIcon : icon;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-icons/src/createIcon.tsx` around lines 198 - 207, The console.warn inside render() for when set === 'rh-ui' and rhUiIcon === null causes repeated logs on every render; move that warning to componentDidMount (or gate it with a static dev-only seen flag on the component) so it runs at most once per mounted component, and remove the eslint-disable comment. Also simplify the render guard expression by replacing "(set === undefined && rhUiIcon === null) || set !== undefined" with "set !== undefined || rhUiIcon === null", and replace the ternary condition "set !== undefined && set === 'rh-ui'" with simply "set === 'rh-ui'" when computing iconData (refer to render(), rhUiIcon, set, icon, and the IconDefinitionWithSvgPathData assignment).
🧹 Nitpick comments (2)
packages/react-icons/src/createIcon.tsx (2)
122-152: Consolidate duplicated path/viewBox rendering with the single-SVG branch.Lines 124-150 and the inline single-SVG branch at Lines 208-225 compute the same
viewBox,svgClassNamemerge, andsvgPathsarray/string rendering. Extracting a shared helper (e.g.buildSvgInner(icon)returning{ viewBox, svgClassName, svgPaths }) would remove the duplication and keep future fixes (e.g., accessibility or keying tweaks) in one place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-icons/src/createIcon.tsx` around lines 122 - 152, The createSvg implementation duplicates viewBox, className merging, and svgPaths construction that are also used in the single-SVG rendering branch; extract that shared logic into a new helper (e.g., buildSvgInner(icon)) which returns { viewBox, svgClassName, svgPaths } and have both createSvg and the single-SVG render call buildSvgInner instead of duplicating the code; ensure the helper uses the same symbol names (xOffset, yOffset, width, height, svgPathData, svgClassName) and preserves path keying/className behavior so both createSvg and the single-SVG branch reuse identical viewBox, class merging, and path rendering.
89-120: TightenNormalizedCreateIconProps.iconto required.Both branches of
normalizeCreateIconArgguaranteeiconis set (legacy always normalizesarg; nested throws if missing). Makingiconrequired on the internal type removes the need for theiconData ?? ({} as Partial<…>)fallback at Line 209 and theicon ?? {}at Line 124, and eliminates the| undefinedoniconDataat Line 206.Proposed type tightening
interface NormalizedCreateIconProps { name?: string; - icon?: IconDefinitionWithSvgPathData; + icon: IconDefinitionWithSvgPathData; rhUiIcon: IconDefinitionWithSvgPathData | null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-icons/src/createIcon.tsx` around lines 89 - 120, The NormalizedCreateIconProps.interface currently marks `icon` optional but both branches of `normalizeCreateIconArg` always provide a normalized icon; update the interface so `icon` is required (change `icon?: IconDefinitionWithSvgPathData` to `icon: IconDefinitionWithSvgPathData`) and set `rhUiIcon: IconDefinitionWithSvgPathData | null` remains; then update any callers that previously used `icon ?? {}` or `iconData ?? ({} as Partial<...>)` (references: `normalizeCreateIconArg`, usages around the former `icon ?? {}` and `iconData` handling) to rely on the now-required `icon` without fallback, removing the `| undefined` from `iconData` types and the fallback expressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/react-icons/src/createIcon.tsx`:
- Around line 198-207: The console.warn inside render() for when set === 'rh-ui'
and rhUiIcon === null causes repeated logs on every render; move that warning to
componentDidMount (or gate it with a static dev-only seen flag on the component)
so it runs at most once per mounted component, and remove the eslint-disable
comment. Also simplify the render guard expression by replacing "(set ===
undefined && rhUiIcon === null) || set !== undefined" with "set !== undefined ||
rhUiIcon === null", and replace the ternary condition "set !== undefined && set
=== 'rh-ui'" with simply "set === 'rh-ui'" when computing iconData (refer to
render(), rhUiIcon, set, icon, and the IconDefinitionWithSvgPathData
assignment).
---
Nitpick comments:
In `@packages/react-icons/src/createIcon.tsx`:
- Around line 122-152: The createSvg implementation duplicates viewBox,
className merging, and svgPaths construction that are also used in the
single-SVG rendering branch; extract that shared logic into a new helper (e.g.,
buildSvgInner(icon)) which returns { viewBox, svgClassName, svgPaths } and have
both createSvg and the single-SVG render call buildSvgInner instead of
duplicating the code; ensure the helper uses the same symbol names (xOffset,
yOffset, width, height, svgPathData, svgClassName) and preserves path
keying/className behavior so both createSvg and the single-SVG branch reuse
identical viewBox, class merging, and path rendering.
- Around line 89-120: The NormalizedCreateIconProps.interface currently marks
`icon` optional but both branches of `normalizeCreateIconArg` always provide a
normalized icon; update the interface so `icon` is required (change `icon?:
IconDefinitionWithSvgPathData` to `icon: IconDefinitionWithSvgPathData`) and set
`rhUiIcon: IconDefinitionWithSvgPathData | null` remains; then update any
callers that previously used `icon ?? {}` or `iconData ?? ({} as Partial<...>)`
(references: `normalizeCreateIconArg`, usages around the former `icon ?? {}` and
`iconData` handling) to rely on the now-required `icon` without fallback,
removing the `| undefined` from `iconData` types and the fallback expressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 991b1b0d-13d1-4420-84cb-4800498423f9
📒 Files selected for processing (1)
packages/react-icons/src/createIcon.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/react-icons/src/createIcon.tsx (3)
39-43:CreateIconProps.iconis optional in the type but required at runtime — lift the check into the type.
normalizeCreateIconArgthrows whenp.icon == nullfor the nested branch (line 105), buticon?: IconDefinitionhere lets TypeScript acceptcreateIcon({ rhUiIcon })orcreateIcon({ name: 'X' })without compile error, yielding a runtime failure instead of a type error. Since the nested shape is new in this PR, tightening the contract costs nothing and surfaces mistakes at the call site.Suggested change
export interface CreateIconProps { name?: string; - icon?: IconDefinition; + icon: IconDefinition; rhUiIcon?: IconDefinition | null; }You can keep the
p.icon == nullruntime guard as defense-in-depth, but the type should reflect the real contract so consumers using TypeScript get an immediate error rather than discovering the failure at runtime.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-icons/src/createIcon.tsx` around lines 39 - 43, The CreateIconProps interface declares icon as optional but normalizeCreateIconArg and the nested branch in createIcon (check p.icon == null) require icon at runtime; change CreateIconProps.icon from optional to required (make it IconDefinition not IconDefinition | undefined) so calls like createIcon({ rhUiIcon }) or createIcon({ name }) are compile-time errors, and keep the existing p.icon == null runtime guard in normalizeCreateIconArg/createIcon as defense-in-depth; update any related call sites/types that assumed icon could be omitted to satisfy the tightened contract.
91-96: Nit:NormalizedCreateIconProps.iconcan be non-optional after normalization.Post-
normalizeCreateIconArgtheiconfield is always set (the nested branch throws on missing, the legacy branch assigns unconditionally). Making the field required here removes the need for theiconData ?? ({} as Partial<…>)fallback on line 215 and theicon ?? {}fallback on line 126, simplifying the render/createSvg paths.Suggested change
interface NormalizedCreateIconProps { name?: string; - icon?: IconDefinitionWithSvgPathData; + icon: IconDefinitionWithSvgPathData; rhUiIcon: IconDefinitionWithSvgPathData | null; }With that in place:
-const createSvg = (icon: IconDefinitionWithSvgPathData, iconClassName: string) => { - const { xOffset, yOffset, width, height, svgPathData, svgClassName } = icon ?? {}; +const createSvg = (icon: IconDefinitionWithSvgPathData, iconClassName: string) => { + const { xOffset, yOffset, width, height, svgPathData, svgClassName } = icon;- const iconData: IconDefinitionWithSvgPathData | undefined = - set !== undefined && set === 'rh-ui' && rhUiIcon !== null ? rhUiIcon : icon; - const { xOffset, yOffset, width, height, svgPathData, svgClassName } = - iconData ?? ({} as Partial<IconDefinitionWithSvgPathData>); + const iconData: IconDefinitionWithSvgPathData = + set === 'rh-ui' && rhUiIcon !== null ? rhUiIcon : icon; + const { xOffset, yOffset, width, height, svgPathData, svgClassName } = iconData;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-icons/src/createIcon.tsx` around lines 91 - 96, Make NormalizedCreateIconProps.icon required (remove the optional ?) because normalizeCreateIconArg always sets icon; update the interface NormalizedCreateIconProps accordingly, then remove the defensive fallbacks that assume icon may be undefined (the uses of icon ?? {} and iconData ?? ({} as Partial<...>)), and simplify the render/createSvg code paths to rely on a present icon object; keep normalizeCreateIconArg and the branch that throws on missing icon as-is to guarantee this invariant.
211-213: Nit: simplify the branch predicate and the redundantset !== undefinedguard.
(set === undefined && rhUiIcon === null) || set !== undefinedreduces toset !== undefined || rhUiIcon === null. Likewise, in the ternaryset !== undefined && set === 'rh-ui', the first clause is implied by the second. Not a bug, but the intent reads cleaner:Suggested change
- if ((set === undefined && rhUiIcon === null) || set !== undefined) { + if (set !== undefined || rhUiIcon === null) { const iconData: IconDefinitionWithSvgPathData | undefined = - set !== undefined && set === 'rh-ui' && rhUiIcon !== null ? rhUiIcon : icon; + set === 'rh-ui' && rhUiIcon !== null ? rhUiIcon : icon;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-icons/src/createIcon.tsx` around lines 211 - 213, The conditional in createIcon (file createIcon.tsx) is more complex than necessary; simplify the if predicate to "set !== undefined || rhUiIcon === null" and simplify the ternary that builds iconData by removing the redundant "set !== undefined" check so it becomes "set === 'rh-ui' ? rhUiIcon : icon"; update the if branch and the iconData assignment in the createIcon function to use these simplified expressions and remove the redundant guards.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/react-icons/src/createIcon.tsx`:
- Around line 39-43: The CreateIconProps interface declares icon as optional but
normalizeCreateIconArg and the nested branch in createIcon (check p.icon ==
null) require icon at runtime; change CreateIconProps.icon from optional to
required (make it IconDefinition not IconDefinition | undefined) so calls like
createIcon({ rhUiIcon }) or createIcon({ name }) are compile-time errors, and
keep the existing p.icon == null runtime guard in
normalizeCreateIconArg/createIcon as defense-in-depth; update any related call
sites/types that assumed icon could be omitted to satisfy the tightened
contract.
- Around line 91-96: Make NormalizedCreateIconProps.icon required (remove the
optional ?) because normalizeCreateIconArg always sets icon; update the
interface NormalizedCreateIconProps accordingly, then remove the defensive
fallbacks that assume icon may be undefined (the uses of icon ?? {} and iconData
?? ({} as Partial<...>)), and simplify the render/createSvg code paths to rely
on a present icon object; keep normalizeCreateIconArg and the branch that throws
on missing icon as-is to guarantee this invariant.
- Around line 211-213: The conditional in createIcon (file createIcon.tsx) is
more complex than necessary; simplify the if predicate to "set !== undefined ||
rhUiIcon === null" and simplify the ternary that builds iconData by removing the
redundant "set !== undefined" check so it becomes "set === 'rh-ui' ? rhUiIcon :
icon"; update the if branch and the iconData assignment in the createIcon
function to use these simplified expressions and remove the redundant guards.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 706ba73c-cc1d-4ab6-8167-a389cf68491b
📒 Files selected for processing (2)
packages/react-icons/src/__tests__/createIcon.test.tsxpackages/react-icons/src/createIcon.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-icons/src/tests/createIcon.test.tsx
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
2 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.
What: Closes #12328
What was changed:
CreateIconPropswithicon/rhUiIcon) and the legacy flatcreateIcon({ … svgPath … })by normalizing at runtime (detect viaicon/rhUiIconkeys).svgPathorsvgPathDataonIconDefinition; resolve tosvgPathDatainternally (svgPathDatawins if both are set).LegacyFlatIconDefinitionas an alias for migration/typing.createIconshapes; RH UI (nested swap markup,setprop, missing-map warning).Summary by CodeRabbit
New Features
Bug Fixes
Tests