test(e2e): Notifications, focus, Calls, menu bar, and permissions - #3862
Conversation
Part 1 of splitting #3847 — core Playwright config, fixtures, electronApp teardown, and GitHub Actions E2E workflow updates. No new specs yet.
Exposes __e2eTestRefs, message-box stub, tray/deep-link hooks (NODE_ENV=test only) plus directLaunch, testRefs, and shared helper updates.
|
@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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds new Playwright/Electron e2e tests covering Calls widget controls, app-switch focus, menu bar actions (clear data, diagnostics, devtools, help menu), and notification triggers (delivery, dock bounce, flash taskbar, click navigation). It introduces shared helpers (settings window, downloads dropdown, method spies), removes numerous platform-specific test skips, refactors several specs to use shared launch/close helpers, adds test-only main-process hooks ( ChangesDesktop E2E coverage
Estimated code review effort: 4 (Complex) | ~60 minutes Flaky Test Analysis
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
yasserfaraazkhan
changed the title
|
❌ 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
|
❌ E2E Test Setup Failed Failed to create E2E test instances: installation wait cancelled: context canceled |
yasserfaraazkhan
added
E2E/Reset-Servers
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (8)
e2e/specs/calls/calls_functionality.test.ts (2)
63-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate widget-wait polling loop.
The same "poll
findCallsWidgetWindowevery 500ms until deadline" loop appears twice. Extract into a shared helper (e.g.waitForCallsWidgetWindow(electronApp, timeoutMs)) reused by both tests and the slash-command test's poll.♻️ Proposed helper extraction
+async function waitForCallsWidgetWindow(electronApp: ElectronApplication, timeoutMs = 20_000): Promise<Page | null> { + const deadline = Date.now() + timeoutMs; + let widgetWindow: Page | null = null; + while (!widgetWindow && Date.now() < deadline) { + widgetWindow = await findCallsWidgetWindow(electronApp); + if (!widgetWindow) { + await new Promise((resolve) => setTimeout(resolve, 500)); + } + } + return widgetWindow; +}Also applies to: 189-196
🤖 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/calls/calls_functionality.test.ts` around lines 63 - 70, The polling logic for findCallsWidgetWindow is duplicated in multiple places, so extract it into a shared helper such as waitForCallsWidgetWindow(electronApp, timeoutMs) and reuse it from both tests and the slash-command polling path. Move the repeated loop that waits up to widgetDeadline into this helper, keep the timeout and retry interval configurable, and update the existing call sites in calls_functionality.test.ts to use the new helper.
135-177: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAwkward type narrowing forces redundant casts.
outcomeis mutated inside theexpect.pollclosure, so TypeScript can't narrow it after theawait, forcing(outcome as Outcome)!.kindand a second cast to{kind:'widget'; window: Page}on Line 172. A type guard function or restructuring to return the outcome directly from the poll callback would avoid the double cast.🤖 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/calls/calls_functionality.test.ts` around lines 135 - 177, The polling logic in calls_functionality.test.ts uses a mutable Outcome variable in the expect.poll closure, which prevents TypeScript from narrowing it cleanly and forces redundant casts later. Refactor the polling flow around the relevant expect.poll block and outcome handling so the callback returns a typed result directly or add a dedicated type guard for Outcome; then use the narrowed result when checking for the widget case in the post-poll branch, avoiding both the non-null assertion and the extra {kind: 'widget'; window: Page} cast.e2e/specs/menu_bar/devtools_current_server.test.ts (1)
28-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
returnaftertest.skip(true, ...).Per Playwright semantics,
test.skip(condition, description)called inside the test body aborts execution immediately when the condition is true, so the subsequentreturn;is unreachable dead code. Harmless but unnecessary.♻️ Optional cleanup
if (!process.env.MM_TEST_SERVER_URL) { test.skip(true, 'MM_TEST_SERVER_URL required'); - return; }🤖 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/menu_bar/devtools_current_server.test.ts` around lines 28 - 31, Remove the redundant unreachable return after the test.skip(true, 'MM_TEST_SERVER_URL required') call in devtools_current_server.test.ts; when locating the code in the test body around the MM_TEST_SERVER_URL check, keep the test.skip guard and delete the following return since Playwright already aborts execution there.e2e/specs/notification_trigger/flash_taskbar.test.ts (1)
65-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale
__e2eOriginalFlashFramemarker left onmainWinafter restore.Unlike
dock_bounce.test.ts'srestoreDockBounce, which explicitlydeletes__e2eOriginalBounceafter restoring, this cleanup restoresmainWin.flashFramebut never removes the__e2eOriginalFlashFramemarker property from the window object. Harmless today, but inconsistent with the cleanup hygiene used elsewhere in this same PR and could cause confusion if a later test checks for that marker to detect "spy installed" state.♻️ Proposed fix
if (mainWin && (mainWin as any).__e2eOriginalFlashFrame) { mainWin.flashFrame = (mainWin as any).__e2eOriginalFlashFrame; + delete (mainWin as any).__e2eOriginalFlashFrame; } delete (global as any).__e2eFlashFrameCalls;🤖 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/notification_trigger/flash_taskbar.test.ts` around lines 65 - 73, The cleanup in the flash taskbar test restores mainWin.flashFrame but leaves the __e2eOriginalFlashFrame marker behind on the MainWindow object. Update the restore logic in electronApp.evaluate to also remove that marker after restoring the original flashFrame, matching the cleanup pattern used by restoreDockBounce in dock_bounce.test.ts and keeping the __e2eTestRefs/MainWindow state consistent.e2e/specs/notification_trigger/desktop_notification_delivery.test.ts (3)
4-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSibling import placed before parent imports.
./helpers(line 4) is a sibling-relative import placed before the parent-relative imports (../../fixtures/index,../../helpers/*) on lines 6-9, and the two groups are separated by a blank line implying distinct groups. Per the import-order convention, parent-relative imports should precede sibling imports.♻️ Proposed fix
-import {triggerTestNotification, verifyNotificationReceivedInDM} from './helpers'; - import {test, expect} from '../../fixtures/index'; import {demoMattermostConfig} from '../../helpers/config'; import {acquireExclusiveLock} from '../../helpers/exclusiveLock'; import {loginToMattermost} from '../../helpers/login'; + +import {triggerTestNotification, verifyNotificationReceivedInDM} from './helpers';As per coding guidelines, "Follow import order enforced by ESLint: builtins → external →
@mattermost/*→ internal aliases (app,common,main,renderer) →types→ siblings/parent/index, with groups separated by blank lines and alphabetized within groups".🤖 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/notification_trigger/desktop_notification_delivery.test.ts` around lines 4 - 9, Reorder the imports in desktop_notification_delivery.test.ts so the parent-relative imports from ../../fixtures/index and ../../helpers/* come before the sibling import from ./helpers, keeping the blank-line group separation and preserving alphabetical order within each group. Use the existing import block in the test file as the target, especially triggerTestNotification, verifyNotificationReceivedInDM, test, expect, demoMattermostConfig, acquireExclusiveLock, and loginToMattermost.Source: Coding guidelines
50-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant condition; simplify to avoid confusion.
unityRunning(lines 56-58) already resolves totruefor every non-Linux platform, so the disjunctprocess.platform !== 'linux'in the gate at line 65 is always true wheneverunityRunningisn't already true from that same platform check — i.e.,(unityRunning || process.platform !== 'linux')is logically equivalent to justunityRunning. The extra clause doesn't change behavior, but obscures intent.♻️ Proposed simplification
- if ((unityRunning || process.platform !== 'linux') && process.platform !== 'win32') { + if (unityRunning && process.platform !== 'win32') {Also applies to: 65-70
🤖 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/notification_trigger/desktop_notification_delivery.test.ts` around lines 50 - 58, The gate in the desktop notification delivery test is redundant because `unityRunning` already falls back to `true` on non-Linux platforms, so the extra `process.platform !== 'linux'` check adds no behavior. Simplify the condition near the `unityRunning` setup and the later assertion block by relying on `unityRunning` alone, keeping the logic in `desktop_notification_delivery.test.ts` clear and avoiding the unnecessary disjunct.
11-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer a top-level
import typefor consistency.Other files in this same cohort (e.g.
dock_bounce.test.ts) useimport type {ElectronApplication} from 'playwright';at the top of the file. This file instead inlines the type asimport('playwright').ElectronApplication. Purely stylistic, but worth aligning for consistency.🤖 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/notification_trigger/desktop_notification_delivery.test.ts` at line 11, The `readBadgeCount` helper is using an inline `import('playwright').ElectronApplication` type annotation instead of the preferred top-level `import type` style. Update `desktop_notification_delivery.test.ts` to add a top-level `import type {ElectronApplication} from 'playwright';` and change `readBadgeCount` to use that imported type, matching the pattern used in `dock_bounce.test.ts` and keeping imports consistent across the cohort.e2e/specs/notification_trigger/dock_bounce.test.ts (1)
71-156: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated monkey-patch/spy/restore pattern across dock_bounce.test.ts and flash_taskbar.test.ts.
installDockBounceSpy/restoreDockBouncehere are structurally identical to the inline spy-install/restore blocks inflash_taskbar.test.ts(same pattern: save original method on a marker property, push observed calls to a global array, restore infinally). Given both files touch the same productionflashFrame()path (per the “Production code path” comment), extracting a small sharedinstallMethodSpy/restoreMethodSpyhelper intoe2e/helperswould reduce duplication and keep the two test suites in sync as the spy contract evolves.🤖 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/notification_trigger/dock_bounce.test.ts` around lines 71 - 156, The dock bounce test repeats the same monkey-patch spy/restore logic used in flash_taskbar tests, so factor this pattern into a shared helper instead of duplicating it. Extract a reusable installMethodSpy/restoreMethodSpy utility under e2e/helpers, then update installDockBounceSpy and restoreDockBounce here (and the equivalent flash_taskbar setup) to use it so both suites stay aligned with the same spy contract.
🤖 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/specs/calls/calls_functionality.test.ts`:
- Around line 137-169: The try/catch around expect.poll in
calls_functionality.test.ts is too broad and turns unrelated failures into a
skip. Narrow the handling in the /call start check so only the expected “Calls
plugin/widget not available” case skips, and let other errors from
findCallsWidgetWindow, serverWin.evaluate, or expect.poll propagate as test
failures. Use the outcome detection logic in the polling block to distinguish
the widget/post cases, and avoid swallowing page/electron errors when deciding
whether to call test.skip.
In `@e2e/specs/menu_bar/edit_menu.test.ts`:
- Line 106: The afterAll cleanup in the menu_bar/edit_menu test should be
protected against a failed launch so it does not call closeElectronAppFast with
an uninitialized electronApp. Update the test setup around launchDirectTestApp
and the afterAll hook to track whether the app was successfully assigned before
attempting cleanup, and only invoke closeElectronAppFast(electronApp,
userDataDir) when that handle exists.
In `@e2e/specs/menu_bar/help_menu.test.ts`:
- Around line 22-29: The help menu test is patching refs.updateNotifier even
though that singleton is never exposed in __e2eTestRefs, so the setup fails
before the menu action is exercised. Update the test bootstrap to expose the
actual update notifier singleton used by the menu, or change the patching code
to target the same instance the click handler invokes; use __e2eTestRefs and
updateNotifier as the key symbols to locate the setup.
In `@e2e/specs/menu_bar/view_menu.test.ts`:
- Line 135: The teardown in the view menu test can run even when Electron never
finished launching, so guard the `closeElectronAppFast` call in
`view_menu.test.ts` behind a check that `electronApp` was assigned before
attempting cleanup. Update the test’s setup/teardown flow to follow the same
guarded pattern used in `focus.test.ts`, and keep the existing
`closeElectronAppFast(electronApp, userDataDir)` call only when the app launch
succeeded.
In `@e2e/specs/notification_trigger/desktop_notification_delivery.test.ts`:
- Around line 47-54: The feature-detection check in the desktop notification
test uses an immediate $ query after login, which can falsely skip the test
before the tour UI finishes mounting. Update the check around loginToMattermost
and tourButton to use an explicit bounded wait such as waitForSelector with a
timeout, and only fall back to test.skip when that wait definitively fails. Keep
the skip logic tied to the CustomizeYourExperienceTour button so the
notification flow is exercised whenever the element does appear.
In `@e2e/specs/notification_trigger/notification_click.test.ts`:
- Around line 70-88: The notification click test is bypassing the real
production handler by registering its own ipcMain listener and sending
NOTIFICATION_CLICKED directly. Update the test to exercise the actual
mention.on('click') flow in notifications/index.ts instead of the synthetic
focus callback, and assert the resulting MainWindow.show() and
TabManager.switchToTab(view.id) behavior through the normal app path.
---
Nitpick comments:
In `@e2e/specs/calls/calls_functionality.test.ts`:
- Around line 63-70: The polling logic for findCallsWidgetWindow is duplicated
in multiple places, so extract it into a shared helper such as
waitForCallsWidgetWindow(electronApp, timeoutMs) and reuse it from both tests
and the slash-command polling path. Move the repeated loop that waits up to
widgetDeadline into this helper, keep the timeout and retry interval
configurable, and update the existing call sites in calls_functionality.test.ts
to use the new helper.
- Around line 135-177: The polling logic in calls_functionality.test.ts uses a
mutable Outcome variable in the expect.poll closure, which prevents TypeScript
from narrowing it cleanly and forces redundant casts later. Refactor the polling
flow around the relevant expect.poll block and outcome handling so the callback
returns a typed result directly or add a dedicated type guard for Outcome; then
use the narrowed result when checking for the widget case in the post-poll
branch, avoiding both the non-null assertion and the extra {kind: 'widget';
window: Page} cast.
In `@e2e/specs/menu_bar/devtools_current_server.test.ts`:
- Around line 28-31: Remove the redundant unreachable return after the
test.skip(true, 'MM_TEST_SERVER_URL required') call in
devtools_current_server.test.ts; when locating the code in the test body around
the MM_TEST_SERVER_URL check, keep the test.skip guard and delete the following
return since Playwright already aborts execution there.
In `@e2e/specs/notification_trigger/desktop_notification_delivery.test.ts`:
- Around line 4-9: Reorder the imports in desktop_notification_delivery.test.ts
so the parent-relative imports from ../../fixtures/index and ../../helpers/*
come before the sibling import from ./helpers, keeping the blank-line group
separation and preserving alphabetical order within each group. Use the existing
import block in the test file as the target, especially triggerTestNotification,
verifyNotificationReceivedInDM, test, expect, demoMattermostConfig,
acquireExclusiveLock, and loginToMattermost.
- Around line 50-58: The gate in the desktop notification delivery test is
redundant because `unityRunning` already falls back to `true` on non-Linux
platforms, so the extra `process.platform !== 'linux'` check adds no behavior.
Simplify the condition near the `unityRunning` setup and the later assertion
block by relying on `unityRunning` alone, keeping the logic in
`desktop_notification_delivery.test.ts` clear and avoiding the unnecessary
disjunct.
- Line 11: The `readBadgeCount` helper is using an inline
`import('playwright').ElectronApplication` type annotation instead of the
preferred top-level `import type` style. Update
`desktop_notification_delivery.test.ts` to add a top-level `import type
{ElectronApplication} from 'playwright';` and change `readBadgeCount` to use
that imported type, matching the pattern used in `dock_bounce.test.ts` and
keeping imports consistent across the cohort.
In `@e2e/specs/notification_trigger/dock_bounce.test.ts`:
- Around line 71-156: The dock bounce test repeats the same monkey-patch
spy/restore logic used in flash_taskbar tests, so factor this pattern into a
shared helper instead of duplicating it. Extract a reusable
installMethodSpy/restoreMethodSpy utility under e2e/helpers, then update
installDockBounceSpy and restoreDockBounce here (and the equivalent
flash_taskbar setup) to use it so both suites stay aligned with the same spy
contract.
In `@e2e/specs/notification_trigger/flash_taskbar.test.ts`:
- Around line 65-73: The cleanup in the flash taskbar test restores
mainWin.flashFrame but leaves the __e2eOriginalFlashFrame marker behind on the
MainWindow object. Update the restore logic in electronApp.evaluate to also
remove that marker after restoring the original flashFrame, matching the cleanup
pattern used by restoreDockBounce in dock_bounce.test.ts and keeping the
__e2eTestRefs/MainWindow state consistent.
🪄 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: 13526025-0d89-44a0-83fd-f27b0a214a56
📒 Files selected for processing (20)
e2e/specs/calls/calls_functionality.test.tse2e/specs/focus.test.tse2e/specs/focus/app_switch_focus.test.tse2e/specs/menu_bar/clear_all_data.test.tse2e/specs/menu_bar/devtools_current_server.test.tse2e/specs/menu_bar/diagnostics.test.tse2e/specs/menu_bar/edit_menu.test.tse2e/specs/menu_bar/file_menu.test.tse2e/specs/menu_bar/full_screen.test.tse2e/specs/menu_bar/help_menu.test.tse2e/specs/menu_bar/menu.test.tse2e/specs/menu_bar/view_menu.test.tse2e/specs/menu_bar/window_menu.test.tse2e/specs/notification_trigger/desktop_notification_delivery.test.tse2e/specs/notification_trigger/dock_bounce.test.tse2e/specs/notification_trigger/flash_taskbar.test.tse2e/specs/notification_trigger/notification_badge_in_dock.test.tse2e/specs/notification_trigger/notification_badge_windows_linux.test.tse2e/specs/notification_trigger/notification_click.test.tse2e/specs/permissions/permissions_ipc.test.ts
💤 Files with no reviewable changes (4)
- e2e/specs/notification_trigger/notification_badge_in_dock.test.ts
- e2e/specs/menu_bar/menu.test.ts
- e2e/specs/menu_bar/full_screen.test.ts
- e2e/specs/notification_trigger/notification_badge_windows_linux.test.ts
yasserfaraazkhan
changed the title
mm-cloud-bot
added
release-note-none
and removed
do-not-merge/release-note-label-needed
labels
…pecs. Harden test setup/teardown guards, exercise production notification click paths, and extract shared E2E helpers for settings windows and method spies. Co-authored-by: Cursor <cursoragent@cursor.com>
github-actions
Bot
added
E2E/Run
github-actions
Bot
added
E2E/Run
github-actions
Bot
added
E2E/Run
github-actions
Bot
added
E2E/Run
Documentation Impact Analysis — no longer neededA previous automated documentation impact comment exists, but the latest analysis determined that no documentation changes are needed. The |
yasserfaraazkhan
added
the
E2E/Reset-Servers
saturninoabril
left a comment
saturninoabril
left a comment
There was a problem hiding this comment.
Thanks @yasserfaraazkhan, looking good. I left few comments for checking on what would be the better approach for conditional test when Calls plugin is not installed.
Also, in general, my observations with the E2E tests approach:
- The tests seemed to lean more on implementation details (mocks, injection) rather than user behavior where controls (like clicking, typing) and verifications are done and observed via UI interaction.
- It would be great to have consistent comments on code to easily follow along the test steps/verifications.
| await use(app); | ||
|
|
||
| await closeElectronApp(app, userDataDir, FAST_TEARDOWN); | ||
| await fs.rm(userDataDir, {recursive: true, force: true}).catch(() => {}); |
There was a problem hiding this comment.
Just curios why need to remove data dir. Maybe would be good to add comment.
| * Register Playwright globals and test-only IPC handlers. | ||
| * No-op outside NODE_ENV=test. | ||
| */ | ||
| export function maybeRegisterE2eHooks(): void { |
There was a problem hiding this comment.
How about rename to registerE2ETestHooks?
| if (!widgetWindow) { | ||
| test.skip(true, 'Calls plugin/widget not available on this test server'); |
There was a problem hiding this comment.
This conditional test is non-deterministic. In the beforeEach, should it ensure that the Calls plugin is installed so that this failed when no Calls widget found and no need to put conditional logic?
| await serverWin.press('#post_textbox', 'Enter'); | ||
|
|
||
| const widgetWindow = await waitForCallsWidgetWindow(electronApp, 30_000); | ||
| if (!widgetWindow) { |
There was a problem hiding this comment.
Same here for conditional logic when Calls plugin is not installed.
3 participants
Summary:
Release Note
Change Impact: 🟠 Medium
Regression Risk: The changes are mostly test coverage and test-helper updates, but they also touch shared E2E utilities and a few main-process test hooks. Risk is moderate because several existing test flows were refactored and platform gating was adjusted, which could expose gaps in cross-platform behavior or flaky teardown/setup paths.
QA Recommendation: Some manual QA is recommended for the newly covered desktop notification, focus, menu bar, calls, and permissions flows on the target platforms, especially around platform-specific behavior and Electron window handling.
Generated by CodeRabbitAI