Refactor/icon button prop #6063
Conversation
Refactors the IconButton component to accept a mandatory 'icon' prop instead of rendering children. This change addresses a TODO comment and makes the component's API more explicit and type-safe, ensuring an icon is always provided. All existing call sites for IconButton have been updated to use the new 'icon' prop, passing in the appropriate icons from lucide-react.
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
|
Important Review skippedAuto 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. ✨ 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 |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the IconButton component to use a dedicated icon prop instead of relying on the generic children prop, making the API more explicit and type-safe.
- Replaced
childrenprop with a mandatoryiconprop of typeElementType - Added
iconSizeClassesmapping to automatically size icons based on button size - Updated all usages across the codebase to pass icon components directly
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/client/src/components/agent-prism/IconButton.tsx | Refactored component to accept icon prop instead of children, added icon size mapping |
| packages/client/src/components/agent-prism/DetailsView/DetailsViewInputOutputTab.tsx | Updated CopyButton to pass icon component via icon prop |
| packages/client/src/components/agent-prism/DetailsView/DetailsViewHeader.tsx | Updated copy button to pass icon component via icon prop |
| packages/client/src/components/agent-prism/CollapseAndExpandControls.tsx | Updated expand/collapse buttons to pass icon components via icon prop |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| {...rest} | ||
| > | ||
| {children} | ||
| <Icon size={iconSizeClasses[size]} /> |
There was a problem hiding this comment.
[nitpick] The icon size is now controlled by the component, but consumers previously had control over icon sizing via className (e.g., 'size-3', 'size-3.5'). This change removes that flexibility. Consider whether the hardcoded size mapping accurately reflects the previous sizing, or if allowing className overrides on the icon would be beneficial.
Removed the 'ref' line from the checkout step.
1 participant
Relates to
This PR addresses a
TODOcomment found inpackages/client/src/components/agent-prism/IconButton.tsx. No issue is formally associated with this change.Risks
Low. This change is a refactoring of a single UI component and all of its usages have been updated. The change is localized to the client-side presentation layer.
Background
What does this PR do?
This PR refactors the
IconButtoncomponent to make its API more explicit and type-safe. It replaces the use of the genericchildrenprop for passing icons with a dedicated and mandatoryiconprop. This ensures that everyIconButtonis always instantiated with an icon component.All existing usages of
IconButtonhave been updated to conform to the new API, passing the appropriatelucide-reacticon component to the new prop.What kind of change is this?
Improvements (misc. changes to existing features)
Documentation changes needed?
My changes do not require a change to the project documentation.
Testing
Where should a reviewer start?
A good starting point is reviewing the changes in
packages/client/src/components/agent-prism/IconButton.tsxto see the new component API.After that, the modified call sites can be reviewed to see how the new API is used:
packages/client/src/components/agent-prism/CollapseAndExpandControls.tsxpackages/client/src/components/agent-prism/DetailsView/DetailsViewHeader.tsxpackages/client/src/components/agent-prism/DetailsView/DetailsViewInputOutputTab.tsxDetailed testing steps
Automated tests are acceptable. The component tests for the
clientpackage should cover these changes.A manual verification can also be done by running the client and navigating to pages where the
IconButtonis used (e.g., the agent details view) to ensure the copy and expand/collapse icons render and function correctly.