chore: improve GenericMenu TypeScript typing#38594
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: b553092 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
No actionable comments were generated in the recent review. 🎉 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx,js}📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Files:
🔇 Additional comments (3)
WalkthroughGenericMenu gained a typed Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Improves the TypeScript typing of GenericMenu’s button prop to ensure it is clonable and removes an unsafe as any cast, aiming to increase type safety without changing runtime behavior.
Changes:
- Adds an explicit
ReactElementtype for thebuttonprop. - Removes the
as anycast when callingcloneElement(button, ...).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thanks for the reviews! This is a small type-safety improvement only, with no runtime behavior changes. Happy to adjust anything if needed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.changeset/chatty-hounds-reflect.md (1)
1-5: LGTM! Changeset is well-formed and appropriate.The changeset correctly uses
patchfor a type-safety improvement with no runtime behavior changes, and the format follows changesets conventions.Optional suggestion: Consider making the description slightly more specific to help future readers understand what changed:
📝 More detailed description option
--- '@rocket.chat/ui-client': patch --- -Improve type safety for GenericMenu component props +Improve type safety for GenericMenu component by adding ReactElement type to button prop and removing 'as any' cast🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.changeset/chatty-hounds-reflect.md around lines 1 - 5, Update the changeset description to be more specific about what changed: clarify that the patch improves type safety for the GenericMenu component's props (e.g., tighten prop generics or typings), mention that this is a non-runtime change, and keep the changeset type as 'patch' for package '@rocket.chat/ui-client' so readers immediately know which component and package the change affects.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.changeset/chatty-hounds-reflect.md
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.changeset/chatty-hounds-reflect.md:
- Around line 1-5: Update the changeset description to be more specific about
what changed: clarify that the patch improves type safety for the GenericMenu
component's props (e.g., tighten prop generics or typings), mention that this is
a non-runtime change, and keep the changeset type as 'patch' for package
'@rocket.chat/ui-client' so readers immediately know which component and package
the change affects.
- Add explicit ReactElement type for button prop - Remove unnecessary 'as any' cast in cloneElement call - TypeScript now knows button is safe to clone without type assertion Without explicit button?: ReactElement, TypeScript would infer button as ReactNode from MenuV2 props spread, making cloneElement unsafe. This explicit type makes the code type-safe without needing 'as any'.
… from arm64 to amd64
TheRazorbill
force-pushed
the
fix/improve-generic-menu-typing
branch
from
673b84f to
3016926
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38594 +/- ##
========================================
Coverage 70.67% 70.67%
========================================
Files 3191 3191
Lines 112963 112963
Branches 20451 20456 +5
========================================
Hits 79832 79832
+ Misses 31085 31082 -3
- Partials 2046 2049 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Hi team 👋 |
Proposed changes
This PR improves TypeScript type safety in
GenericMenuby:ReactElementtype for thebuttonpropas anycast incloneElementThis ensures
buttonis safely clonable and avoids unsafe type assertions, without changing runtime behavior.Issue(s)
Closes #38748
Steps to test or reproduce
Example:
yarn lint yarn testBefore
After
Summary by CodeRabbit
New Features
Refactor
Chores