fix(a11y): ensure accessibility compliance in non-admin forms#40256
fix(a11y): ensure accessibility compliance in non-admin forms#40256dionisio-bot[bot] merged 20 commits intodevelopfrom
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughForm wiring and accessibility were refactored: many form components moved to ChangesForm & Accessibility Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (2)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
juliajforesti
force-pushed
the
fix/a11y-create-modals
branch
from
bdc1d5f to
8098689
Compare
|
juliajforesti
force-pushed
the
fix/a11y-create-modals
branch
from
8098689 to
5e4aeb6
Compare
juliajforesti
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #40256 +/- ##
===========================================
- Coverage 69.99% 69.94% -0.05%
===========================================
Files 3307 3309 +2
Lines 121026 120976 -50
Branches 21638 21664 +26
===========================================
- Hits 84712 84622 -90
- Misses 33006 33046 +40
Partials 3308 3308
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/meteor/client/components/message/helpers/getCheckboxLabel.tsx (1)
7-10: ⚖️ Poor tradeoffRaw Markdown in
message.msgwill be announced literally by screen readers.When
message.msgcontains Markdown tokens (e.g.,**bold**,_italic_,`code`,@mention), they are interpolated directly into thearia-labelviaSelect_message_from_user_with_preview. Screen readers would read the raw punctuation, undermining the accessibility goal.Consider stripping Markdown before using
message.msgas the preview. A simple approach if the codebase has no existing utility:♻️ Suggested approach
export const getCheckboxLabel = (message: IMessage, t: TFunction): string => { const username = message.u.name || message.u.username; if (message.msg) { + const plainText = message.msg.replace(/[*_~`>[\]]/g, '').trim(); return t('Select_message_from_user_with_preview', { username, - message: message.msg, + message: plainText, }); } return t('Select_message_from_user', { username }); };If a proper AST-based Markdown-to-plain-text utility already exists in the codebase, prefer that over the regex approach.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/components/message/helpers/getCheckboxLabel.tsx` around lines 7 - 10, The aria-label is using raw message.msg in getCheckboxLabel via t('Select_message_from_user_with_preview'), which exposes Markdown tokens to screen readers; update getCheckboxLabel to pass a plain-text preview by stripping/normalizing Markdown and mention tokens before interpolation—prefer an existing utility (e.g., stripMarkdown, markdownToText, or similar) if present, otherwise add a small fallback sanitizer that removes common Markdown (**, __, *, _, ``, ``` blocks, links, and `@mentions`) and collapses whitespace, then use that sanitizedPreview in the t(...) call instead of message.msg.
🤖 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/modals/TranscriptModal.tsx`:
- Line 112: The FieldError for the subject field is rendered unconditionally
(FieldError with errors.subject?.message) which creates an empty DOM node when
there is no error; update the TranscriptModal rendering so that the FieldError
for the subject is only rendered when errors.subject exists (mirror the
conditional pattern used for the email field), i.e. guard the FieldError with a
truthy check on errors.subject so it is omitted entirely when there is no
validation error.
In
`@apps/meteor/client/views/room/contextualBar/PruneMessages/PruneMessagesDateTimeRow.tsx`:
- Around line 21-22: The two InputBox components (the ones using
register(`${field}.date`) and register(`${field}.time`)) both use the same
aria-labelledby={fieldId}, causing identical labels for date and time; update
each input to provide distinct accessible names—either set an explicit
aria-label (e.g., append "date" and "time" to the field label, using your i18n
strings) or wrap both in a fieldset with a legend using fieldId so screen
readers get a proper group label; ensure the date InputBox and time InputBox
have unique accessible names while keeping register(`${field}.date`) and
register(`${field}.time`) intact.
---
Nitpick comments:
In `@apps/meteor/client/components/message/helpers/getCheckboxLabel.tsx`:
- Around line 7-10: The aria-label is using raw message.msg in getCheckboxLabel
via t('Select_message_from_user_with_preview'), which exposes Markdown tokens to
screen readers; update getCheckboxLabel to pass a plain-text preview by
stripping/normalizing Markdown and mention tokens before interpolation—prefer an
existing utility (e.g., stripMarkdown, markdownToText, or similar) if present,
otherwise add a small fallback sanitizer that removes common Markdown (**, __,
*, _, ``, ``` blocks, links, and `@mentions`) and collapses whitespace, then use
that sanitizedPreview in the t(...) call instead of message.msg.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b40d2c92-0509-4134-91a4-dddcad184fca
⛔ Files ignored due to path filters (1)
apps/meteor/client/components/CreateDiscussion/__snapshots__/CreateDiscussion.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (19)
apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsxapps/meteor/client/components/CreateDiscussion/DefaultParentRoomField.tsxapps/meteor/client/components/UserAutoCompleteMultiple/UserAutoCompleteMultiple.tsxapps/meteor/client/components/message/helpers/getCheckboxLabel.tsxapps/meteor/client/components/message/variants/RoomMessage.tsxapps/meteor/client/components/message/variants/SystemMessage.tsxapps/meteor/client/components/message/variants/ThreadMessagePreview.tsxapps/meteor/client/navbar/NavBarPagesGroup/actions/CreateChannelModal.tsxapps/meteor/client/navbar/NavBarPagesGroup/actions/CreateDirectMessage.tsxapps/meteor/client/navbar/NavBarPagesGroup/actions/CreateTeamModal.tsxapps/meteor/client/views/omnichannel/contactInfo/EditContactInfo.tsxapps/meteor/client/views/omnichannel/modals/TranscriptModal.spec.tsxapps/meteor/client/views/omnichannel/modals/TranscriptModal.tsxapps/meteor/client/views/room/contextualBar/ExportMessages/ExportMessages.tsxapps/meteor/client/views/room/contextualBar/PruneMessages/PruneMessagesDateTimeRow.tsxapps/meteor/tests/e2e/e2e-encryption/e2ee-pdf-export.spec.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/export-messages-flextab.tsapps/meteor/tests/e2e/page-objects/fragments/modals/create-new-modal.tspackages/i18n/src/locales/en.i18n.json
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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/tests/e2e/page-objects/fragments/flextabs/export-messages-flextab.tsapps/meteor/client/views/room/contextualBar/PruneMessages/PruneMessagesDateTimeRow.tsxapps/meteor/client/components/UserAutoCompleteMultiple/UserAutoCompleteMultiple.tsxapps/meteor/client/components/message/variants/SystemMessage.tsxapps/meteor/client/components/message/variants/RoomMessage.tsxapps/meteor/client/components/CreateDiscussion/DefaultParentRoomField.tsxapps/meteor/client/components/message/helpers/getCheckboxLabel.tsxapps/meteor/tests/e2e/e2e-encryption/e2ee-pdf-export.spec.tsapps/meteor/client/components/message/variants/ThreadMessagePreview.tsxapps/meteor/tests/e2e/page-objects/fragments/modals/create-new-modal.tsapps/meteor/client/views/omnichannel/contactInfo/EditContactInfo.tsxapps/meteor/client/navbar/NavBarPagesGroup/actions/CreateDirectMessage.tsxapps/meteor/client/views/omnichannel/modals/TranscriptModal.tsxapps/meteor/client/views/omnichannel/modals/TranscriptModal.spec.tsxapps/meteor/client/navbar/NavBarPagesGroup/actions/CreateTeamModal.tsxapps/meteor/client/navbar/NavBarPagesGroup/actions/CreateChannelModal.tsxapps/meteor/client/views/room/contextualBar/ExportMessages/ExportMessages.tsxapps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx
apps/meteor/tests/e2e/page-objects/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Utilize existing page objects pattern from
apps/meteor/tests/e2e/page-objects/
Files:
apps/meteor/tests/e2e/page-objects/fragments/flextabs/export-messages-flextab.tsapps/meteor/tests/e2e/page-objects/fragments/modals/create-new-modal.ts
apps/meteor/tests/e2e/**/*.{ts,spec.ts}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,spec.ts}: Store commonly used locators in variables/constants for reuse
Follow Page Object Model pattern consistently in Playwright tests
Files:
apps/meteor/tests/e2e/page-objects/fragments/flextabs/export-messages-flextab.tsapps/meteor/tests/e2e/e2e-encryption/e2ee-pdf-export.spec.tsapps/meteor/tests/e2e/page-objects/fragments/modals/create-new-modal.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
apps/meteor/tests/e2e/e2e-encryption/e2ee-pdf-export.spec.ts
apps/meteor/tests/e2e/**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.spec.ts: All test files must be created inapps/meteor/tests/e2e/directory
Avoid usingpage.locator()in Playwright tests - always prefer semantic locators such aspage.getByRole(),page.getByLabel(),page.getByText(), orpage.getByTitle()
Usetest.beforeAll()andtest.afterAll()for setup/teardown in Playwright tests
Usetest.step()for complex test scenarios to improve organization in Playwright tests
Group related tests in the same file
Utilize Playwright fixtures (test,page,expect) for consistency in test files
Prefer web-first assertions (toBeVisible,toHaveText, etc.) in Playwright tests
Useexpectmatchers for assertions (toEqual,toContain,toBeTruthy,toHaveLength, etc.) instead ofassertstatements in Playwright tests
Usepage.waitFor()with specific conditions instead of hardcoded timeouts in Playwright tests
Implement proper wait strategies for dynamic content in Playwright tests
Maintain test isolation between test cases in Playwright tests
Ensure clean state for each test execution in Playwright tests
Ensure tests run reliably in parallel without shared state conflicts
Files:
apps/meteor/tests/e2e/e2e-encryption/e2ee-pdf-export.spec.ts
🧠 Learnings (7)
📚 Learning: 2025-12-16T17:29:40.430Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37834
File: apps/meteor/tests/e2e/page-objects/fragments/admin-flextab-emoji.ts:12-22
Timestamp: 2025-12-16T17:29:40.430Z
Learning: In all page-object files under apps/meteor/tests/e2e/page-objects/, import expect from ../../utils/test (Playwright's async expect) instead of from Jest. Jest's expect is synchronous and incompatible with web-first assertions like toBeVisible, which can cause TypeScript errors.
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/flextabs/export-messages-flextab.tsapps/meteor/tests/e2e/page-objects/fragments/modals/create-new-modal.ts
📚 Learning: 2026-02-24T19:39:42.247Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/message.ts:7-7
Timestamp: 2026-02-24T19:39:42.247Z
Learning: In RocketChat e2e tests, avoid using data-qa attributes to locate elements. Prefer semantic locators such as getByRole, getByLabel, getByText, getByTitle and ARIA-based selectors. Apply this rule to all TypeScript files under apps/meteor/tests/e2e to improve test reliability, accessibility, and maintainability.
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/flextabs/export-messages-flextab.tsapps/meteor/tests/e2e/e2e-encryption/e2ee-pdf-export.spec.tsapps/meteor/tests/e2e/page-objects/fragments/modals/create-new-modal.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/flextabs/export-messages-flextab.tsapps/meteor/tests/e2e/e2e-encryption/e2ee-pdf-export.spec.tsapps/meteor/tests/e2e/page-objects/fragments/modals/create-new-modal.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/flextabs/export-messages-flextab.tsapps/meteor/tests/e2e/e2e-encryption/e2ee-pdf-export.spec.tsapps/meteor/tests/e2e/page-objects/fragments/modals/create-new-modal.ts
📚 Learning: 2026-03-27T14:52:56.865Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39892
File: apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:150-155
Timestamp: 2026-03-27T14:52:56.865Z
Learning: In Rocket.Chat, there are two different `ModalBackdrop` components with different prop APIs. During review, confirm the import source: (1) `rocket.chat/fuselage` `ModalBackdrop` uses `ModalBackdropProps` based on `BoxProps` (so it supports `onClick` and other Box/DOM props) and does not have an `onDismiss` prop; (2) `rocket.chat/ui-client` `ModalBackdrop` uses a narrower props interface like `{ children?: ReactNode; onDismiss?: () => void }` and handles Escape keypress and outside mouse-up, and it does not forward arbitrary DOM props such as `onClick`. Flag mismatched props (e.g., `onDismiss` passed to the fuselage component or `onClick` passed to the ui-client component) and ensure the usage matches the correct component being imported.
Applied to files:
apps/meteor/client/views/room/contextualBar/PruneMessages/PruneMessagesDateTimeRow.tsxapps/meteor/client/components/UserAutoCompleteMultiple/UserAutoCompleteMultiple.tsxapps/meteor/client/components/message/variants/SystemMessage.tsxapps/meteor/client/components/message/variants/RoomMessage.tsxapps/meteor/client/components/CreateDiscussion/DefaultParentRoomField.tsxapps/meteor/client/components/message/helpers/getCheckboxLabel.tsxapps/meteor/client/components/message/variants/ThreadMessagePreview.tsxapps/meteor/client/views/omnichannel/contactInfo/EditContactInfo.tsxapps/meteor/client/navbar/NavBarPagesGroup/actions/CreateDirectMessage.tsxapps/meteor/client/views/omnichannel/modals/TranscriptModal.tsxapps/meteor/client/views/omnichannel/modals/TranscriptModal.spec.tsxapps/meteor/client/navbar/NavBarPagesGroup/actions/CreateTeamModal.tsxapps/meteor/client/navbar/NavBarPagesGroup/actions/CreateChannelModal.tsxapps/meteor/client/views/room/contextualBar/ExportMessages/ExportMessages.tsxapps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx
📚 Learning: 2026-02-24T19:22:48.358Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/omnichannel/omnichannel-send-pdf-transcript.spec.ts:66-67
Timestamp: 2026-02-24T19:22:48.358Z
Learning: In Playwright end-to-end tests (e.g., under apps/meteor/tests/e2e/...), prefer locating elements by translated text (getByText) and ARIA roles (getByRole) over data-qa attributes. If translation values change, update the corresponding test locators accordingly. Never use data-qa locators. This guideline applies to all Playwright e2e test specs in the repository and helps keep tests robust to UI text changes and accessible semantics.
Applied to files:
apps/meteor/tests/e2e/e2e-encryption/e2ee-pdf-export.spec.ts
📚 Learning: 2026-03-06T18:10:15.268Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:15.268Z
Learning: In tests (especially those using testing-library/dom/jsdom) for Rocket.Chat components, the HTML <code> element has an implicit ARIA role of 'code'. Therefore, screen.getByRole('code') or screen.findByRole('code') will locate <code> elements even without a role attribute. Do not flag findByRole('code') as invalid in reviews; prefer using the implicit role instead of adding role="code" unless necessary for accessibility.
Applied to files:
apps/meteor/tests/e2e/e2e-encryption/e2ee-pdf-export.spec.tsapps/meteor/client/views/omnichannel/modals/TranscriptModal.spec.tsx
🔇 Additional comments (18)
apps/meteor/client/views/room/contextualBar/PruneMessages/PruneMessagesDateTimeRow.tsx (1)
1-28: Remaining changes LGTM.
useId()is the correct React 18+ idiom for stable unique IDs,id={fieldId}onFieldLabelis the right counterpart toaria-labelledbywhenhtmlForcannot target multiple inputs, and thetype ReactElementimport is idiomatic TypeScript.packages/i18n/src/locales/en.i18n.json (1)
7215-7217: Looks good — accessible label strings are clear and correctly parameterized.The new keys read naturally and the
{{username}}/{{message}}placeholders are appropriate for aria-label generation.apps/meteor/client/components/message/variants/SystemMessage.tsx (1)
33-33: LGTM — accessible label wired correctly.Import, label computation, and
aria-labelbinding are consistent with the helper contract. System messages withoutmsgcorrectly fall back to the user-only label.Also applies to: 67-68, 81-81
apps/meteor/client/components/message/variants/ThreadMessagePreview.tsx (1)
34-34: LGTM — pattern is consistent and types are compatible.
IThreadMessageis structurally compatible withIMessage(standard in this codebase), so the call togetCheckboxLabel(message, t)is type-safe. Thearia-labelis correctly scoped to the selecting branch.Also applies to: 73-74, 123-123
apps/meteor/client/components/message/variants/RoomMessage.tsx (1)
76-76: LGTM — implementation is correct and symmetric with the other message variants.
useTranslationis already the established hook used in this file's sibling components, and thearia-labelbinding mirrors the pattern inSystemMessageandThreadMessagePreview.Also applies to: 93-94, 129-129
apps/meteor/client/components/CreateDiscussion/DefaultParentRoomField.tsx (1)
1-2: LGTM — import switch is self-consistent.The
ComponentPropsWithoutRef<typeof TextInput>type constraint on line 12 references the newly importedTextInputfromfuselage-forms, so the prop surface and rendering are aligned.apps/meteor/client/views/omnichannel/modals/TranscriptModal.spec.tsx (1)
38-46: LGTM — semanticfindByRoleselectors align with the removedaria-labelattributes.Using visible text as the accessible name and querying by role + name is the correct approach after removing the explicit
aria-labelprops from the buttons.apps/meteor/client/views/omnichannel/contactInfo/EditContactInfo.tsx (1)
2-3: LGTM — import split is well-structured.Keeping
ButtonGroup,Button,IconButton, andDividerin@rocket.chat/fuselagewhile migrating the form-specific primitives to@rocket.chat/fuselage-formsis the correct separation for this PR's intent.apps/meteor/client/navbar/NavBarPagesGroup/actions/CreateDirectMessage.tsx (1)
69-95: LGTM — field layout and error wiring follow the PR-wide pattern correctly.
aria-required='true'on the autocomplete,errorprop for field-level error state, and a guardedFieldErrorfor visible error text are all consistent with how this PR refactors other forms.apps/meteor/client/views/room/contextualBar/ExportMessages/ExportMessages.tsx (2)
291-306: LGTM — cross-field error clearing and field destructuring are correct.The asymmetric destructuring (
{ value, onChange, onBlur, name, ...field }forUserAutoCompleteMultipleand{ onChange, onBlur, ...field }forTextInput) correctly interceptsonChangeto callclearErrorson the cross-validated partner field, while still forwardingrefand other Controller-managed props via...field.
248-271: IntentionalhtmlFor/idretention for date fields is correct.Since
dateFrom/dateTouseInputBoxfrom@rocket.chat/fuselage(notfuselage-forms), explicitid/htmlForwiring remains necessary. This is the right call and keeps the label-input association intact for those controls.apps/meteor/client/components/UserAutoCompleteMultiple/UserAutoCompleteMultiple.tsx (1)
1-1: VerifyMultiSelectFilteredis exported from@rocket.chat/fuselage-forms.The component is used with standard
MultiSelectFilteredprops (filter,setFilter,renderSelected,renderOptions,options,value,onChange,placeholder), but no other files in the codebase importMultiSelectFilteredfrom@rocket.chat/fuselage-forms. Similar components (PaginatedMultiSelectFiltered,MultiSelectFiltered) are imported from@rocket.chat/fuselageelsewhere. The import may be incorrect or require confirmation thatfuselage-formsexports this component with compatible props signature, particularly support forerrorandaria-requiredattributes.apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx (2)
148-156:FieldError id/aria-describedbylinkage — previously flagged.Whether
FieldErrorfrom@rocket.chat/fuselage-formsforwards theidprop to its root DOM element (required foraria-describedbyto resolve) remains unverified. If it doesn't, the${parentRoomId}-errorreference is a no-op and the association is silently dropped.#!/bin/bash # Check if FieldError in fuselage-forms source forwards the id prop fd 'FieldError' node_modules/@rocket.chat/fuselage-forms --type f 2>/dev/null | head -5 rg -n 'id' node_modules/@rocket.chat/fuselage-forms/dist/ 2>/dev/null | grep -i 'FieldError' | head -20
124-129: No issues found —DefaultParentRoomFieldcorrectly accepts the field props andrequiredattribute.The component's type definition (
Omit<ComponentPropsWithoutRef<typeof TextInput>, 'defaultValue' | 'disabled'>) includes all standard HTML input props exceptdefaultValueanddisabled. This coversrequired,onChange,onBlur,value, andnamefrom the RHF field spread. The props are properly typed and will not be silently dropped.apps/meteor/tests/e2e/page-objects/fragments/flextabs/export-messages-flextab.ts (1)
60-62: LGTM — more specific ARIA role locator.Switching to
getByRole('combobox', { name: 'To users' })is more precise thangetByLabeland aligns with the underlying component's rendered role after the fuselage-forms migration.apps/meteor/client/navbar/NavBarPagesGroup/actions/CreateTeamModal.tsx (1)
172-190: LGTM — Controller migration with inline validation looks correct.apps/meteor/client/navbar/NavBarPagesGroup/actions/CreateChannelModal.tsx (1)
204-267: LGTM — consistent Controller pattern across channel/team/discussion modals.apps/meteor/tests/e2e/page-objects/fragments/modals/create-new-modal.ts (1)
42-47: LGTM — trailing click is intentional for sequential multi-member usage.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx`:
- Around line 121-149: The FieldLabel/inputs in CreateDiscussion need proper
a11y wiring: generate a stable id using React's useId (e.g., fieldId) in the
CreateDiscussion component and pass id={fieldId} into DefaultParentRoomField and
RoomAutoComplete, set FieldLabel htmlFor={fieldId}, add
aria-describedby={`${fieldId}-error`} to inputs, give FieldError
id={`${fieldId}-error`} and role="alert", and pass the actual error message
string (errors.parentRoom?.message) into RoomAutoComplete's error prop (not
Boolean) so it matches UserAutoCompleteMultiple expectations; also ensure any
Controller render spreads/ref forwards include the ref so focus management works
(mirror pattern used in RegisterForm and UsersInRole components) and keep
onParentRoomChange unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2af7435f-835b-4135-b6a2-1df84ebc1a7d
📒 Files selected for processing (2)
apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsxapps/meteor/client/components/RoomAutoComplete/RoomAutoComplete.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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- 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/components/RoomAutoComplete/RoomAutoComplete.tsxapps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx
🧠 Learnings (1)
📚 Learning: 2026-03-27T14:52:56.865Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39892
File: apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:150-155
Timestamp: 2026-03-27T14:52:56.865Z
Learning: In Rocket.Chat, there are two different `ModalBackdrop` components with different prop APIs. During review, confirm the import source: (1) `rocket.chat/fuselage` `ModalBackdrop` uses `ModalBackdropProps` based on `BoxProps` (so it supports `onClick` and other Box/DOM props) and does not have an `onDismiss` prop; (2) `rocket.chat/ui-client` `ModalBackdrop` uses a narrower props interface like `{ children?: ReactNode; onDismiss?: () => void }` and handles Escape keypress and outside mouse-up, and it does not forward arbitrary DOM props such as `onClick`. Flag mismatched props (e.g., `onDismiss` passed to the fuselage component or `onClick` passed to the ui-client component) and ensure the usage matches the correct component being imported.
Applied to files:
apps/meteor/client/components/RoomAutoComplete/RoomAutoComplete.tsxapps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx
🔇 Additional comments (4)
apps/meteor/client/components/RoomAutoComplete/RoomAutoComplete.tsx (2)
38-41: LGTM! Clean forwardRef implementation.The
forwardRefpattern correctly forwards therefto the underlyingAutoCompletecomponent, enabling parent components (like theControllerinCreateDiscussion) to pass refs for focus management.Also applies to: 66-66, 104-104
19-23: LGTM! Well-structured type refinement.Using
Omit<AutoCompleteProps<LabelType>, 'filter'>properly extends the baseAutoCompletePropswhile excluding the internally-managedfilterprop.apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx (2)
177-184: 💤 Low valueThe review comment's concern about ref forwarding is incorrect.
UserAutoCompleteMultipleis a regular functional component (not wrapped withforwardRef), so spreading{...field}would not enable focus management viasetFocus('usernames'). The ref would be silently ignored regardless of how the field object is passed. The current explicit destructuring of{ name, onChange, value, onBlur }is the appropriate approach and doesn't prevent any valid use case.> Likely an incorrect or invalid review comment.
135-144: Theerrorprop is fully supported byRoomAutoCompleteandAutoCompletefrom@rocket.chat/fuselage. This is confirmed by existing usage inRoomsTab.tsx(apps/meteor/client/views/audit/components/tabs/RoomsTab.tsx:27) whereRoomAutoCompleteis already used witherror={!!ridFieldState.error}, and the validation state is properly communicated through the component.> Likely an incorrect or invalid review comment.
juliajforesti
force-pushed
the
fix/a11y-create-modals
branch
2 times, most recently
from
924e740 to
f1181b4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/meteor/client/navbar/NavBarPagesGroup/actions/CreateChannelModal.tsx`:
- Around line 237-248: Wrap the Controller/UserAutoCompleteMultiple in a
FieldRow so the members input is programmatically associated with its FieldLabel
and consistent with other forms: replace the bare Controller render of
UserAutoCompleteMultiple (the Controller with name='members' and render ->
UserAutoCompleteMultiple) by rendering the UserAutoCompleteMultiple inside
<FieldRow> and ensure FieldRow includes aria-describedby pointing to the
FieldError when errors.members exists (use a stable id derived from 'members' or
errors.members to connect FieldError to the FieldRow).
In
`@apps/meteor/client/views/room/contextualBar/ExportMessages/ExportMessages.tsx`:
- Around line 291-294: The UserAutoCompleteMultiple component must be converted
to use React.forwardRef so the ref provided by RHF's Controller is forwarded to
the underlying input component (MultiSelectFiltered); update the component
declaration (UserAutoCompleteMultiple) to accept (props, ref) and pass that ref
to MultiSelectFiltered (or to the actual DOM/input node) and ensure prop types
include Ref attributes; then update any internal handlers to use the forwarded
ref and keep existing props/behavior so Controller's render (the field ref) is
correctly applied and RHF focus management on validation errors works.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c696a85a-76d6-4458-9f20-b5decde8e8d0
⛔ Files ignored due to path filters (1)
apps/meteor/client/components/CreateDiscussion/__snapshots__/CreateDiscussion.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (20)
apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsxapps/meteor/client/components/CreateDiscussion/DefaultParentRoomField.tsxapps/meteor/client/components/RoomAutoComplete/RoomAutoComplete.tsxapps/meteor/client/components/UserAutoCompleteMultiple/UserAutoCompleteMultiple.tsxapps/meteor/client/components/message/helpers/getCheckboxLabel.tsxapps/meteor/client/components/message/variants/RoomMessage.tsxapps/meteor/client/components/message/variants/SystemMessage.tsxapps/meteor/client/components/message/variants/ThreadMessagePreview.tsxapps/meteor/client/navbar/NavBarPagesGroup/actions/CreateChannelModal.tsxapps/meteor/client/navbar/NavBarPagesGroup/actions/CreateDirectMessage.tsxapps/meteor/client/navbar/NavBarPagesGroup/actions/CreateTeamModal.tsxapps/meteor/client/views/omnichannel/contactInfo/EditContactInfo.tsxapps/meteor/client/views/omnichannel/modals/TranscriptModal.spec.tsxapps/meteor/client/views/omnichannel/modals/TranscriptModal.tsxapps/meteor/client/views/room/contextualBar/ExportMessages/ExportMessages.tsxapps/meteor/client/views/room/contextualBar/PruneMessages/PruneMessagesDateTimeRow.tsxapps/meteor/tests/e2e/e2e-encryption/e2ee-pdf-export.spec.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/export-messages-flextab.tsapps/meteor/tests/e2e/page-objects/fragments/modals/create-new-modal.tspackages/i18n/src/locales/en.i18n.json
✅ Files skipped from review due to trivial changes (6)
- apps/meteor/client/components/message/variants/SystemMessage.tsx
- apps/meteor/client/components/UserAutoCompleteMultiple/UserAutoCompleteMultiple.tsx
- apps/meteor/tests/e2e/page-objects/fragments/flextabs/export-messages-flextab.ts
- apps/meteor/client/components/CreateDiscussion/DefaultParentRoomField.tsx
- packages/i18n/src/locales/en.i18n.json
- apps/meteor/client/components/RoomAutoComplete/RoomAutoComplete.tsx
🚧 Files skipped from review as they are similar to previous changes (12)
- apps/meteor/tests/e2e/e2e-encryption/e2ee-pdf-export.spec.ts
- apps/meteor/client/views/room/contextualBar/PruneMessages/PruneMessagesDateTimeRow.tsx
- apps/meteor/client/views/omnichannel/modals/TranscriptModal.spec.tsx
- apps/meteor/client/components/message/variants/ThreadMessagePreview.tsx
- apps/meteor/client/components/message/helpers/getCheckboxLabel.tsx
- apps/meteor/tests/e2e/page-objects/fragments/modals/create-new-modal.ts
- apps/meteor/client/views/omnichannel/contactInfo/EditContactInfo.tsx
- apps/meteor/client/components/message/variants/RoomMessage.tsx
- apps/meteor/client/navbar/NavBarPagesGroup/actions/CreateTeamModal.tsx
- apps/meteor/client/navbar/NavBarPagesGroup/actions/CreateDirectMessage.tsx
- apps/meteor/client/views/omnichannel/modals/TranscriptModal.tsx
- apps/meteor/client/components/CreateDiscussion/CreateDiscussion.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: 📦 Build Packages
- 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/navbar/NavBarPagesGroup/actions/CreateChannelModal.tsxapps/meteor/client/views/room/contextualBar/ExportMessages/ExportMessages.tsx
🧠 Learnings (1)
📚 Learning: 2026-03-27T14:52:56.865Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39892
File: apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:150-155
Timestamp: 2026-03-27T14:52:56.865Z
Learning: In Rocket.Chat, there are two different `ModalBackdrop` components with different prop APIs. During review, confirm the import source: (1) `rocket.chat/fuselage` `ModalBackdrop` uses `ModalBackdropProps` based on `BoxProps` (so it supports `onClick` and other Box/DOM props) and does not have an `onDismiss` prop; (2) `rocket.chat/ui-client` `ModalBackdrop` uses a narrower props interface like `{ children?: ReactNode; onDismiss?: () => void }` and handles Escape keypress and outside mouse-up, and it does not forward arbitrary DOM props such as `onClick`. Flag mismatched props (e.g., `onDismiss` passed to the fuselage component or `onClick` passed to the ui-client component) and ensure the usage matches the correct component being imported.
Applied to files:
apps/meteor/client/navbar/NavBarPagesGroup/actions/CreateChannelModal.tsxapps/meteor/client/views/room/contextualBar/ExportMessages/ExportMessages.tsx
🔇 Additional comments (3)
apps/meteor/client/views/room/contextualBar/ExportMessages/ExportMessages.tsx (1)
201-243: Field/control migration to@rocket.chat/fuselage-formslooks good.The updated Controller wiring and direct
errorpropagation simplify the form structure while staying aligned with the accessibility cleanup goal.Also applies to: 275-360
apps/meteor/client/navbar/NavBarPagesGroup/actions/CreateChannelModal.tsx (2)
206-226: LGTM — name field Controller wiring is correct
error={errors.name?.message}applies the visual error state to the input;<FieldError>renders the message text below — the dual usage is intentional and correct.aria-required='true'is serialised identically toaria-required={true}by React, so both forms are fine.
275-328: LGTM — advanced-settings toggle accessibility refactorRemoving per-field
id/htmlFor/aria-*in favour offuselage-formscontext-based label association is the correct approach for this library. All fourToggleSwitchcontrollers properly forward thereffrom the field render prop, ensuring RHF focus management (e.g. on validation error) works correctly.
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
…components Co-authored-by: Copilot <copilot@github.com>
juliajforesti
force-pushed
the
fix/a11y-create-modals
branch
from
f1181b4 to
a040c93
Compare
…CreateChannelModal Co-authored-by: Copilot <copilot@github.com>
dougfabris
added
the
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
2 participants
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
WA-79
WA-74
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests