feat(ui): refactor Icon component to use remixicon registry#172
feat(ui): refactor Icon component to use remixicon registry#172
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe pull request refactors icon management by centralizing Remix icon imports into a dynamic registry file, merging custom icons with Remix icons into a unified map, and broadening the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the Icon component to dynamically load all Remix icons from a registry instead of using hardcoded imports. This change makes all Remix icons available through the Icon component rather than just a curated subset of 14 icons.
Key Changes:
- Introduced
remixicon-registry.tsthat builds a dynamic map of all Remix icons by filtering exports and transforming icon names - Modified
icon.tsxto merge the full Remix icon registry with custom icons, replacing explicit icon imports - Changed icon naming convention from custom short names (e.g.,
google,check,close) to systematic names derived from Remix component names (e.g.,googleFill,checkLine,closeLine)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| libs/react/ui/src/components/icon/remixicon-registry.ts | Creates a registry of all Remix icons by filtering and transforming icon component names, enabling dynamic access to the full icon library |
| libs/react/ui/src/components/icon/icon.tsx | Replaces hardcoded icon imports with the dynamic registry, merging all Remix icons with custom icons while changing the icon naming convention |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
libs/react/ui/src/components/icon/remixicon-registry.ts (3)
4-7: Double‑check thetypeof value === 'function'filter against your installed@remixicon/reactThe prefix + typeof guard is a nice way to exclude non‑icon exports, but it assumes every icon export from
@remixicon/reacthastypeof === 'function'. If a future (or current) version ever exposes icons as exotic components wheretypeofis'object', they’ll be silently dropped andremixiconMapwill end up incomplete.Consider either:
- Relaxing the guard (e.g. also allowing
'object') while still excluding obvious non‑icon exports, or- Asserting this behaviour in a small test that fails loudly if icons stop being functions.
9-12: Optionally tighteniconNameToKeytypingThe normalization logic is straightforward, but you could make the intent clearer and catch misuse by constraining the input type, e.g.:
function iconNameToKey(iconName: `Ri${string}`): string { ... }This is purely a type‑level improvement; runtime behaviour stays the same.
19-25:RemixIconNameis effectively juststringdue to theRecord<string, …>castBecause
remixiconMapis asserted asRecord<string, RemixiconComponentType>,keyof typeof remixiconMap(and thusRemixIconName) collapses tostringinstead of a finite union of normalized icon keys. That’s fine if you only need a generic string index, but if the goal is stronger typing/autocomplete on allowed Remix icon names, this cast prevents it.If you want a narrower
RemixIconName, you’ll likely need either:
- A generated type/registry (build‑time codegen), or
- A manually maintained
type RemixIconName = 'alertFill' | 'addLine' | ...that you use to type the map, e.g.Record<RemixIconName, RemixiconComponentType>.libs/react/ui/src/components/icon/icon.tsx (1)
38-44:IconNameis now effectivelystring; consider whether that loss of narrowing is acceptableWith:
const iconsMap = { ...remixiconMap, ...customIconsMap, } as Record<string, RemixiconComponentType> & typeof customIconsMap; export type IconName = keyof typeof iconsMap;
keyof typeof iconsMapwidens tostringbecause of theRecord<string, …>part, soIconNameno longer encodes the concrete set of icon keys (Remix + custom). That meansIconwill happily accept any string at the type level, even thoughiconsMap[name]may beundefinedat runtime.If you want to preserve stricter typing, possible directions:
- Drop the
Record<string, …>from the cast and let TS infer, or- Shape the type explicitly, e.g.:
type IconName = RemixIconName | keyof typeof customIconsMap; const iconsMap: Record<IconName, RemixiconComponentType> = { ...remixiconMap, ...customIconsMap, };If you’re okay with
IconNamebeing juststring, this is fine as‑is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/react/ui/src/components/icon/icon.tsx(2 hunks)libs/react/ui/src/components/icon/remixicon-registry.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit configuration file
We handle errors at the edge of our applications in most cases. Do not recommend to add error handling around every single function. We prefer them to bubble up and be handled at upper layers.
Files:
libs/react/ui/src/components/icon/remixicon-registry.tslibs/react/ui/src/components/icon/icon.tsx
🧬 Code graph analysis (1)
libs/react/ui/src/components/icon/icon.tsx (1)
libs/react/ui/src/components/icon/remixicon-registry.ts (1)
remixiconMap(19-22)
⏰ 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). (2)
- GitHub Check: Agent
- GitHub Check: Continuous integration
🔇 Additional comments (3)
libs/react/ui/src/components/icon/icon.tsx (2)
21-36: Good use ofsatisfiesto keep custom icons aligned with Remix propsTyping
customIconsMapwithas const satisfies Record<string, RemixiconComponentType>is a solid pattern: it preserves literal keys while enforcing that each custom icon matches the Remix icon component contract.
46-47: Base icon props generalization looks good; verify compatibility with your TS/React versionsSwitching to:
type BaseIconProps = ComponentProps<RemixiconComponentType>; type IconProps = {name: IconName} & Omit<BaseIconProps, 'name'>;nicely aligns the
IconAPI with whatever props@remixicon/reactexposes (e.g.size,color,className) and keepsnameunder your control.Just ensure your installed versions of React/TypeScript and
@remixicon/reactagree onRemixiconComponentTypebeing a valid argument toComponentProps; if not, you may need to switch toComponentPropsWithoutRefor a custom props alias.libs/react/ui/src/components/icon/remixicon-registry.ts (1)
1-2: No issues found—dependency is properly declaredThe verification confirms that
@remixicon/reactis already listed inlibs/react/ui/package.jsonas a dependency with version^4.6.0. The imports in this file are properly supported and will not rely on transitive installs.
…ctor registry filtering
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.