E2E: Channel menu helper and Mattermost UI specs - #3858
Conversation
|
@yasserfaraazkhan: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsI understand the commands that are listed here |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds a new ChangesChannel menu helpers and consumers
New standalone E2E test suites
Sequence Diagram(s)sequenceDiagram
participant Test
participant openTeamSidebarContextMenu
participant webContents
participant Chromium
Test->>openTeamSidebarContextMenu: open(win, app, webContentsId)
openTeamSidebarContextMenu->>webContents: sendInputEvent(right mouse down/up)
webContents->>Chromium: trigger context-menu event
Chromium-->>Test: waitForNativeContextMenu observed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
yasserfaraazkhan
changed the title
Resolve harness/hook conflicts by keeping merged #3855 changes. Co-authored-by: Cursor <cursoragent@cursor.com>
tray_menu.test.ts belongs in the tray/deeplink PR; split-e2e-prs.sh is a local helper script, not part of the migration stack. Co-authored-by: Cursor <cursoragent@cursor.com>
|
❌ E2E Test Setup Failed Failed to create E2E test instances: installation wait cancelled: context canceled |
![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 (6)
e2e/specs/mattermost/alt_enter.test.ts (3)
66-76: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winProgrammatic DOM click bypasses real interaction simulation for the Send button.
sendButton.click()invoked viaevaluate()calls the native DOMclick()method directly, skipping Playwright's actionability checks (visibility, enabled state, not obscured, in viewport). Since this test's purpose is to verify the Send button still posts the composed message, a real interaction (firstServer!.click(selector)) would better reflect actual user behavior and catch UI bugs (e.g., a disabled or hidden button) that a programmatic click would silently bypass.♻️ Suggested change
- const sendButtonClicked = await firstServer!.evaluate(() => { - const sendButton = document.querySelector( - '`#channelHeaderSubmitButton`, button[aria-label*="Send" i], [data-testid="SendMessageButton"]', - ) as HTMLButtonElement | null; - if (!sendButton) { - return false; - } - sendButton.click(); - return true; - }); - expect(sendButtonClicked, 'Send button must be present before posting').toBe(true); + const sendButtonSelector = '`#channelHeaderSubmitButton`, button[aria-label*="Send" i], [data-testid="SendMessageButton"]'; + await firstServer!.waitForSelector(sendButtonSelector, {timeout: 5_000}); + await firstServer!.click(sendButtonSelector);🤖 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 `@e2e/specs/mattermost/alt_enter.test.ts` around lines 66 - 76, The Send button interaction is using a programmatic DOM click inside the evaluate block, which bypasses Playwright’s real user action checks. Update the alt_enter test to use the existing firstServer! click flow against the same Send button selector instead of sendButton.click(), so the test exercises visibility, enabled state, and other actionability conditions while still asserting the button exists before posting.
36-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffRepeated
document.querySelector/evaluatepatterns instead of Playwright locators.Throughout the test, DOM queries are done via
evaluate()rather than Playwright'slocator()API (e.g.firstServer.locator('.post-message__text').count()), which provides built-in auto-waiting/retry and is the more idiomatic Playwright pattern. The current approach works because of the explicitexpect.pollcalls already wrapping the async checks, but consolidating onto locators would reduce boilerplate and improve consistency with the codebase's polling-first guideline.Also applies to: 53-65, 85-91
🤖 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 `@e2e/specs/mattermost/alt_enter.test.ts` around lines 36 - 47, The test is using repeated firstServer.evaluate(document.querySelector/document.querySelectorAll) checks instead of Playwright locators. Update the affected assertions and polling logic in alt_enter.test.ts to use firstServer.locator(...) and locator.count()/other locator-based waits where appropriate, especially around the loading check and post/message counts, so the test follows Playwright’s idiomatic auto-waiting pattern and matches the polling-first style used elsewhere.
19-23: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winExpensive fixtures are resolved before the env-var skip check runs.
Destructuring
serverMapin the test signature forces the full Electron launch + app-ready + server-discovery fixture chain to execute before the body checksMM_TEST_SERVER_URL. If the env var is unset, this wastes the cost of launching Electron just to skip immediately afterward. Consider a describe-level conditional skip (e.g.test.skip(() => !process.env.MM_TEST_SERVER_URL, 'MM_TEST_SERVER_URL required')) placed insidetest.describe, which is evaluated before fixtures are resolved.Also,
test.skip(true, ...)aborts the test immediately by throwing, so thereturnon line 22 is unreachable/dead code.♻️ Suggested change
test.describe('mattermost/alt_enter', () => { test.use({appConfig: demoMattermostConfig}); test.setTimeout(120_000); + test.skip(() => !process.env.MM_TEST_SERVER_URL, 'MM_TEST_SERVER_URL required'); test('MM-T2023 ALT+ENTER inserts a newline without sending the message', {tag: ['`@P2`', '`@all`']}, async ({serverMap}) => { - if (!process.env.MM_TEST_SERVER_URL) { - test.skip(true, 'MM_TEST_SERVER_URL required'); - return; - } - const firstServer = serverMap[demoMattermostConfig.servers[0].name]?.[0]?.win;Please confirm whether
test.skip()accepts a callback form scoped totest.describein the Playwright version pinned for this repo (1.61.0), to ensure this refactor is valid.🤖 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 `@e2e/specs/mattermost/alt_enter.test.ts` around lines 19 - 23, Move the MM_TEST_SERVER_URL guard out of the test body in alt_enter.test.ts so the expensive serverMap fixture chain is not resolved before skipping; place a describe-level conditional skip using test.describe with a predicate that checks process.env.MM_TEST_SERVER_URL before any fixtures are requested. Update the existing test.skip(true, 'MM_TEST_SERVER_URL required') usage so it no longer relies on an immediate-throw skip inside the test body, and remove the unreachable return. Verify the refactor against the pinned Playwright 1.61.0 API on the relevant test.describe/test.skip symbols before applying it.e2e/specs/mattermost/window_close.test.ts (1)
8-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared close/verify logic to reduce duplication.
Both tests repeat the same get-serverView → close → poll mainWindow → rebuild serverMap sequence; only the blur/focus step differs. Extracting a shared helper would reduce duplication and make future variants easier to add.
♻️ Suggested refactor
+async function closeServerViewAndVerifyRecovery( + electronApp: ElectronApplication, + mainWindow: Page, + serverMap: ServerMap, + serverName: string, +) { + const serverView = serverMap[serverName]?.[0]?.win; + expect(serverView).toBeDefined(); + + await serverView!.evaluate(() => { + window.close(); + }); + + await expect.poll( + () => mainWindow.evaluate(() => document.readyState === 'complete'), + {timeout: 10_000}, + ).toBe(true); + + const refreshedMap = await buildServerMap(electronApp); + expect(refreshedMap[serverName]?.length ?? 0).toBeGreaterThan(0); +} + test.describe('mattermost/window_close', () => { test( 'MM-67909 window.close() in a server view does not crash the app', {tag: ['`@P1`', '`@all`']}, async ({electronApp, mainWindow, serverMap}) => { - const serverName = demoConfig.servers[0].name; - const serverView = serverMap[serverName]?.[0]?.win; - expect(serverView).toBeDefined(); - - await serverView!.evaluate(() => { - window.close(); - }); - - expect(mainWindow).toBeDefined(); - await expect.poll( - () => mainWindow.evaluate(() => document.readyState === 'complete'), - {timeout: 10_000}, - ).toBe(true); - - const refreshedMap = await buildServerMap(electronApp); - expect(refreshedMap[serverName]?.length ?? 0).toBeGreaterThan(0); + const serverName = demoConfig.servers[0].name; + await closeServerViewAndVerifyRecovery(electronApp, mainWindow, serverMap, serverName); }, );🤖 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 `@e2e/specs/mattermost/window_close.test.ts` around lines 8 - 58, Both tests in mattermost/window_close repeat the same serverView close and verification flow, so extract that shared sequence into a helper to reduce duplication. Move the common get serverView from serverMap, call window.close(), wait for mainWindow to be ready, and rebuild/validate the server map into a reusable function, then keep only the blur/focus variation inside the second test. Use the existing test block and symbols like serverMap, mainWindow, buildServerMap, and electronApp to locate the shared logic.e2e/specs/mattermost/context_menu.test.ts (1)
23-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
MM_TEST_SERVER_URLskip guard across both tests.Both tests repeat the same inline skip check.
copy_link.test.tsin this same PR uses a single describe-leveltest.skip(!process.env.MM_TEST_SERVER_URL, 'MM_TEST_SERVER_URL required');. Consider applying the same pattern here for consistency and to avoid duplication.♻️ Suggested refactor
test.describe('mattermost/context_menu', () => { test.use({appConfig: demoMattermostConfig}); test.setTimeout(120_000); + test.skip(!process.env.MM_TEST_SERVER_URL, 'MM_TEST_SERVER_URL required'); test('MM-T1307 Right-click a channel name in LHS shows context menu', {tag: ['`@P2`', '`@all`']}, async ({electronApp, serverMap}) => { - if (!process.env.MM_TEST_SERVER_URL) { - test.skip(true, 'MM_TEST_SERVER_URL required'); - return; - } - const serverEntry = serverMap[demoMattermostConfig.servers[0].name]?.[0];Also applies to: 54-57
🤖 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 `@e2e/specs/mattermost/context_menu.test.ts` around lines 23 - 26, The two tests in context_menu.test.ts duplicate the MM_TEST_SERVER_URL availability check inline, so refactor them to use a single describe-level test.skip guard like copy_link.test.ts. Update the relevant describe block(s) to skip when process.env.MM_TEST_SERVER_URL is missing, and remove the repeated per-test conditional checks from the individual test cases to keep the suite consistent and avoid duplication.e2e/helpers/channelMenu.ts (1)
84-110: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCopy-link menu lookup doesn't verify element visibility, and duplicates polling logic already available via
expect.poll.
waitForCopyLinkInMenuonly checks DOM presence (win.$(selector)), not visibility, unlike the bounding-rect checks used elsewhere in this file (e.g.waitForBookmarkInBar,waitForWebappContextMenu). A stale/hidden menu node matching one of theCOPY_LINK_SELECTORScould causeclickCopyLinkInMenuto click an invisible element and fail or click the wrong target. The hand-rolledDate.now()/setTimeoutloop also duplicates theexpect.pollpattern already used at lines 239, 247, 277, 400 in this same file.♻️ Suggested refactor using `expect.poll` + visibility check
export async function waitForCopyLinkInMenu(win: ServerView): Promise<void> { - const deadline = Date.now() + 15_000; - while (Date.now() < deadline) { - for (const selector of COPY_LINK_SELECTORS) { - const candidate = await win.$(selector); - if (candidate) { - return; - } - } - await new Promise((resolve) => setTimeout(resolve, 200)); - } - throw new Error('"Copy Link" item not found in the channel menu'); + await expect.poll(async () => { + for (const selector of COPY_LINK_SELECTORS) { + const candidate = await win.$(selector); + if (candidate && await candidate.isVisible()) { + return true; + } + } + return false; + }, {timeout: 15_000, message: '"Copy Link" item must appear in the channel menu'}).toBe(true); }🤖 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 `@e2e/helpers/channelMenu.ts` around lines 84 - 110, The Copy Link menu helpers are only checking DOM presence and using a custom polling loop. Update waitForCopyLinkInMenu and clickCopyLinkInMenu to use the same expect.poll pattern already used elsewhere in channelMenu.ts, and verify the matched COPY_LINK_SELECTORS element is actually visible (similar to the bounding-rect checks in waitForBookmarkInBar and waitForWebappContextMenu) before returning or clicking. Keep the retry logic centralized in waitForCopyLinkInMenu so clickCopyLinkInMenu only waits and then clicks the confirmed visible selector.
🤖 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 `@e2e/helpers/channelMenu.ts`:
- Around line 318-373: The deleteAllBookmarksInBar retry loop can finish without
actually removing all bookmarks, because it exits silently after 10 attempts
even if hasBookmark stays true. Update deleteAllBookmarksInBar to keep the
current retry/recheck behavior, but after the loop completes with bookmarks
still present, throw a failure so callers know cleanup did not succeed. Keep the
transient-failure handling around the click/delete flow, but make sure
persistent failures in the bookmark-bar deletion path are surfaced instead of
being treated as success.
In `@e2e/specs/mattermost/copy_link.test.ts`:
- Around line 15-29: The Mattermost copy-link test still uses
serverEntry.webContentsId after only checking firstServer, so serverEntry
remains possibly undefined. Update the guard in the test setup around
serverMap/demoMattermostConfig usage to narrow serverEntry itself before calling
prepareMattermostServerView, either by explicitly checking serverEntry or using
a non-null assertion where appropriate. Keep the fix localized to the setup that
fetches serverEntry and firstServer in copy_link.test.ts.
---
Nitpick comments:
In `@e2e/helpers/channelMenu.ts`:
- Around line 84-110: The Copy Link menu helpers are only checking DOM presence
and using a custom polling loop. Update waitForCopyLinkInMenu and
clickCopyLinkInMenu to use the same expect.poll pattern already used elsewhere
in channelMenu.ts, and verify the matched COPY_LINK_SELECTORS element is
actually visible (similar to the bounding-rect checks in waitForBookmarkInBar
and waitForWebappContextMenu) before returning or clicking. Keep the retry logic
centralized in waitForCopyLinkInMenu so clickCopyLinkInMenu only waits and then
clicks the confirmed visible selector.
In `@e2e/specs/mattermost/alt_enter.test.ts`:
- Around line 66-76: The Send button interaction is using a programmatic DOM
click inside the evaluate block, which bypasses Playwright’s real user action
checks. Update the alt_enter test to use the existing firstServer! click flow
against the same Send button selector instead of sendButton.click(), so the test
exercises visibility, enabled state, and other actionability conditions while
still asserting the button exists before posting.
- Around line 36-47: The test is using repeated
firstServer.evaluate(document.querySelector/document.querySelectorAll) checks
instead of Playwright locators. Update the affected assertions and polling logic
in alt_enter.test.ts to use firstServer.locator(...) and locator.count()/other
locator-based waits where appropriate, especially around the loading check and
post/message counts, so the test follows Playwright’s idiomatic auto-waiting
pattern and matches the polling-first style used elsewhere.
- Around line 19-23: Move the MM_TEST_SERVER_URL guard out of the test body in
alt_enter.test.ts so the expensive serverMap fixture chain is not resolved
before skipping; place a describe-level conditional skip using test.describe
with a predicate that checks process.env.MM_TEST_SERVER_URL before any fixtures
are requested. Update the existing test.skip(true, 'MM_TEST_SERVER_URL
required') usage so it no longer relies on an immediate-throw skip inside the
test body, and remove the unreachable return. Verify the refactor against the
pinned Playwright 1.61.0 API on the relevant test.describe/test.skip symbols
before applying it.
In `@e2e/specs/mattermost/context_menu.test.ts`:
- Around line 23-26: The two tests in context_menu.test.ts duplicate the
MM_TEST_SERVER_URL availability check inline, so refactor them to use a single
describe-level test.skip guard like copy_link.test.ts. Update the relevant
describe block(s) to skip when process.env.MM_TEST_SERVER_URL is missing, and
remove the repeated per-test conditional checks from the individual test cases
to keep the suite consistent and avoid duplication.
In `@e2e/specs/mattermost/window_close.test.ts`:
- Around line 8-58: Both tests in mattermost/window_close repeat the same
serverView close and verification flow, so extract that shared sequence into a
helper to reduce duplication. Move the common get serverView from serverMap,
call window.close(), wait for mainWindow to be ready, and rebuild/validate the
server map into a reusable function, then keep only the blur/focus variation
inside the second test. Use the existing test block and symbols like serverMap,
mainWindow, buildServerMap, and electronApp to locate the shared logic.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fd6e4bb6-d824-44ad-9b2d-6c5bb4ef9fb8
📒 Files selected for processing (6)
e2e/helpers/channelMenu.tse2e/specs/mattermost/alt_enter.test.tse2e/specs/mattermost/context_menu.test.tse2e/specs/mattermost/copy_link.test.tse2e/specs/mattermost/external_links.test.tse2e/specs/mattermost/window_close.test.ts
Fail bookmark cleanup after retry exhaustion and guard serverEntry before prepareMattermostServerView in copy_link.test.ts. Co-authored-by: Cursor <cursoragent@cursor.com>
github-actions
Bot
added
E2E/Run
|
❌ E2E Test Setup Failed Failed to create E2E test instances: installation wait cancelled: context canceled |
|
@coderabbitai review |
✅ Action performedReview finished.
|
yasserfaraazkhan
added
E2E/Reset-Servers
yasserfaraazkhan
removed
the
E2E/Reset-Servers
fd75868
into
e2e/03-mattermost-shell
2 participants
Summary
Stacked PR 4/10 — channelMenu helper + context menu, copy link, alt+enter, etc.
Test plan
e2e/specs/mattermost/context_menu.test.tse2e/specs/mattermost/copy_link.test.tse2e/specs/mattermost/alt_enter.test.tsChange Impact: Medium 🟠
Regression Risk: The changes are mostly isolated to E2E test coverage and a new shared test helper, but the helper is imported across multiple Mattermost specs and uses UI/Electron interaction patterns that can be brittle across platforms. Existing application logic is not directly changed, so production regression risk is limited.
QA Recommendation: Run the affected Mattermost E2E specs across supported desktop platforms, with extra attention to context menu, copy link, and Alt+Enter flows. Manual QA can be limited if these automated tests are stable, but a focused spot check is recommended because the helper centralizes UI interaction behavior.