chore(omnichannel): fix unsafe type assertions in CannedResponsesComposer#38704
Merged
ggazzo merged 2 commits intoRocketChat:developfrom Feb 24, 2026
Merged
Conversation
…oser Replace as any casts with proper type definitions: - Define CannedResponsesComposerProps overriding onChange to (value: string) => void - Remove two as any casts in useMarkdownSyntax and onClickEmoji handlers - Bridge ChangeEvent to string in MessageComposerInput onChange wrapper - Fix InsertPlaceholderDropdown: onChange: any -> (value: string) => void, name: any -> string
Contributor
|
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 |
|
Contributor
WalkthroughThe PR replaces unsafe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx,js}📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Files:
⏰ 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)
🔇 Additional comments (4)
✏️ Tip: You can disable this entire section by setting 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}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38704 +/- ##
===========================================
- Coverage 70.78% 70.44% -0.35%
===========================================
Files 3159 3175 +16
Lines 109364 111103 +1739
Branches 19671 20064 +393
===========================================
+ Hits 77415 78264 +849
- Misses 29920 30794 +874
- Partials 2029 2045 +16
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
NAME-ASHWANIYADAV
force-pushed
the
fix/canned-responses-composer-unsafe-type
branch
from
18c113f to
caafba9
Compare
KevLehman
added
the
valid
Contributor
Author
|
Hi @KevLehman , this one has contrib: valid and is awaiting review. Since the milestone deadline is coming up, it would be great if this could be reviewed. Let me know if any changes are needed! |
ggazzo
reviewed
Feb 24, 2026
Comment on lines
+19
to
+21
| type CannedResponsesComposerProps = Omit<ComponentProps<typeof MessageComposerInput>, 'onChange'> & { | ||
| onChange: (value: string) => void; | ||
| }; |
Member
There was a problem hiding this comment.
I dont think this is the right type
ggazzo
reviewed
Feb 24, 2026
| <MessageComposerInput | ||
| ref={textAreaRef} | ||
| rows={10} | ||
| onChange={(e: ChangeEvent<HTMLTextAreaElement>): void => onChange(e.target.value)} |
Member
There was a problem hiding this comment.
you changed the way onChange is called, is this a fix or are we introducing a bug?
Contributor
There was a problem hiding this comment.
Pull request overview
Removes unsafe any/type assertions in the Omnichannel canned responses composer by tightening onChange and placeholder typing.
Changes:
- Introduced a dedicated
CannedResponsesComposerPropstype overridingonChangeto(value: string) => void. - Removed
as anycasts when forwarding textarea values toonChange. - Tightened
InsertPlaceholderDropdownprops and placeholder name typing tostring.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| apps/meteor/client/views/omnichannel/cannedResponses/components/CannedResponsesComposer/InsertPlaceholderDropdown.tsx | Replaces any usage with string-typed placeholder insertion APIs. |
| apps/meteor/client/views/omnichannel/cannedResponses/components/CannedResponsesComposer/CannedResponsesComposer.tsx | Replaces unsafe casts and adapts composer input onChange to a string-based callback. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+19
to
+21
| type CannedResponsesComposerProps = Omit<ComponentProps<typeof MessageComposerInput>, 'onChange'> & { | ||
| onChange: (value: string) => void; | ||
| }; |
There was a problem hiding this comment.
onChange is defined as required in CannedResponsesComposerProps, but it’s still invoked with optional chaining (onChange?.(...)). This is inconsistent: either make onChange optional in the props type (if callers may omit it) or remove the optional chaining and treat it as required everywhere for clearer intent.
| } | ||
|
|
||
| onChange?.(textAreaRef.current.value as any); | ||
| onChange?.(textAreaRef.current.value); |
There was a problem hiding this comment.
onChange is defined as required in CannedResponsesComposerProps, but it’s still invoked with optional chaining (onChange?.(...)). This is inconsistent: either make onChange optional in the props type (if callers may omit it) or remove the optional chaining and treat it as required everywhere for clearer intent.
| textAreaRef.current.setSelectionRange(startPos + emojiValue.length, startPos + emojiValue.length); | ||
|
|
||
| onChange?.(textAreaRef.current.value as any); | ||
| onChange?.(textAreaRef.current.value); |
There was a problem hiding this comment.
onChange is defined as required in CannedResponsesComposerProps, but it’s still invoked with optional chaining (onChange?.(...)). This is inconsistent: either make onChange optional in the props type (if callers may omit it) or remove the optional chaining and treat it as required everywhere for clearer intent.
| onChange?.(textAreaRef.current.value as any); | ||
| onChange?.(textAreaRef.current.value); | ||
| } | ||
| }, [char]); |
There was a problem hiding this comment.
The callback dependency list only includes char, but the callback also captures onChange. This can lead to stale onChange being called if the prop changes. Add onChange to the dependency array (and keep textAreaRef out since refs are stable) to satisfy hook rules and avoid subtle bugs.
Suggested change
| }, [char]); | |
| }, [char, onChange]); |
| <MessageComposerInput | ||
| ref={textAreaRef} | ||
| rows={10} | ||
| onChange={(e: ChangeEvent<HTMLTextAreaElement>): void => onChange(e.target.value)} |
There was a problem hiding this comment.
This inline handler allocates a new function each render. Consider wrapping it in useCallback (e.g., const handleChange = useCallback((e) => onChange(e.target.value), [onChange])) and passing handleChange to MessageComposerInput to reduce re-renders in memoized child components.
ggazzo
approved these changes
Feb 24, 2026
Member
|
/jira ARCH-2021 |
Contributor
Author
|
Thankyou @ggazzo 😊😊🙏 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed Changes
This PR resolves technical debt in the CannedResponsesComposer component by removing unsafe type assertions.
as anycasts with proper type definitions in CannedResponsesComposer.tsx.onChangehandler signature fromChangeEventto (value: string) => void, aligning with the usage in CannedResponseForm.any.Issue(s)
Closes #38703
Steps to Test or Reproduce
as anyusage.Further Comments
This change is purely a code quality improvement (chore) and does not affect runtime behavior or UI. It ensures better type safety and maintainability.
Summary by CodeRabbit
Task: ARCH-2065