fix: Enable validation feedback for Canned Response Tags field#38723
fix: Enable validation feedback for Canned Response Tags field#38723sezallagwal wants to merge 2 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 |
|
WalkthroughCurrentChatTags now accepts an optional Changes
Sequence Diagram(s)(none — changes are prop additions and validation rendering without new multi-component control flow requiring a sequence diagram.) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/meteor/client/views/omnichannel/cannedResponses/components/CannedResponseForm.tsx (2)
102-113: Accessibility: Tags field doesn't follow the samearia-*pattern as Shortcut and Message.The Shortcut and Message fields use
useId()to generate a unique field ID, then wire uparia-describedby={${id}-error},aria-required, andaria-invalidon the input, with the matchingidonFieldError. The Tags field instead hardcodesid='tags-error'and doesn't passaria-describedbyoraria-invalidtoTags/its inputs. This breaks screen-reader association between the input and its error message and risks ID collisions.Consider generating a
tagsFieldID viauseId()and threading it through like the other fields.Suggested approach
const departmentField = useId(); + const tagsField = useId();Then in the Controller render:
- <Tags handler={onChange} tags={value} error={errors.tags?.message} /> + <Tags handler={onChange} tags={value} error={errors.tags?.message} aria-describedby={`${tagsField}-error`} aria-invalid={Boolean(errors.tags)} />And for the FieldError:
- <FieldError aria-live='assertive' id='tags-error'> + <FieldError aria-live='assertive' id={`${tagsField}-error`}>(This would also require
Tagsto forwardaria-describedby/aria-invalidto its underlying inputs.)
14-14: Consider removing or updating the TODO comment.The TODO
// TODO: refactor Tags field to get proper validationappears to be partially addressed by this PR. Either update it to reflect remaining work or remove it if this PR fully resolves the validation gap. As per coding guidelines, code comments should be avoided in the implementation.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/meteor/client/views/omnichannel/additionalForms/CurrentChatTags.tsxapps/meteor/client/views/omnichannel/cannedResponses/components/CannedResponseForm.tsxapps/meteor/client/views/omnichannel/components/Tags.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/components/Tags.tsxapps/meteor/client/views/omnichannel/additionalForms/CurrentChatTags.tsxapps/meteor/client/views/omnichannel/cannedResponses/components/CannedResponseForm.tsx
🧠 Learnings (1)
📚 Learning: 2025-09-18T17:32:33.969Z
Learnt from: aleksandernsilva
Repo: RocketChat/Rocket.Chat PR: 36974
File: apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/MessageForm/MessageForm.tsx:124-129
Timestamp: 2025-09-18T17:32:33.969Z
Learning: The ARIA mismatch issue in MessageForm's template field (where FieldError id used templateId instead of messageFormId) was addressed in PR `#36972` through refactoring the template field into a separate TemplateField component, which uses consistent templateFieldId for both aria-describedby and FieldError id.
Applied to files:
apps/meteor/client/views/omnichannel/cannedResponses/components/CannedResponseForm.tsx
🔇 Additional comments (2)
apps/meteor/client/views/omnichannel/components/Tags.tsx (1)
76-85: LGTM! Theerrorprop is correctly forwarded to both theCurrentChatTagsandTextInputbranches, ensuring validation feedback works regardless of which input variant is rendered.Also applies to: 88-101
apps/meteor/client/views/omnichannel/additionalForms/CurrentChatTags.tsx (1)
6-12: LGTM! Theerrorprop is properly typed as an optional string, destructured separately, and coerced to boolean before passing toAutoCompleteTagsMultiple, which is the expected pattern for toggling visual error states.Also applies to: 14-14, 28-28
KevLehman
added
the
valid
There was a problem hiding this comment.
Actionable comments posted: 1
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/additionalForms/CurrentChatTags.tsx (1)
14-21:⚠️ Potential issue | 🟠 MajorError prop is destructured but not forwarded to
AutoCompleteTagsMultiple.The
errorprop is accepted in the component signature but never passed toAutoCompleteTagsMultiple. This means validation feedback won't be displayed when using theCurrentChatTagspath.AutoCompleteTagsMultipleinherits theerrorprop fromPaginatedMultiSelectFiltered, so the prop must be explicitly forwarded.🐛 Proposed fix
- return <AutoCompleteTagsMultiple {...props} onChange={handler} value={value} department={department} viewAll={viewAll} />; + return <AutoCompleteTagsMultiple {...props} onChange={handler} value={value} department={department} viewAll={viewAll} error={error} />;🤖 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` around lines 14 - 21, The CurrentChatTags component destructures an error prop but never forwards it to AutoCompleteTagsMultiple; update the return in CurrentChatTags to pass the error prop through (alongside existing props like value, handler/onChange, department, viewAll) so AutoCompleteTagsMultiple (and its PaginatedMultiSelectFiltered) can display validation feedback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/client/views/omnichannel/components/Tags.tsx`:
- Line 2: The import line in Tags.tsx is malformed: the import of useEffectEvent
from '@rocket.chat/fuselage-hooks' has a URL concatenated to it (merge artifact)
which causes a syntax error; open the Tags.tsx file, locate the import statement
referencing useEffectEvent and remove the trailing URL/merge-conflict text so
the line is a clean import (import { useEffectEvent } from
'@rocket.chat/fuselage-hooks';), then save and run the build/test to verify the
component (Tags component and any usages) compile correctly.
---
Outside diff comments:
In `@apps/meteor/client/views/omnichannel/additionalForms/CurrentChatTags.tsx`:
- Around line 14-21: The CurrentChatTags component destructures an error prop
but never forwards it to AutoCompleteTagsMultiple; update the return in
CurrentChatTags to pass the error prop through (alongside existing props like
value, handler/onChange, department, viewAll) so AutoCompleteTagsMultiple (and
its PaginatedMultiSelectFiltered) can display validation feedback.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/meteor/client/views/omnichannel/additionalForms/CurrentChatTags.tsxapps/meteor/client/views/omnichannel/components/Tags.tsx
📜 Review details
🧰 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/components/Tags.tsxapps/meteor/client/views/omnichannel/additionalForms/CurrentChatTags.tsx
🧠 Learnings (2)
📓 Common learnings
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 39003
File: apps/meteor/client/lib/chats/flows/sendMessage.ts:39-45
Timestamp: 2026-02-24T19:16:35.307Z
Learning: In apps/meteor/client/lib/chats/flows/sendMessage.ts, when sdk.call('sendMessage', ...) throws an error, the message is intentionally left with temp: true (not deleted or cleaned up) to support a future retry UI feature. This allows users to retry sending failed messages rather than losing them.
📚 Learning: 2025-11-17T15:07:13.273Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37398
File: packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx:357-363
Timestamp: 2025-11-17T15:07:13.273Z
Learning: In packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx, IconElement is a presentational, non-actionable element that does not require wrapping in AppIdProvider, similar to plain_text and mrkdwn renderers. Only actionable elements (those with actions, actionId, or interactive behavior) should be wrapped in AppIdProvider.
Applied to files:
apps/meteor/client/views/omnichannel/components/Tags.tsx
🔇 Additional comments (1)
apps/meteor/client/views/omnichannel/components/Tags.tsx (1)
12-20: LGTM! Error prop propagation is correctly implemented.The
errorprop is properly typed inTagsPropsand passed to bothCurrentChatTagsandTextInputcomponents, ensuring validation feedback can be displayed in both code paths.Also applies to: 76-85, 88-101
| @@ -1,5 +1,5 @@ | |||
| import { TextInput, Chip, Button, FieldLabel, FieldRow } from '@rocket.chat/fuselage'; | |||
| import { useEffectEvent } from '@rocket.chat/fuselage-hooks'; | |||
| import { useEffectEvent } from '@rocket.chat/fuselage-hooks';https://github.com/RocketChat/Rocket.Chat/pull/38723/conflict?name=apps%252Fmeteor%252Fclient%252Fviews%252Fomnichannel%252FadditionalForms%252FCurrentChatTags.tsx&ancestor_oid=e298f3cb3dbc49d0e2bdcd928013aee0d2e22440&base_oid=d7bc69afc620395b2bddd53c4da0accc5750597f&head_oid=0df8f5d14a2ec04cd58afc296f8c9cde98a6b6f4 | |||
There was a problem hiding this comment.
Critical: Malformed import statement with concatenated URL.
Line 2 contains a URL appended to the import statement, which is a syntax error that will break compilation. This appears to be a merge conflict artifact or copy-paste error.
🐛 Proposed fix
-import { useEffectEvent } from '@rocket.chat/fuselage-hooks';https://github.com/RocketChat/Rocket.Chat/pull/38723/conflict?name=apps%252Fmeteor%252Fclient%252Fviews%252Fomnichannel%252FadditionalForms%252FCurrentChatTags.tsx&ancestor_oid=e298f3cb3dbc49d0e2bdcd928013aee0d2e22440&base_oid=d7bc69afc620395b2bddd53c4da0accc5750597f&head_oid=0df8f5d14a2ec04cd58afc296f8c9cde98a6b6f4
+import { useEffectEvent } from '@rocket.chat/fuselage-hooks';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { useEffectEvent } from '@rocket.chat/fuselage-hooks';https://github.com/RocketChat/Rocket.Chat/pull/38723/conflict?name=apps%252Fmeteor%252Fclient%252Fviews%252Fomnichannel%252FadditionalForms%252FCurrentChatTags.tsx&ancestor_oid=e298f3cb3dbc49d0e2bdcd928013aee0d2e22440&base_oid=d7bc69afc620395b2bddd53c4da0accc5750597f&head_oid=0df8f5d14a2ec04cd58afc296f8c9cde98a6b6f4 | |
| import { useEffectEvent } from '@rocket.chat/fuselage-hooks'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/client/views/omnichannel/components/Tags.tsx` at line 2, The
import line in Tags.tsx is malformed: the import of useEffectEvent from
'@rocket.chat/fuselage-hooks' has a URL concatenated to it (merge artifact)
which causes a syntax error; open the Tags.tsx file, locate the import statement
referencing useEffectEvent and remove the trailing URL/merge-conflict text so
the line is a clean import (import { useEffectEvent } from
'@rocket.chat/fuselage-hooks';), then save and run the build/test to verify the
component (Tags component and any usages) compile correctly.
2 participants
Proposed changes
This PR fixes an issue where the Tags field in the Canned Responses form was disconnected from the form's validation system. Previously, if validation rules were applied to the Tags field, no visual feedback (red border or error message) was displayed to the user, leading to confusing "silent failures" on form submission.
Changes:
TagsController to passerrors.tagsand render a<FieldError>component, aligning with the pattern used forShortcutandMessage.errorprop and propagate it to its children (TextInputorCurrentChatTags).errorprop and pass it as a boolean toPaginatedMultiSelectFiltered, ensuring the UI correctly reflects the error state.Issue(s)
N/A (Addressed inline TODO:
// TODO: refactor Tags field validation)Verification Steps
Since the Tags field is currently optional, you must temporarily enforce a rule to verify the fix:
client/views/omnichannel/cannedResponses/components/CannedResponseForm.tsx, temporarily add a rule to the Tags Controller:Screenshots
Types of changes
Checklist
Summary by CodeRabbit