chore: align CurrentChatTags handler type with AutoCompleteTagsMultiple onChange#38879
chore: align CurrentChatTags handler type with AutoCompleteTagsMultiple onChange#38879Agarwalchetan wants to merge 4 commits into
Conversation
…le onChange to remove as any
|
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 |
|
WalkthroughType and usage updates for tag handlers: Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/client/views/omnichannel/additionalForms/CurrentChatTags.tsx (1)
7-7: Nitpick:valueprop type is not aligned withAutoCompleteTagsMultiple.
handleris now derived fromAutoCompleteTagsMultiple'sonChangeprop, butvalueis still a hardcodedArray<{ value: string; label: string }>. Per the PR description,AutoCompleteTagsMultipleusesstring | numberfor value entries — keeping a hand-rolled narrower type here is inconsistent with the refactor's intent. It's harmless today (string ⊆ string | number), but could silently reject valid callers passing numeric values.♻️ Optional alignment
- value: Array<{ value: string; label: string }>; + value: NonNullable<ComponentProps<typeof AutoCompleteTagsMultiple>['value']>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/omnichannel/additionalForms/CurrentChatTags.tsx` at line 7, The prop type for value in the CurrentChatTags component is narrower than AutoCompleteTagsMultiple's items and should be widened to accept numeric values too; update the value prop type used in CurrentChatTags (the value prop and any local type alias it uses) from Array<{ value: string; label: string }> to Array<{ value: string | number; label: string }> so it matches AutoCompleteTagsMultiple and prevents rejecting numeric tag values.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/client/views/omnichannel/additionalForms/CurrentChatTags.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/views/omnichannel/additionalForms/CurrentChatTags.tsx
⏰ 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: 📦 Build Packages
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
apps/meteor/client/views/omnichannel/additionalForms/CurrentChatTags.tsx (1)
8-8: Verify thathandlerhasn't inadvertently become optional.
ComponentProps<typeof AutoCompleteTagsMultiple>['onChange']resolves to exactly what the underlying component declares.AutoCompleteTagsMultipleinherits fromComponentProps<typeof PaginatedMultiSelectFiltered>without explicitly overriding theonChangetype, yet provides a default parameteronChange = () => undefined. If the underlying component'sonChangeprop is optional, the extracted type would includeundefined, silently makinghandleroptional. This would be inconsistent with all current usages, which pass non-nullable values from React Hook Form field callbacks.Verify the type of
onChangeinPaginatedMultiSelectFilteredfrom@rocket.chat/fuselage. If optional, consider usingNonNullableto preserve the required behavior:Proposed fix if `onChange` is optional in the underlying component
- handler: ComponentProps<typeof AutoCompleteTagsMultiple>['onChange']; + handler: NonNullable<ComponentProps<typeof AutoCompleteTagsMultiple>['onChange']>;
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/client/views/omnichannel/additionalForms/CurrentChatTags.tsx`:
- Line 7: The prop type for value in the CurrentChatTags component is narrower
than AutoCompleteTagsMultiple's items and should be widened to accept numeric
values too; update the value prop type used in CurrentChatTags (the value prop
and any local type alias it uses) from Array<{ value: string; label: string }>
to Array<{ value: string | number; label: string }> so it matches
AutoCompleteTagsMultiple and prevents rejecting numeric tag values.
…le onChange to remove as any
…rwalchetan/Rocket.Chat into fix/current-chat-tags-as-any # Conflicts: # apps/meteor/client/views/omnichannel/additionalForms/CurrentChatTags.tsx
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38879 +/- ##
===========================================
+ Coverage 70.55% 70.57% +0.01%
===========================================
Files 3189 3189
Lines 112703 112695 -8
Branches 20429 20420 -9
===========================================
+ Hits 79519 79535 +16
+ Misses 31123 31095 -28
- Partials 2061 2065 +4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Hi, just a gentle follow-up on this PR |
Removes the
as anytype assertion inCurrentChatTags.tsxby aligning thehandlerprop type with theonChangetype ofAutoCompleteTagsMultiple.Problem
CurrentChatTagsmanually defined thehandlerprop as:However,
AutoCompleteTagsMultipleexpects anonChangesignature derived fromPaginatedMultiSelectProps, where:Because
stringis narrower thanstring | number, TypeScript rejected the assignment and required an unsafe cast:Solution
The
handlertype is now derived directly from the component it is passed to:This keeps the prop fully aligned with
AutoCompleteTagsMultipleand prevents future type drift. Theas anycast has been removed.Since this change widens the accepted handler type, the callback in
Tags.tsxwas also updated to match the correctonChangesignature and avoid narrowing the parameter type.Changes
handlertype inCurrentChatTagsPropsto derive fromAutoCompleteTagsMultiple['onChange']as anyfromonChangeTags.tsxhandler callback to align with the widenedonChangetypeVerification
yarn typecheck)Summary by CodeRabbit