fix: Enable validation feedback for Canned Response Tags field#38723
fix: Enable validation feedback for Canned Response Tags field#38723sezallagwal wants to merge 1 commit 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 |
|
WalkthroughThree omnichannel components in the tags hierarchy are updated to support error prop forwarding. CurrentChatTags accepts an optional error string prop and passes it as a boolean to AutoCompleteTagsMultiple. Tags forwards this error prop. CannedResponseForm passes validation errors to Tags and displays error feedback. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ 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 |
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
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