test: Flaky should navigate on navbar toolbar pressing tab by dougfabris · Pull Request #41135 · RocketChat/Rocket.Chat

dougfabris · 2026-07-01T14:17:15Z

Proposed changes (including videos or screenshots)

Issue(s)

Steps to test or reproduce

Further comments

FLAKY-1798

Summary by CodeRabbit

Bug Fixes
- Improved sidebar page loading checks for more reliable navigation.
- Refined keyboard focus behavior coverage in the toolbar, including movement to Marketplace and search.

dionisio-bot · 2026-07-01T14:17:19Z

Looks like this PR is not ready to merge, because of the following issues:

This PR is missing the 'stat: QA assured' label

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

changeset-bot · 2026-07-01T14:17:22Z

⚠️ No Changeset found

Latest commit: c4374f1

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

coderabbitai · 2026-07-01T14:17:24Z

Walkthrough

Adds a btnMarketplace locator getter to the Navbar e2e page object and updates the sidebar e2e spec to use poHomeChannel.waitForHome() instead of page.waitForSelector('main'), plus refactors the toolbar tab navigation test into sequential test.step focus assertions.

Changes

Sidebar E2E Test Updates

Layer / File(s)	Summary
Navbar Marketplace locator `apps/meteor/tests/e2e/page-objects/fragments/navbar.ts`	Adds a `btnMarketplace` getter returning the “Marketplace” button locator within `pagesGroup`.
Sidebar spec home-page wait and tab focus test `apps/meteor/tests/e2e/sidebar.spec.ts`	Replaces `page.waitForSelector('main')` with `poHomeChannel.waitForHome()` in both the main and embedded layout `beforeEach` hooks, and reworks the toolbar tab navigation test into `test.step` blocks asserting sequential focus transitions across directory, marketplace, display, and create-new buttons, ending at the search input.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Test
  participant Page
  participant Navbar

  Test->>Page: waitForHome()
  Test->>Page: keyboard.press(Tab)
  Page->>Navbar: focus directory button
  Test->>Page: keyboard.press(Tab)
  Page->>Navbar: focus btnMarketplace
  Test->>Page: keyboard.press(Tab)
  Page->>Navbar: focus display button
  Test->>Page: keyboard.press(Tab)
  Page->>Navbar: focus create new button
  Test->>Page: keyboard.press(Tab)
  Page->>Navbar: focus search input

Suggested labels: type: chore

Suggested reviewers: juliajforesti, gabriellsh

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title matches the main change: a flaky navbar toolbar tab-navigation test update.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

codecov · 2026-07-01T14:28:43Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.12%. Comparing base (3001445) to head (c4374f1).
⚠️ Report is 4 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #41135      +/-   ##
===========================================
+ Coverage    69.10%   69.12%   +0.02%     
===========================================
  Files         3433     3433              
  Lines       132323   132340      +17     
  Branches     23068    23097      +29     
===========================================
+ Hits         91438    91482      +44     
+ Misses       37513    37489      -24     
+ Partials      3372     3369       -3

Flag	Coverage Δ
e2e	`59.34% <ø> (-0.01%)`	⬇️
e2e-api	`40.39% <ø> (-0.02%)`	⬇️
unit	`69.98% <ø> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

coderabbitai

🧹 Nitpick comments (1)

apps/meteor/tests/e2e/sidebar.spec.ts (1)
65-92: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Assert initial focus before the Tab sequence.

Every subsequent step asserts focus after each Tab press, but the initial btnHome.focus() call at Line 66 has no corresponding assertion. If .focus() silently fails, every following step could still pass/fail for the wrong reason, undermining the goal of this fix (per commit "add an expect for each tab step").
🔧 Proposed fix
 		test('should navigate on navbar toolbar pressing tab', async ({ page }) => {
 			await poHomeChannel.navbar.btnHome.focus();
+			await expect(poHomeChannel.navbar.btnHome).toBeFocused();
 
 			await test.step('move focus to directory button', async () => {
🤖 Prompt for 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.

In `@apps/meteor/tests/e2e/sidebar.spec.ts` around lines 65 - 92, The
tab-navigation test in sidebar.spec.ts should also verify the starting focus set
by poHomeChannel.navbar.btnHome.focus() before any Tab presses. Add an assertion
immediately after the initial focus call to confirm btnHome is actually focused,
so the rest of the sequence in test('should navigate on navbar toolbar pressing
tab') only runs from a known state. Use the existing btnHome and toBeFocused
symbols to keep the check aligned with the other focus assertions.

🤖 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.

Nitpick comments:
In `@apps/meteor/tests/e2e/sidebar.spec.ts`:
- Around line 65-92: The tab-navigation test in sidebar.spec.ts should also
verify the starting focus set by poHomeChannel.navbar.btnHome.focus() before any
Tab presses. Add an assertion immediately after the initial focus call to
confirm btnHome is actually focused, so the rest of the sequence in test('should
navigate on navbar toolbar pressing tab') only runs from a known state. Use the
existing btnHome and toBeFocused symbols to keep the check aligned with the
other focus assertions.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: cb872745-7ae4-4ef1-bdb6-9a6d64c152d5

📥 Commits

Reviewing files that changed from the base of the PR and between 70c0ff0 and c4374f1.

📒 Files selected for processing (2)

apps/meteor/tests/e2e/page-objects/fragments/navbar.ts
apps/meteor/tests/e2e/sidebar.spec.ts

📜 Review details

⏰ Context from checks skipped due to timeout. (2)

GitHub Check: cubic · AI code reviewer
GitHub Check: Hacktron Security Check

🧰 Additional context used

📓 Path-based instructions (5)

**/*.{ts,tsx,js}