chore: replace unsafe any in CannedResponseList onClickItem with proper type#39001
chore: replace unsafe any in CannedResponseList onClickItem with proper type#39001Agarwalchetan wants to merge 3 commits intoRocketChat:developfrom
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 |
|
WalkthroughAdded a new back-action prop and narrowed the item click prop type across the canned responses list: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/client/views/omnichannel/cannedResponses/contextualBar/CannedResponse/CannedResponseList.tsx (1)
23-38: Consider extracting the repeated intersection type into a named alias.The inline intersection
IOmnichannelCannedResponse & { departmentName?: ILivechatDepartment['name'] }is now duplicated verbatim on lines 25 (cannedItems) and 34 (onClickItem). A named alias keeps the two props obviously in sync and makes future type changes a single-point edit.♻️ Proposed refactor: extract a shared type alias
+type CannedResponseItem = IOmnichannelCannedResponse & { departmentName?: ILivechatDepartment['name'] }; + type CannedResponseListProps = { loadMoreItems: () => void; - cannedItems: (IOmnichannelCannedResponse & { departmentName?: ILivechatDepartment['name'] })[]; + cannedItems: CannedResponseItem[]; itemCount: number; onClose: () => void; options: [string, string][]; text: string; setText: FormEventHandler<HTMLInputElement>; type: string; setType: Dispatch<SetStateAction<string>>; isRoomOverMacLimit: boolean; - onClickItem: (data: IOmnichannelCannedResponse & { departmentName?: ILivechatDepartment['name'] }) => void; + onClickItem: (data: CannedResponseItem) => void; onClickCreate: (e: MouseEvent<HTMLOrSVGElement>) => void; onClickUse: (e: MouseEvent<HTMLOrSVGElement>, text: string) => void; reload: () => void; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/omnichannel/cannedResponses/contextualBar/CannedResponse/CannedResponseList.tsx` around lines 23 - 38, The inline intersection type IOmnichannelCannedResponse & { departmentName?: ILivechatDepartment['name'] } is duplicated in the CannedResponseListProps definition; extract it into a named alias (e.g., CannedResponseWithDepartment) and replace both cannedItems and onClickItem signatures to use that alias so future changes are single-point edits and both props stay in sync (update the type alias and references in the CannedResponseListProps declaration).
🤖 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/cannedResponses/contextualBar/CannedResponse/CannedResponseList.tsx`:
- Around line 23-38: The inline intersection type IOmnichannelCannedResponse & {
departmentName?: ILivechatDepartment['name'] } is duplicated in the
CannedResponseListProps definition; extract it into a named alias (e.g.,
CannedResponseWithDepartment) and replace both cannedItems and onClickItem
signatures to use that alias so future changes are single-point edits and both
props stay in sync (update the type alias and references in the
CannedResponseListProps declaration).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/client/views/omnichannel/cannedResponses/contextualBar/CannedResponse/CannedResponseList.tsx
📜 Review details
⏰ 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: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
🧰 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/cannedResponses/contextualBar/CannedResponse/CannedResponseList.tsx
🔇 Additional comments (1)
apps/meteor/client/views/omnichannel/cannedResponses/contextualBar/CannedResponse/CannedResponseList.tsx (1)
70-78: Type mismatch:onClickItemcallback signature does not matchonClickBackrequirements.
onClickItemexpects a data object(data: IOmnichannelCannedResponse & { departmentName?: ILivechatDepartment['name'] }) => void, butonClickBackis typed asMouseEventHandler<HTMLOrSVGElement>(i.e.,(event: MouseEvent<HTMLOrSVGElement>) => void). Passing a data callback where an event handler is expected is a type incompatibility unrelated to contravariance of optional properties.Likely an incorrect or invalid review comment.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/client/views/omnichannel/cannedResponses/contextualBar/CannedResponse/CannedResponseList.spec.tsx (1)
88-99:⚠️ Potential issue | 🟡 MinorPre-existing test name contradicts its assertion.
The test is named
should NOT render Create button when user has only save-department-canned-responses permission, but the assertion isexpect(screen.getByText('Create')).toBeInTheDocument()— a positive assertion. Compare with line 115 which correctly pairs a "should NOT render" name withqueryByText(...).not.toBeInTheDocument(). Either the test description needs to drop "NOT", or the assertion needs to flip to match the description.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/omnichannel/cannedResponses/contextualBar/CannedResponse/CannedResponseList.spec.tsx` around lines 88 - 99, The test with description "should NOT render Create button when user has only save-department-canned-responses permission" is contradictory because it asserts expect(screen.getByText('Create')).toBeInTheDocument(); fix by making the behavior and assertion consistent: either rename the test string to "should render Create button when user has only save-department-canned-responses permission" or change the assertion to expect(screen.queryByText('Create')).not.toBeInTheDocument(); update the test that calls render(<CannedResponseList {...defaultProps} />) with mockAppRoot().withPermission('save-department-canned-responses') accordingly so the test name and assertion match (adjust the string in the it(...) call or replace getByText with queryByText + .not.toBeInTheDocument()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@apps/meteor/client/views/omnichannel/cannedResponses/contextualBar/CannedResponse/CannedResponseList.spec.tsx`:
- Around line 88-99: The test with description "should NOT render Create button
when user has only save-department-canned-responses permission" is contradictory
because it asserts expect(screen.getByText('Create')).toBeInTheDocument(); fix
by making the behavior and assertion consistent: either rename the test string
to "should render Create button when user has only
save-department-canned-responses permission" or change the assertion to
expect(screen.queryByText('Create')).not.toBeInTheDocument(); update the test
that calls render(<CannedResponseList {...defaultProps} />) with
mockAppRoot().withPermission('save-department-canned-responses') accordingly so
the test name and assertion match (adjust the string in the it(...) call or
replace getByText with queryByText + .not.toBeInTheDocument()).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/meteor/client/views/omnichannel/cannedResponses/contextualBar/CannedResponse/CannedResponseList.spec.tsxapps/meteor/client/views/omnichannel/cannedResponses/contextualBar/CannedResponse/CannedResponseList.stories.tsxapps/meteor/client/views/omnichannel/cannedResponses/contextualBar/CannedResponse/CannedResponseList.tsxapps/meteor/client/views/omnichannel/cannedResponses/contextualBar/CannedResponse/WrapCannedResponseList.tsx
📜 Review details
⏰ 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). (1)
- GitHub Check: cubic · AI code reviewer
🧰 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/cannedResponses/contextualBar/CannedResponse/CannedResponseList.stories.tsxapps/meteor/client/views/omnichannel/cannedResponses/contextualBar/CannedResponse/CannedResponseList.spec.tsxapps/meteor/client/views/omnichannel/cannedResponses/contextualBar/CannedResponse/CannedResponseList.tsxapps/meteor/client/views/omnichannel/cannedResponses/contextualBar/CannedResponse/WrapCannedResponseList.tsx
🧠 Learnings (1)
📚 Learning: 2026-02-24T19:36:47.551Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82
Timestamp: 2026-02-24T19:36:47.551Z
Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').
Applied to files:
apps/meteor/client/views/omnichannel/cannedResponses/contextualBar/CannedResponse/CannedResponseList.spec.tsx
🔇 Additional comments (5)
apps/meteor/client/views/omnichannel/cannedResponses/contextualBar/CannedResponse/CannedResponseList.spec.tsx (1)
69-69: LGTM —onClickBack: jest.fn()correctly satisfies the new required prop.apps/meteor/client/views/omnichannel/cannedResponses/contextualBar/CannedResponse/WrapCannedResponseList.tsx (1)
56-64: LGTM —onClickBackhandler is correctly implemented.Usage of
useEffectEventis consistent with the existingonClickItempattern. Navigating without acontextparam correctly strips the canned-response detail context, returning to the list view.apps/meteor/client/views/omnichannel/cannedResponses/contextualBar/CannedResponse/CannedResponseList.stories.tsx (1)
76-78: LGTM — story args correctly extended for the expanded public API.apps/meteor/client/views/omnichannel/cannedResponses/contextualBar/CannedResponse/CannedResponseList.tsx (2)
34-35: LGTM — type updates are clean and internally consistent.
onClickItemnow matches thecannedItemselement type (line 25) andonClickBack: MouseEventHandler<HTMLOrSVGElement>aligns with the handler signature inWrapCannedResponseList.tsx.
70-80: No issues found.WrapCannedResponse.tsxcorrectly declaresonClickBackas a required prop (line 13 of its type definition) and does not declareonClickItem. The code inCannedResponseList.tsxproperly passesonClickBack={onClickBack}without causing TypeScript errors.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #39001 +/- ##
===========================================
- Coverage 70.98% 70.92% -0.07%
===========================================
Files 3209 3209
Lines 113896 113900 +4
Branches 20677 20638 -39
===========================================
- Hits 80844 80778 -66
- Misses 30997 31064 +67
- Partials 2055 2058 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ggazzo
changed the title
ggazzo
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
|
Hi, just a gentle follow-up on this PR |
dionisio-bot
bot
removed
the
stat: ready to merge
4 participants
Related Issue / Discussion
Resolves the existing
// FIXME: fix typingscomment inCannedResponseList.tsx.Changes
Modified
CannedResponseList.tsxanyin theonClickItemprop with the appropriate composite type already used in the same component.Further Details
Summary
The
onClickItemprop was previously defined as:This has been updated to use the same type as the elements of
cannedItemsdefined in the same props interface:The composite type matches:
Impact
anyCompatibility
WrapCannedResponseListpasses a stricter (non-optional)departmentName, which is structurally compatiblejest.fn()foronClickItem, which remains validThis PR strictly addresses the explicit FIXME comment and does not modify runtime behavior.
Summary by CodeRabbit
COMM-144