test: Reorg flextabs locators#38533
Merged
dionisio-bot[bot] merged 24 commits intodevelopfrom Apr 7, 2026
Merged
Conversation
Contributor
|
Looks like this PR is ready to merge! 🎉 |
|
Contributor
WalkthroughReorganized e2e flextab page-objects into a new Changes
Sequence Diagram(s)sequenceDiagram
participant Test as E2E Test
participant Home as HomeChannel / HomeTeam
participant Toolbar as RoomToolbar
participant Menu as MenuMore
participant Dialog as FlexTab/Dialog
Test->>Home: open room/team UI
Home->>Toolbar: access toolbar instance
Test->>Toolbar: openMoreOptions()
Toolbar->>Menu: click btnMoreOptions
Menu->>Menu: render menu
Test->>Menu: getMenuItem("Export" / "Enable E2E" / "Notifications")
Menu->>Dialog: click menu item
Dialog->>Dialog: open FlexTab dialog or perform action
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Contributor
📦 Docker Image Size Report➡️ Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 23:27", "12/20 21:03", "12/22 18:54", "12/23 16:16", "12/24 19:38", "12/25 17:51", "12/26 13:18", "12/29 19:01", "12/30 20:52", "02/10 00:04 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.00]
line "omnichannel-transcript-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "queue-worker-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "rocketchat" [0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.00]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Tue, 10 Feb 2026 00:04:04 GMT |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38533 +/- ##
===========================================
- Coverage 70.57% 70.51% -0.07%
===========================================
Files 3270 3271 +1
Lines 116766 116804 +38
Branches 21091 21112 +21
===========================================
- Hits 82409 82361 -48
- Misses 32304 32381 +77
- Partials 2053 2062 +9
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
dougfabris
force-pushed
the
test/reorg-flextabs
branch
from
20966ee to
6eee4f8
Compare
dougfabris
added
the
stat: QA assured
dougfabris
force-pushed
the
test/reorg-flextabs
branch
from
e308937 to
6cf7948
Compare
… EditTeamFlexTab class
dougfabris
force-pushed
the
test/reorg-flextabs
branch
from
6cf7948 to
7cc04bb
Compare
dougfabris
removed
the
stat: QA assured
![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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
apps/meteor/client/views/teams/contextualBar/channels/AddExistingModal/AddExistingModal.tsx (1)
32-32: Consider removing or addressing the TODO comment.The TODO comment violates the coding guideline to avoid code comments in the implementation. Either address this by using
GenericModalnow, or track it in an issue and remove the comment.As per coding guidelines: "Avoid code comments in the implementation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/teams/contextualBar/channels/AddExistingModal/AddExistingModal.tsx` at line 32, Remove or resolve the TODO comment in AddExistingModal: either refactor the component to use GenericModal instead of Modal (update imports and replace Modal usage within the AddExistingModal component) or create a tracked issue and remove the inline TODO; if choosing the issue route, add the issue ID in a short comment and delete the TODO. Ensure the change references the AddExistingModal component and the Modal/GenericModal symbols so reviewers can verify the replacement or the created issue.apps/meteor/tests/e2e/page-objects/fragments/flextabs/flextab.ts (1)
6-8: Consider removing the JSDoc comment.While this documents a design decision, it could be seen as violating the guideline to avoid code comments. An alternative is to track this refactoring task in an issue.
As per coding guidelines: "Avoid code comments in the implementation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/e2e/page-objects/fragments/flextabs/flextab.ts` around lines 6 - 8, Remove the JSDoc comment above the public property "root" in flextab.ts (the comment that explains why root is public) and replace that in-repo explanation with a tracked issue instead; simply delete the comment block so the code no longer contains the implementation-level justification, and create/link an issue to track the refactor of making root protected in the future.apps/meteor/tests/e2e/page-objects/fragments/modals/confirm-remove-modal.ts (1)
5-8: Constructor can be simplified.The constructor only passes
rootto the parent class without additional logic, making it redundant in TypeScript.♻️ Proposed simplification
export class ConfirmRemoveModal extends Modal { - constructor(root: Locator) { - super(root); - } - get btnRemove() {Note: This requires the parent
Modalclass to have a public constructor that acceptsLocator.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/e2e/page-objects/fragments/modals/confirm-remove-modal.ts` around lines 5 - 8, The ConfirmRemoveModal constructor is redundant because it only forwards `root` to the parent; remove the constructor from the `ConfirmRemoveModal` class so it inherits `Modal`'s constructor directly. Ensure the parent `Modal` class has a public constructor signature that accepts a `Locator` (update `Modal` to export a public constructor if needed) so instances of `ConfirmRemoveModal` continue to be constructible with a `Locator`.apps/meteor/tests/e2e/page-objects/fragments/flextabs/admin-edit-room-flextab.ts (1)
10-47: Prefer semantic role/label locators in this flextab page object.Line 10-Line 47 currently rely on CSS/text-engine selectors (
locator('input[...]'),label >> text=...); migrating togetByRole/getByLabelwould make these locators more resilient to markup refactors.Based on learnings: "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."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/e2e/page-objects/fragments/flextabs/admin-edit-room-flextab.ts` around lines 10 - 47, The current getters (roomNameInput, privateLabel, privateInput, roomOwnerInput, archivedLabel, archivedInput, favoriteLabel, favoriteInput, defaultLabel, defaultInput) use CSS/text selectors; replace them with semantic Playwright locators like getByLabel/getByRole/getByText/getByTitle or ARIA-based selectors (e.g., use getByLabel for inputs by their visible label, getByRole('checkbox'/'textbox'/'switch') with name for inputs, and getByText for labels) to make selectors resilient—update each getter to use the corresponding semantic locator instead of locator('input[...]') or 'label >> text=...'; ensure you do not switch to data-qa attributes.apps/meteor/tests/e2e/page-objects/home-team.ts (1)
14-20: Consider caching tabs instances.The
tabsgetter creates newTeamInfoFlexTabandEditTeamFlexTabinstances on every access. While this follows the existing pattern in the codebase and works correctly, caching these instances (similar to other properties likeheaderToolbar) would be slightly more efficient.♻️ Optional: Cache tabs instances
export class HomeTeam extends HomeChannel { readonly headerToolbar: TeamToolbar; + private readonly _room: TeamInfoFlexTab; + private readonly _editRoom: EditTeamFlexTab; constructor(page: Page) { super(page); this.headerToolbar = new TeamToolbar(page); + this._room = new TeamInfoFlexTab(page); + this._editRoom = new EditTeamFlexTab(page); } override get tabs() { return { ...super.tabs, - room: new TeamInfoFlexTab(this.page), - editRoom: new EditTeamFlexTab(this.page), + room: this._room, + editRoom: this._editRoom, }; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/e2e/page-objects/home-team.ts` around lines 14 - 20, The tabs getter currently instantiates new TeamInfoFlexTab and EditTeamFlexTab objects on every access (override get tabs), which is inefficient; change it to cache those instances on the class (similar to headerToolbar) by creating private properties (e.g., _tabs or _teamInfoTab/_editTeamTab) and return the cached objects from the tabs getter, instantiating them only if the cache is empty; ensure you still spread super.tabs when building the returned object.
🤖 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/tests/e2e/avatar-settings.ts`:
- Line 94: The test is calling the async DOM method getAttribute('src') without
awaiting and asserting its return directly; replace that with a Playwright
web-first assertion by awaiting expect on the image locator
(poHomeChannel.userCard.imgUserCard) and using toHaveAttribute('src', avatarUrl)
so the assertion is retried and stable; update the assertion from
expect(poHomeChannel.userCard.imgUserCard.getAttribute('src')).toBe(avatarUrl)
to an awaited web-first form (await
expect(poHomeChannel.userCard.imgUserCard).toHaveAttribute('src', avatarUrl)).
In `@apps/meteor/tests/e2e/page-objects/fragments/flextabs/members-flextab.ts`:
- Around line 123-125: The showAllUsers() helper is not idempotent because it
always queries the trigger by its current label ('Online') and fails once 'All'
is selected; update showAllUsers() to short-circuit when 'All' is already
selected or to target the filter control independent of its displayed value:
first inspect the listbox/current selection (e.g., read the selected option text
or an aria-selected attribute on the listbox) and return immediately if it
equals 'All', otherwise open the filter control (locate the toggle by a stable
selector instead of name, or locate the listbox container and click its toggle)
and then call listbox.selectOption('All'); ensure you update references to
root.getByRole('button', { name: 'Online' }) and listbox.selectOption('All')
accordingly so the helper works whether the current value is 'Online' or 'All'.
In
`@apps/meteor/tests/e2e/page-objects/fragments/flextabs/notification-preferences-flextab.ts`:
- Around line 15-20: The XPath locators in the methods are bypassing this.root
because they start with '//' (absolute); change them to relative XPath by using
'.//' so they stay scoped to the notification-preferences dialog—update the
locator in the method that returns
this.root.locator(`//div[`@id`="${device}Alert"]`) and in async
selectDropdownById(text: string) which uses
this.root.locator(`//div[`@id`="${text}"]`) to use './/div[`@id`="..."]' instead.
In `@apps/meteor/tests/e2e/page-objects/fragments/flextabs/threads-flextab.ts`:
- Around line 6-8: The constructor for ThreadsFlexTab currently binds to a
generic dialog via super(page.getByRole('dialog')), which can attach to the
wrong dialog; update the locator to scope by name by calling
page.getByRole('dialog', { name: 'Threads' }) (i.e., change the super(...) call
in the ThreadsFlexTab constructor) so it follows the established pattern used by
FilesFlexTab, SearchMessagesFlexTab, and UserInfoFlexTab and prevents flakiness
when multiple dialogs are present.
- Around line 14-16: Update the lastThreadMessage getter to avoid data-qa-type
selectors: within the existing lastThreadMessage accessor (which uses
listThreadMessages), replace the chain that locates '[data-qa-type="message"]'
with listThreadMessages.getByRole('listitem').last() and then access the message
body using .locator('.rcx-message-body') so it follows the semantic locator
pattern used in home-content.ts.
In `@apps/meteor/tests/e2e/page-objects/fragments/modals/confirm-delete-modal.ts`:
- Around line 21-23: The dialog locator in the ConfirmDeleteRoomModal
constructor (super(page.getByRole('dialog', { name: 'Delete' }))) is performing
substring matching and can collide with other dialogs; update the locator to use
exact matching by adding exact: true to the options object so the constructor
targets only the dialog with the exact name "Delete".
In `@apps/meteor/tests/e2e/page-objects/fragments/modals/confirm-remove-modal.ts`:
- Line 1: Change the Locator import in this fragment to use the project's common
Playwright package: replace the current import of Locator from 'playwright-core'
with an import from '@playwright/test' so the file's top-level import (the
Locator type used in this module) matches other modal page-object files.
In `@apps/meteor/tests/e2e/page-objects/fragments/toolbar.ts`:
- Around line 127-138: The openTeamMembers method clicks menuItemTeamMembers
without opening the overflow menu first; update openTeamMembers in TeamToolbar
to open the menu (e.g., await this.menu.open()) before awaiting
this.menuItemTeamMembers.click() so the menu item is visible and clickable —
ensure you use the existing menu accessor and keep the method async/await flow
intact.
In `@apps/meteor/tests/e2e/prune-messages.spec.ts`:
- Around line 64-65: The test is asserting checkboxes via label element locators
(pruneMessages.labelFilesOnly and pruneMessages.labelDoNotPrunePinned), but
Playwright's .toBeChecked() only works on input elements; change those locators
to semantic label-to-input resolvers using getByLabel with the exact label text
(e.g., getByLabel('Only remove the attached files, keep messages') and
getByLabel('Do not prune pinned messages')) so the assertions target the actual
checkbox inputs and not the label elements, then update the assertions to use
those new locators.
---
Nitpick comments:
In
`@apps/meteor/client/views/teams/contextualBar/channels/AddExistingModal/AddExistingModal.tsx`:
- Line 32: Remove or resolve the TODO comment in AddExistingModal: either
refactor the component to use GenericModal instead of Modal (update imports and
replace Modal usage within the AddExistingModal component) or create a tracked
issue and remove the inline TODO; if choosing the issue route, add the issue ID
in a short comment and delete the TODO. Ensure the change references the
AddExistingModal component and the Modal/GenericModal symbols so reviewers can
verify the replacement or the created issue.
In
`@apps/meteor/tests/e2e/page-objects/fragments/flextabs/admin-edit-room-flextab.ts`:
- Around line 10-47: The current getters (roomNameInput, privateLabel,
privateInput, roomOwnerInput, archivedLabel, archivedInput, favoriteLabel,
favoriteInput, defaultLabel, defaultInput) use CSS/text selectors; replace them
with semantic Playwright locators like getByLabel/getByRole/getByText/getByTitle
or ARIA-based selectors (e.g., use getByLabel for inputs by their visible label,
getByRole('checkbox'/'textbox'/'switch') with name for inputs, and getByText for
labels) to make selectors resilient—update each getter to use the corresponding
semantic locator instead of locator('input[...]') or 'label >> text=...'; ensure
you do not switch to data-qa attributes.
In `@apps/meteor/tests/e2e/page-objects/fragments/flextabs/flextab.ts`:
- Around line 6-8: Remove the JSDoc comment above the public property "root" in
flextab.ts (the comment that explains why root is public) and replace that
in-repo explanation with a tracked issue instead; simply delete the comment
block so the code no longer contains the implementation-level justification, and
create/link an issue to track the refactor of making root protected in the
future.
In `@apps/meteor/tests/e2e/page-objects/fragments/modals/confirm-remove-modal.ts`:
- Around line 5-8: The ConfirmRemoveModal constructor is redundant because it
only forwards `root` to the parent; remove the constructor from the
`ConfirmRemoveModal` class so it inherits `Modal`'s constructor directly. Ensure
the parent `Modal` class has a public constructor signature that accepts a
`Locator` (update `Modal` to export a public constructor if needed) so instances
of `ConfirmRemoveModal` continue to be constructible with a `Locator`.
In `@apps/meteor/tests/e2e/page-objects/home-team.ts`:
- Around line 14-20: The tabs getter currently instantiates new TeamInfoFlexTab
and EditTeamFlexTab objects on every access (override get tabs), which is
inefficient; change it to cache those instances on the class (similar to
headerToolbar) by creating private properties (e.g., _tabs or
_teamInfoTab/_editTeamTab) and return the cached objects from the tabs getter,
instantiating them only if the cache is empty; ensure you still spread
super.tabs when building the returned object.
🪄 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: 3bd3b19b-d644-496b-a04b-d182066ee96f
⛔ Files ignored due to path filters (1)
apps/meteor/client/components/UserInfo/__snapshots__/UserInfo.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (64)
apps/meteor/client/components/UserInfo/UserInfo.tsxapps/meteor/client/views/teams/contextualBar/channels/AddExistingModal/AddExistingModal.tsxapps/meteor/tests/e2e/account-profile.spec.tsapps/meteor/tests/e2e/avatar-settings.tsapps/meteor/tests/e2e/channel-management.spec.tsapps/meteor/tests/e2e/e2e-encryption/e2ee-encrypted-channels.spec.tsapps/meteor/tests/e2e/e2e-encryption/e2ee-pdf-export.spec.tsapps/meteor/tests/e2e/export-messages.spec.tsapps/meteor/tests/e2e/feature-preview.spec.tsapps/meteor/tests/e2e/message-actions.spec.tsapps/meteor/tests/e2e/page-objects/admin-device-management.tsapps/meteor/tests/e2e/page-objects/admin-emojis.tsapps/meteor/tests/e2e/page-objects/admin-rooms.tsapps/meteor/tests/e2e/page-objects/encrypted-room.tsapps/meteor/tests/e2e/page-objects/fragments/edit-room-flextab.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/admin-edit-room-flextab.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/admin-flextab-emoji.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/channels-flextab.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/device-info-flextab.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/edit-contact-flextab.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/edit-room-flextab.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/edit-user-flextab.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/export-messages-flextab.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/files-flextab.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/flextab.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/index.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/members-flextab.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/notification-preferences-flextab.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/prune-messages-flextab.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/room-info-flextab.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/search-messages-flextab.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/threads-flextab.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/user-info-flextab.tsapps/meteor/tests/e2e/page-objects/fragments/home-content.tsapps/meteor/tests/e2e/page-objects/fragments/home-flextab-channels.tsapps/meteor/tests/e2e/page-objects/fragments/home-flextab-members.tsapps/meteor/tests/e2e/page-objects/fragments/home-flextab-pruneMessages.tsapps/meteor/tests/e2e/page-objects/fragments/home-flextab-room.tsapps/meteor/tests/e2e/page-objects/fragments/home-flextab.tsapps/meteor/tests/e2e/page-objects/fragments/index.tsapps/meteor/tests/e2e/page-objects/fragments/modals/confirm-delete-modal.tsapps/meteor/tests/e2e/page-objects/fragments/modals/confirm-remove-modal.tsapps/meteor/tests/e2e/page-objects/fragments/modals/index.tsapps/meteor/tests/e2e/page-objects/fragments/room-info-flextab.tsapps/meteor/tests/e2e/page-objects/fragments/toolbar.tsapps/meteor/tests/e2e/page-objects/fragments/user-card.tsapps/meteor/tests/e2e/page-objects/home-channel.tsapps/meteor/tests/e2e/page-objects/home-discussion.tsapps/meteor/tests/e2e/page-objects/home-omnichannel.tsapps/meteor/tests/e2e/page-objects/home-team.tsapps/meteor/tests/e2e/page-objects/omnichannel/omnichannel-agents.tsapps/meteor/tests/e2e/page-objects/omnichannel/omnichannel-contact-center/omnichannel-contact-center-chats.tsapps/meteor/tests/e2e/page-objects/omnichannel/omnichannel-contact-center/omnichannel-contact-center-contacts.tsapps/meteor/tests/e2e/page-objects/omnichannel/omnichannel-custom-fields.tsapps/meteor/tests/e2e/page-objects/omnichannel/omnichannel-info.tsapps/meteor/tests/e2e/page-objects/omnichannel/omnichannel-priorities.tsapps/meteor/tests/e2e/page-objects/omnichannel/omnichannel-sla-policies.tsapps/meteor/tests/e2e/page-objects/omnichannel/omnichannel-tags.tsapps/meteor/tests/e2e/page-objects/omnichannel/omnichannel-triggers.tsapps/meteor/tests/e2e/page-objects/omnichannel/omnichannel-units.tsapps/meteor/tests/e2e/prune-messages.spec.tsapps/meteor/tests/e2e/retention-policy.spec.tsapps/meteor/tests/e2e/team-management.spec.tspackages/ui-client/src/components/InfoPanel/InfoPanelField.tsx
💤 Files with no reviewable changes (8)
- apps/meteor/tests/e2e/page-objects/fragments/home-flextab.ts
- apps/meteor/tests/e2e/page-objects/fragments/home-content.ts
- apps/meteor/tests/e2e/page-objects/fragments/home-flextab-pruneMessages.ts
- apps/meteor/tests/e2e/page-objects/fragments/home-flextab-channels.ts
- apps/meteor/tests/e2e/page-objects/fragments/home-flextab-room.ts
- apps/meteor/tests/e2e/page-objects/fragments/edit-room-flextab.ts
- apps/meteor/tests/e2e/page-objects/fragments/room-info-flextab.ts
- apps/meteor/tests/e2e/page-objects/fragments/home-flextab-members.ts
apps/meteor/tests/e2e/page-objects/fragments/flextabs/members-flextab.ts
Show resolved
Hide resolved
apps/meteor/tests/e2e/page-objects/fragments/flextabs/notification-preferences-flextab.ts
Show resolved
Hide resolved
apps/meteor/tests/e2e/page-objects/fragments/flextabs/threads-flextab.ts
Show resolved
Hide resolved
apps/meteor/tests/e2e/page-objects/fragments/flextabs/threads-flextab.ts
Show resolved
Hide resolved
apps/meteor/tests/e2e/page-objects/fragments/modals/confirm-delete-modal.ts
Show resolved
Hide resolved
apps/meteor/tests/e2e/page-objects/fragments/modals/confirm-remove-modal.ts
Outdated
Show resolved
Hide resolved
aleksandernsilva
requested changes
Apr 6, 2026
Contributor
aleksandernsilva
left a comment
aleksandernsilva
left a comment
There was a problem hiding this comment.
Just a few nitpicks and questions.
apps/meteor/tests/e2e/page-objects/fragments/flextabs/user-info-flextab.ts
Outdated
Show resolved
Hide resolved
apps/meteor/tests/e2e/page-objects/fragments/flextabs/threads-flextab.ts
Show resolved
Hide resolved
apps/meteor/tests/e2e/page-objects/fragments/flextabs/channels-flextab.ts
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/tests/e2e/page-objects/fragments/flextabs/room-info-flextab.ts (1)
109-115: PrefergetByTextoverlocator()for text-based selectors.These methods use
locator()with CSS/text selectors, which is inconsistent with the semantic locator pattern used elsewhere in this file and the codebase.♻️ Proposed refactor to use semantic locators
getInfo(value: string): Locator { - return this.root.locator(`span >> text="${value}"`); + return this.root.getByText(value, { exact: true }); } getLabel(label: string): Locator { - return this.root.locator(`div >> text="${label}"`); + return this.root.getByText(label, { exact: true }); }Based on learnings: "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."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/e2e/page-objects/fragments/flextabs/room-info-flextab.ts` around lines 109 - 115, Replace the text-based locator calls in getInfo and getLabel with semantic getByText locators: locate the two methods getInfo(value: string) and getLabel(label: string) and change their return from this.root.locator(`...text=...`) to this.root.getByText(...) (using the value/label parameter) so the file uses semantic Playwright locators instead of CSS/text locator strings; preserve exact-match behavior if intended by passing the original string directly to getByText.
🤖 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/tests/e2e/page-objects/fragments/flextabs/room-info-flextab.ts`:
- Around line 109-115: Replace the text-based locator calls in getInfo and
getLabel with semantic getByText locators: locate the two methods getInfo(value:
string) and getLabel(label: string) and change their return from
this.root.locator(`...text=...`) to this.root.getByText(...) (using the
value/label parameter) so the file uses semantic Playwright locators instead of
CSS/text locator strings; preserve exact-match behavior if intended by passing
the original string directly to getByText.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8663aefd-15de-47c4-93fe-1971f7b61d54
📒 Files selected for processing (6)
apps/meteor/tests/e2e/account-profile.spec.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/room-info-flextab.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/user-info-flextab.tsapps/meteor/tests/e2e/page-objects/home-channel.tsapps/meteor/tests/e2e/page-objects/home-discussion.tsapps/meteor/tests/e2e/page-objects/home-omnichannel.ts
✅ Files skipped from review due to trivial changes (1)
- apps/meteor/tests/e2e/page-objects/home-omnichannel.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/meteor/tests/e2e/account-profile.spec.ts
- apps/meteor/tests/e2e/page-objects/fragments/flextabs/user-info-flextab.ts
- apps/meteor/tests/e2e/page-objects/home-discussion.ts
📜 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). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 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/tests/e2e/page-objects/home-channel.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/room-info-flextab.ts
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/home-channel.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/room-info-flextab.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/home-channel.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/room-info-flextab.ts
🧠 Learnings (17)
📓 Common learnings
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:55.089Z
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').
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:52.984Z
Learning: In RocketChat/Rocket.Chat Playwright e2e tests, prefer using translated text and ARIA roles (getByRole, getByText, etc.) over data-qa locators. If translation values change, update the corresponding test locators accordingly. Never prefer data-qa locators.
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
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/`
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
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
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
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 : Group related tests in the same file
📚 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/tests/e2e/page-objects/home-channel.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/room-info-flextab.ts
📚 Learning: 2026-02-24T19:36:55.089Z
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:55.089Z
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/tests/e2e/page-objects/home-channel.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/room-info-flextab.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/home-channel.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/room-info-flextab.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/home-channel.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/room-info-flextab.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 : Group related tests in the same file
Applied to files:
apps/meteor/tests/e2e/page-objects/home-channel.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/room-info-flextab.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/home-channel.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/room-info-flextab.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/home-channel.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/room-info-flextab.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/home-channel.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/room-info-flextab.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/home-channel.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/room-info-flextab.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/home-channel.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/room-info-flextab.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/home-channel.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/room-info-flextab.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/home-channel.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/room-info-flextab.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/home-channel.tsapps/meteor/tests/e2e/page-objects/fragments/flextabs/room-info-flextab.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 : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/flextabs/room-info-flextab.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/flextabs/room-info-flextab.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 `page.waitFor()` with specific conditions instead of hardcoded timeouts in Playwright tests
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/flextabs/room-info-flextab.ts
🔇 Additional comments (7)
apps/meteor/tests/e2e/page-objects/fragments/flextabs/room-info-flextab.ts (3)
8-63: Well-structured modal and flextab page objects.The
ConfirmLeaveRoomModalandRoomInfoFlexTabclasses follow the Page Object Model pattern correctly. Good use of semantic locators (getByRole,getByLabel) and proper composition withMenuMoreand modal fragments.
65-102: Good extension pattern for team-specific flows.
ConfirmConvertIntoChannelModalandTeamInfoFlexTabproperly extend base classes and add team-specific functionality (deleteTeam,convertIntoChannel). The dialog scoping with{ name: 'Team info' }is appropriate.
117-123: LGTM!
getInfoByLabelandgetTagInfoByLabelcorrectly use semantic locators (getByLabel,getByRole,getByText).apps/meteor/tests/e2e/page-objects/home-channel.ts (4)
3-24: Clean import organization for the new fragment-based architecture.The imports are well-organized, consolidating flextab fragments from the central
./fragmentsbarrel export while keeping specialized fragments (RoomToolbar,UserCard,VoiceCalls) as separate imports.
38-52: Good typed structure for flextab composition.The
_tabsobject is properly typed with specific flextab classes, providing type safety and IDE autocompletion for test authors.
70-83: Proper use of semantic locators for dialog-scoped flextabs.The
roomandeditRoomtabs are correctly scoped to their respective dialogs usinggetByRole('dialog', { name: '...' }), which aligns with the PR objective of using more accurate locators to reduce flakiness.
91-93: Getter pattern acknowledged.The
private _tabs+ getter pattern was discussed in a previous review thread, and the author confirmed it's intentional.
dougfabris
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
aleksandernsilva
approved these changes
Apr 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
CORE-1989
Summary by CodeRabbit
Accessibility
Refactor
Style