-
Notifications
You must be signed in to change notification settings - Fork 13k
test: replace data-qa attributes for Autocomplete locators
#38029
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…` `data-qa-type` attributes
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughThis PR systematically replaces test-centric Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38029 +/- ##
===========================================
+ Coverage 70.66% 70.67% +0.01%
===========================================
Files 3146 3146
Lines 108829 108829
Branches 19590 19603 +13
===========================================
+ Hits 76900 76912 +12
+ Misses 29923 29916 -7
+ Partials 2006 2001 -5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
a17a78c to
3c0d85a
Compare
3c0d85a to
dc7a259
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 issues found across 16 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/meteor/tests/e2e/federation/page-objects/fragments/home-flextab-members.ts">
<violation number="1" location="apps/meteor/tests/e2e/federation/page-objects/fragments/home-flextab-members.ts:48">
P1: `selectOption()` is only for native `<select>` elements, not custom autocomplete components with `role="listbox"`. This will fail at runtime. Use `getByRole('option')` and `click()` instead.</violation>
</file>
<file name="apps/meteor/tests/e2e/page-objects/fragments/modals/create-new-modal.ts">
<violation number="1" location="apps/meteor/tests/e2e/page-objects/fragments/modals/create-new-modal.ts:82">
P1: `dmListbox.selectOption()` will fail because `dmListbox` is a raw Playwright `Locator`, and Playwright's `selectOption()` only works with native `<select>` elements, not ARIA listboxes. Use `this.listbox.selectOption(username)` from the base class instead, which correctly clicks the option role.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
apps/meteor/tests/e2e/federation/page-objects/fragments/home-flextab-members.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/meteor/client/views/omnichannel/modals/ForwardChatModal.tsx (2)
99-110: Remove redundant aria-label when proper label association exists.The
htmlFor/idassociation on Lines 99 and 102 already provides the necessary accessible name. Addingaria-labelon Line 103 is redundant and can override the label in some screen readers, potentially causing confusion.♻️ Remove redundant aria-label
<FieldLabel htmlFor={departmentFieldId}>{t('Forward_to_department')}</FieldLabel> <FieldRow> <AutoCompleteDepartment id={departmentFieldId} - aria-label={t('Forward_to_department')} withTitle={false} maxWidth='100%' flexGrow={1}
115-129: Remove redundant aria-label when proper label association exists.Similar to the department field, the
htmlFor/idassociation on Lines 115 and 118 provides the accessible name. Thearia-labelon Line 119 is redundant.♻️ Remove redundant aria-label
<FieldLabel htmlFor={userFieldId}>{t('Forward_to_user')}</FieldLabel> <FieldRow> <AutoCompleteAgent id={userFieldId} - aria-label={t('Forward_to_user')} withTitle onlyAvailable
🤖 Fix all issues with AI agents
In
@apps/meteor/tests/e2e/federation/page-objects/fragments/home-flextab-members.ts:
- Around line 44-45: The repeated locator this.page.getByRole('textbox', { name:
'Choose users' }) should be stored in a local constant (e.g., chooseUsersInput)
and reused; replace the two calls with a single const assignment and then call
chooseUsersInput.click() and chooseUsersInput.fill(username) to improve
readability and maintainability.
- Line 48: The test calls selectOption on a listbox role
(this.page.getByRole('listbox').selectOption(username)), which is only valid for
<select> elements; replace it by locating the listbox option (e.g.,
this.page.getByRole('option', { name: username }) or find the option inside the
listbox) and perform a click on that option so the custom ARIA
listbox/autocomplete works; if the displayed option text differs from username,
target the option by its value attribute or a more specific locator inside the
listbox before clicking.
In
@apps/meteor/tests/e2e/page-objects/fragments/modals/omnichannel-transfer-chat-modal.ts:
- Around line 18-24: The selectors in the page object getters get
inputForwardDepartment and get inputForwardUser rely on localized label text;
add stable data-qa-id attributes to the input components in ForwardChatModal.tsx
(set data-qa-id='ForwardChatModalInputForwardDepartment' on the
AutoCompleteDepartment input and data-qa-id='ForwardChatModalInputForwardUser'
on the AutoCompleteAgent input) and then update the page object getters to
select by those data-qa-id attributes instead of getByLabel so tests don’t break
with language changes.
In @apps/meteor/tests/e2e/page-objects/omnichannel-departments.ts:
- Around line 128-130: The test's locator relies on translated labels and is
brittle; modify the AutoCompleteUnit component (look for PaginatedSelectFiltered
usage in AutoCompleteUnit.tsx) to add a stable attribute (e.g.,
data-testid="AutoCompleteUnit-Unit" or data-qa="AutoCompleteUnit-Unit") on the
PaginatedSelectFiltered element, then update the test's inputUnit locator to use
that stable selector (e.g.,
this.page.locator('[data-testid="AutoCompleteUnit-Unit"]') or
'[data-qa="AutoCompleteUnit-Unit"]'). Ensure the chosen attribute is unique and
matches the pattern used by other form inputs.
🧹 Nitpick comments (1)
apps/meteor/client/views/omnichannel/modals/ForwardChatModal.tsx (1)
132-142: Consider adding proper label association for the comment field.While this file adds
htmlFor/idassociations for the department and user fields, the comment textarea on Line 140 still uses adata-qa-idattribute without a properidand associated label. For consistency with the PR's accessibility improvements, consider:
- Adding an
idprop to the TextAreaInput- Adding
htmlForto the FieldLabel (though it already has text content)- Removing or replacing the
data-qa-idattribute
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (16)
apps/meteor/client/components/UserAutoCompleteMultiple/UserAutoCompleteMultiple.tsxapps/meteor/client/components/UserAutoCompleteMultiple/UserAutoCompleteMultipleOption.tsxapps/meteor/client/views/omnichannel/additionalForms/AutoCompleteUnit.tsxapps/meteor/client/views/omnichannel/agents/AgentsTable/AddAgent.tsxapps/meteor/client/views/omnichannel/components/AutoCompleteAgent.tsxapps/meteor/client/views/omnichannel/components/AutoCompleteDepartment.tsxapps/meteor/client/views/omnichannel/modals/ForwardChatModal.tsxapps/meteor/client/views/omnichannel/monitors/MonitorsTable.tsxapps/meteor/tests/e2e/federation/page-objects/channel.tsapps/meteor/tests/e2e/federation/page-objects/fragments/home-flextab-members.tsapps/meteor/tests/e2e/page-objects/fragments/modals/create-new-modal.tsapps/meteor/tests/e2e/page-objects/fragments/modals/omnichannel-transfer-chat-modal.tsapps/meteor/tests/e2e/page-objects/omnichannel-agents.tsapps/meteor/tests/e2e/page-objects/omnichannel-contact-center-chats.tsapps/meteor/tests/e2e/page-objects/omnichannel-departments.tspackages/ui-client/src/components/UserAutoComplete/UserAutoComplete.tsx
💤 Files with no reviewable changes (7)
- apps/meteor/tests/e2e/federation/page-objects/channel.ts
- apps/meteor/client/components/UserAutoCompleteMultiple/UserAutoCompleteMultipleOption.tsx
- apps/meteor/client/components/UserAutoCompleteMultiple/UserAutoCompleteMultiple.tsx
- apps/meteor/client/views/omnichannel/components/AutoCompleteAgent.tsx
- apps/meteor/client/views/omnichannel/components/AutoCompleteDepartment.tsx
- apps/meteor/tests/e2e/page-objects/omnichannel-contact-center-chats.ts
- packages/ui-client/src/components/UserAutoComplete/UserAutoComplete.tsx
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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/additionalForms/AutoCompleteUnit.tsxapps/meteor/client/views/omnichannel/agents/AgentsTable/AddAgent.tsxapps/meteor/tests/e2e/page-objects/fragments/modals/create-new-modal.tsapps/meteor/tests/e2e/page-objects/omnichannel-departments.tsapps/meteor/tests/e2e/federation/page-objects/fragments/home-flextab-members.tsapps/meteor/client/views/omnichannel/monitors/MonitorsTable.tsxapps/meteor/tests/e2e/page-objects/omnichannel-agents.tsapps/meteor/tests/e2e/page-objects/fragments/modals/omnichannel-transfer-chat-modal.tsapps/meteor/client/views/omnichannel/modals/ForwardChatModal.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/modals/create-new-modal.tsapps/meteor/tests/e2e/page-objects/omnichannel-departments.tsapps/meteor/tests/e2e/page-objects/omnichannel-agents.tsapps/meteor/tests/e2e/page-objects/fragments/modals/omnichannel-transfer-chat-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/modals/create-new-modal.tsapps/meteor/tests/e2e/page-objects/omnichannel-departments.tsapps/meteor/tests/e2e/federation/page-objects/fragments/home-flextab-members.tsapps/meteor/tests/e2e/page-objects/omnichannel-agents.tsapps/meteor/tests/e2e/page-objects/fragments/modals/omnichannel-transfer-chat-modal.ts
🧠 Learnings (13)
📓 Common learnings
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 36987
File: apps/meteor/tests/e2e/page-objects/fragments/room-toolbar.ts:10-20
Timestamp: 2025-09-23T19:22:59.217Z
Learning: In Playwright e2e tests, prefer stable selectors like data-qa-id attributes over localized text in getByRole() or getByText() calls to prevent test failures when UI language changes. Test translations separately by validating actual text content after ensuring UI interactions work with stable selectors.
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Avoid using `page.locator()` in Playwright tests - always prefer semantic locators such as `page.getByRole()`, `page.getByLabel()`, `page.getByText()`, or `page.getByTitle()`
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/client/views/omnichannel/additionalForms/AutoCompleteUnit.tsxapps/meteor/tests/e2e/page-objects/fragments/modals/create-new-modal.tsapps/meteor/tests/e2e/page-objects/omnichannel-departments.tsapps/meteor/tests/e2e/federation/page-objects/fragments/home-flextab-members.tsapps/meteor/tests/e2e/page-objects/omnichannel-agents.tsapps/meteor/tests/e2e/page-objects/fragments/modals/omnichannel-transfer-chat-modal.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Avoid using `page.locator()` in Playwright tests - always prefer semantic locators such as `page.getByRole()`, `page.getByLabel()`, `page.getByText()`, or `page.getByTitle()`
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/modals/create-new-modal.tsapps/meteor/tests/e2e/page-objects/omnichannel-departments.tsapps/meteor/tests/e2e/federation/page-objects/fragments/home-flextab-members.tsapps/meteor/tests/e2e/page-objects/omnichannel-agents.tsapps/meteor/tests/e2e/page-objects/fragments/modals/omnichannel-transfer-chat-modal.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Store commonly used locators in variables/constants for reuse
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/modals/create-new-modal.tsapps/meteor/tests/e2e/page-objects/omnichannel-departments.tsapps/meteor/tests/e2e/federation/page-objects/fragments/home-flextab-members.tsapps/meteor/tests/e2e/page-objects/omnichannel-agents.tsapps/meteor/tests/e2e/page-objects/fragments/modals/omnichannel-transfer-chat-modal.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/modals/create-new-modal.tsapps/meteor/tests/e2e/page-objects/omnichannel-departments.tsapps/meteor/tests/e2e/federation/page-objects/fragments/home-flextab-members.tsapps/meteor/tests/e2e/page-objects/omnichannel-agents.tsapps/meteor/tests/e2e/page-objects/fragments/modals/omnichannel-transfer-chat-modal.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/modals/create-new-modal.tsapps/meteor/tests/e2e/page-objects/omnichannel-departments.tsapps/meteor/tests/e2e/federation/page-objects/fragments/home-flextab-members.tsapps/meteor/tests/e2e/page-objects/omnichannel-agents.tsapps/meteor/tests/e2e/page-objects/fragments/modals/omnichannel-transfer-chat-modal.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/modals/create-new-modal.tsapps/meteor/tests/e2e/page-objects/omnichannel-departments.tsapps/meteor/tests/e2e/federation/page-objects/fragments/home-flextab-members.tsapps/meteor/tests/e2e/page-objects/fragments/modals/omnichannel-transfer-chat-modal.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Prefer web-first assertions (`toBeVisible`, `toHaveText`, etc.) in Playwright tests
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/modals/create-new-modal.tsapps/meteor/tests/e2e/page-objects/omnichannel-departments.tsapps/meteor/tests/e2e/federation/page-objects/fragments/home-flextab-members.tsapps/meteor/tests/e2e/page-objects/omnichannel-agents.tsapps/meteor/tests/e2e/page-objects/fragments/modals/omnichannel-transfer-chat-modal.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Implement proper wait strategies for dynamic content in Playwright tests
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/modals/create-new-modal.tsapps/meteor/tests/e2e/federation/page-objects/fragments/home-flextab-members.tsapps/meteor/tests/e2e/page-objects/omnichannel-agents.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/modals/create-new-modal.ts
📚 Learning: 2025-09-23T19:22:59.217Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 36987
File: apps/meteor/tests/e2e/page-objects/fragments/room-toolbar.ts:10-20
Timestamp: 2025-09-23T19:22:59.217Z
Learning: In Playwright e2e tests, prefer stable selectors like data-qa-id attributes over localized text in getByRole() or getByText() calls to prevent test failures when UI language changes. Test translations separately by validating actual text content after ensuring UI interactions work with stable selectors.
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/modals/create-new-modal.tsapps/meteor/tests/e2e/page-objects/omnichannel-departments.tsapps/meteor/tests/e2e/federation/page-objects/fragments/home-flextab-members.tsapps/meteor/tests/e2e/page-objects/omnichannel-agents.tsapps/meteor/tests/e2e/page-objects/fragments/modals/omnichannel-transfer-chat-modal.ts
📚 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/modals/create-new-modal.tsapps/meteor/tests/e2e/page-objects/omnichannel-departments.tsapps/meteor/tests/e2e/page-objects/omnichannel-agents.tsapps/meteor/tests/e2e/page-objects/fragments/modals/omnichannel-transfer-chat-modal.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
apps/meteor/tests/e2e/page-objects/omnichannel-departments.tsapps/meteor/tests/e2e/federation/page-objects/fragments/home-flextab-members.ts
🧬 Code graph analysis (4)
apps/meteor/client/views/omnichannel/agents/AgentsTable/AddAgent.tsx (1)
apps/meteor/tests/e2e/page-objects/auth.ts (1)
username(68-70)
apps/meteor/tests/e2e/page-objects/fragments/modals/create-new-modal.ts (1)
apps/meteor/tests/e2e/page-objects/auth.ts (1)
username(68-70)
apps/meteor/tests/e2e/federation/page-objects/fragments/home-flextab-members.ts (1)
apps/meteor/tests/e2e/page-objects/auth.ts (1)
username(68-70)
apps/meteor/client/views/omnichannel/monitors/MonitorsTable.tsx (2)
apps/meteor/tests/e2e/page-objects/auth.ts (1)
username(68-70)packages/models/src/models/Users.ts (1)
setUsername(2791-2795)
⏰ 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). (10)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (8)
apps/meteor/tests/e2e/federation/page-objects/fragments/home-flextab-members.ts (1)
44-48: Replace localized text selector with stable identifier and extract to constant.The selector uses the localized string 'Choose users' in the
nameparameter, which breaks when UI language changes. WhilegetByRole()is the correct semantic locator approach per Playwright rules, thenameparameter should reference a stable, language-independent identifier (likearia-labelordata-qa).Additionally, the same selector is called twice on consecutive lines (44-45) and should be extracted to a getter variable following the Page Object Model pattern.
Related issue: The same pattern appears on line 33 in
apps/meteor/tests/e2e/page-objects/fragments/home-flextab-members.ts, affecting multiple federation test files.apps/meteor/client/views/omnichannel/modals/ForwardChatModal.tsx (1)
46-47: LGTM! Proper use of React's useId hook.The use of
useId()to generate unique IDs for form fields is correct and follows React 18 best practices for accessible form associations.apps/meteor/client/views/omnichannel/additionalForms/AutoCompleteUnit.tsx (1)
43-59: LGTM! Appropriate interim accessibility solution.The
aria-labelwith the FIXME comment on Line 46 appropriately acknowledges this is a temporary solution untilPaginatedSelectFilteredsupports proper label association viahtmlFor. This is a reasonable interim approach.apps/meteor/tests/e2e/page-objects/omnichannel-agents.ts (1)
21-23: Verify test stability across language changes.The selector uses localized text "Username" which may break if the UI language changes or translations are updated. This is part of the broader shift toward semantic selectors for accessibility, but consider the maintenance implications.
Based on learnings, which recommend stable selectors over localized text to prevent test failures when UI language changes.
apps/meteor/tests/e2e/page-objects/fragments/modals/create-new-modal.ts (2)
55-63: Good improvement using fill() instead of typing.The change from XPath-based locators to role-based locators and using
fill()instead of typing is a good improvement for test reliability. However, the localized text "Add people" in the selector may break if UI language changes.Consider the trade-off between semantic clarity and test stability across languages. Based on learnings, which recommend stable selectors to prevent failures when UI language changes.
75-83: Consistent improvement with role-based locators.The changes align with the pattern established in CreateNewChannelModal and use more reliable
fill()for input. Same localization stability consideration applies.Based on learnings, verify these tests work consistently across supported UI languages.
apps/meteor/client/views/omnichannel/monitors/MonitorsTable.tsx (1)
31-31: LGTM! Proper label-input association improves accessibility.The implementation correctly uses
useId()to generate a unique identifier and associates the label with the input field viahtmlForandidprops. This follows WCAG best practices for accessible form controls.Also applies to: 40-40, 143-145
apps/meteor/client/views/omnichannel/agents/AgentsTable/AddAgent.tsx (1)
6-6: LGTM! Consistent accessibility pattern.The implementation follows the same proper label-input association pattern as MonitorsTable.tsx, using
useId()to link the label with the input field. This enhances form accessibility and maintains consistency across the codebase.Also applies to: 18-19, 41-43
Proposed changes (including videos or screenshots)
data-qaattributes fromAutoCompletecomponentsdata-qaIssue(s)
Steps to test or reproduce
Further comments
CORE-1595
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.