feat(ui): streamline icon imports and enhance dynamic icon mapping#152
feat(ui): streamline icon imports and enhance dynamic icon mapping#152
Conversation
kylengn
requested review from
EnzalRad,
dvxam and
noe-charmet
as code owners
|
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 icon component's import strategy is refactored from direct named imports to a namespace-based approach. A dynamic icon discovery layer is introduced that converts Remix icon names to camelCase keys, merges custom icons with runtime-scanned Remix icons into a unified mapping, and updates the BaseIconProps type signature to reflect this new architecture. Changes
Sequence DiagramsequenceDiagram
participant Init as Module Init
participant Utils as Utility Functions
participant CustomIcons as Custom Icons Map
participant RemixNS as RemixIcons Namespace
participant Dynamic as Dynamic Discovery
participant Final as Unified iconsMap
Init->>Utils: Define regex & toCamelCase
Init->>CustomIcons: Build custom icons (google, microsoft)
Init->>RemixNS: Access RemixIcons namespace
RemixNS->>Dynamic: Scan for Ri* prefixed icons
Dynamic->>Dynamic: Convert to camelCase keys
Dynamic->>Dynamic: Filter out already-mapped customs
Dynamic->>Final: Merge customIcons + dynamicIconsMap
Final->>Init: Export unified iconsMap & IconName
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 use dynamic icon mapping instead of individual imports, enabling automatic access to all Remix icons through a wildcard import and camelCase name conversion.
- Replaced individual icon imports with
import * as RemixIconsto access all icons - Added dynamic icon mapping that converts PascalCase Remix icon names to camelCase (e.g.,
RiGoogleFill→google) - Implemented automatic icon registration by filtering and mapping all Remix icons that don't conflict with custom icons
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
🧹 Nitpick comments (2)
libs/react/ui/src/components/icon/icon.tsx (2)
64-79: Dynamic spread intoiconsMaplikely widensIconNametostring, weakening type safetyBecause
dynamicIconsMapis typed asRecord<string, RemixiconComponentType>and then spread intoiconsMap, the inferred type oficonsMapbecomes something like:{ /* literal keys from customIcons */ } & Record<string, RemixiconComponentType>For such an intersection,
keyof typeof iconsMapeffectively collapses tostring. That means:
IconName = keyof typeof iconsMapis now juststring, not the literal union of known keys.<Icon name="totallyWrong" />will type‑check, even thoughiconsMap["totallyWrong"]will beundefined, leading to an invalid React element (IconComponentisundefined) at runtime.If you want to preserve a strongly‑typed public surface while still benefiting from dynamic discovery, consider splitting the concerns:
Keep a curated map for the public API, e.g.:
const staticIconsMap = customIcons; export type IconName = keyof typeof staticIconsMap;Use
iconsMap(with dynamic spread) internally where you really need “all Remix icons”, or expose a separateallIconNames: string[]/allIconsMapfor tooling like icon pickers.This keeps the runtime behavior you added but restores compile‑time checking for consumer‑facing
IconProps.name.Please confirm in your editor what
IconNamecurrently resolves to; if it’sstring, I’d strongly consider tightening it as above.Also applies to: 81-87
89-90:BaseIconPropsupdate is consistent with the new namespace import; optional decoupling from a specific iconSwitching from
ComponentProps<typeof RiGoogleFill>toComponentProps<typeof RemixIcons.RiGoogleFill>is a mechanical, correct change that keeps the Icon props aligned with a representative Remix icon while matching the new import style.If you ever want to avoid tying
BaseIconPropsto a single concrete icon, you could instead extract props fromRemixiconComponentTypevia a conditional type (e.g.,type BaseIconProps = RemixiconComponentType extends React.ComponentType<infer P> ? P : never;), but that’s purely optional and what you have now is consistent with the previous design.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/react/ui/src/components/icon/icon.tsx(3 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/icon.tsx
🧬 Code graph analysis (1)
libs/react/ui/src/components/icon/icon.tsx (3)
libs/react/ui/src/components/icon/custom/shipfox-logo.tsx (1)
ShipfoxLogo(8-20)libs/react/ui/src/components/icon/custom/slack-logo.tsx (1)
SlackLogo(6-35)libs/react/ui/src/components/icon/custom/stripe-logo.tsx (1)
StripeLogo(8-27)
⏰ 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 (2)
libs/react/ui/src/components/icon/icon.tsx (2)
21-23: Camel‑case helper and numeric prefix handling look correct
RI_PREFIX_REGEX/NUMERIC_START_REGEXplustoCamelCaseproduce the expected keys (e.g.,RiHomeSmileFill→homeSmileFill,Ri2KLine→i2KLine). The logic is straightforward and matches the new dynamic mapping needs.Also applies to: 55-62
24-53:customIconsmap additions are consistent and readableThe expanded
customIconsobject cleanly mixes custom SVG logos with explicit Remix aliases (google, microsoft, imageAdd, etc.), and the keys follow the existing camelCase naming convention. Marking itas constkeeps key names precise for typing. No issues from a correctness or clarity standpoint.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@noe-charmet can you help review and approve this PR please? |
2 participants
Summary by CodeRabbit