test(e2e): Mattermost webapp interaction, channel UI, and downloads - #3857
Merged
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 |
Contributor
|
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 shared E2E helpers for Mattermost, downloads, and GitHub status handling, then updates Mattermost and downloads Playwright specs to use them. It also changes E2E workflow cancellation and final-status logic for PR-triggered runs and overrides. ChangesE2E Helper, Spec, and Workflow Updates
Estimated code review effort: 4 (Complex) | ~60 minutes 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
Resolve conflicts by keeping merged harness/hook fixes from #3855 and retaining PR 3 Mattermost shell helpers and specs. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
e2e/specs/mattermost/custom_groups.test.ts (2)
56-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate menu-item lookup logic.
The
hasGroupsMenuItemcheck (lines 56-64) and thegroupsClickedevaluate (lines 74-87) repeat the same selector and lowercase text-matching logic. Consider combining into a singleevaluate()call that locates and clicks the item in one pass, returning whether it was found/clicked.🤖 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/custom_groups.test.ts` around lines 56 - 87, The product switcher lookup is duplicated in the custom_groups test, with the same selector and text-matching logic used for both `hasGroupsMenuItem` and `groupsClicked`. Refactor the `firstServer!.evaluate()` flow to locate and click the User Groups item in one pass, returning whether it was found/clicked and using that result to decide whether to `test.skip()`. Keep the logic centered around the existing `hasGroupsMenuItem`/`groupsClicked` block in `custom_groups.test.ts`.
45-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
locator()for the product switcher query.locator()keeps Playwright’s auto-waiting and retry behavior for the click path, while$()returns a staticElementHandle.🤖 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/custom_groups.test.ts` at line 45, The product switcher lookup in the custom groups test is using a static ElementHandle from $(), which bypasses Playwright’s auto-waiting. Update the query in the test around the product switcher selection to use locator() on firstServer instead, and keep the subsequent click path working off that locator so retries and waiting are preserved.e2e/specs/mattermost/bookmarks.test.ts (1)
45-48: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMove shared login/channel setup to
beforeAll()for this serial suite.The suite is serial, but each test calls
loginToOffTopicChannel(). Share the login/channel setup once and reuse the preparedServerView.As per coding guidelines, “Use shared login in
beforeAll()for serial test suites instead ofbeforeEach()to avoid repeating expensive login operations.”Also applies to: 73-73, 99-99
🤖 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/bookmarks.test.ts` around lines 45 - 48, The serial `mattermost/bookmarks` suite is repeating expensive setup by calling `loginToOffTopicChannel()` in each test instead of sharing it once. Move the shared login/channel preparation into `beforeAll()` in this suite, store the resulting `ServerView`, and have the tests reuse that prepared state rather than logging in again. Update the affected tests that currently call `loginToOffTopicChannel()` so they consume the shared setup instead.Source: Coding guidelines
🤖 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/mattermostShell.ts`:
- Around line 8-13: The fallback in POST_TEXTBOX_SELECTOR is too broad because
bare [role="textbox"] can match non-composer inputs; scope this selector so it
only targets the post composer area and not search or modal fields. Update the
selector constant in mattermostShell and review the keyboard-typing flow that
relies on it in the referenced helper methods so the active element path always
resolves to the composer textbox before typing.
- Around line 231-237: The textarea branch in mattermostShell’s caret/marker
coordinate calculation is using the marker’s left edge for x, unlike the
contenteditable path which targets the word center. Update the x value returned
from the textarea-related logic to use the midpoint of the marker rectangle,
while keeping the existing y calculation unchanged, so the click lands on the
textarea word center and reduces spell-check menu misses.
- Around line 113-116: The textarea/input branch in mattermostShell helper is
setting value directly, which can make the helper report success before the app
has actually processed the input event. Update the typing path in the relevant
helper to use the native value setter pattern used by
serverView::__mmSetElementValue instead of assigning root.value directly, then
keep dispatching the input event and verifying the updated value so the keyboard
fallback still runs when needed.
In `@e2e/helpers/team.ts`:
- Around line 135-158: The fallback path in ensure teams handling can return
before the sidebar is ready when ensureMultipleTeamsViaApi(app, serverUrl,
username, password) succeeds with created=false. Update the ensure flow in
team.ts so the readiness waits for `#sidebarItem_town-square` and
`#teamSidebarWrapper` run after the API fallback as well, not only inside the
created branch, while keeping the existing renderer execution and reload logic
in app.evaluate/webContents.
In `@e2e/specs/mattermost/bookmarks.test.ts`:
- Around line 116-203: Move the bookmark cleanup into the existing try/finally
flow so it always runs even when assertions fail. Keep the external-bookmark
test logic in the try block, and call deleteAllBookmarksInBar(firstServer!) from
the finally section (alongside shell.openExternal restoration) so the created
bookmark is removed regardless of test outcome.
- Around line 6-13: The helper import block in bookmarks.test.ts is out of order
within the sibling/parent group. Reorder the imports in the test file to match
the repository’s ESLint import order, keeping groups separated by blank lines
and alphabetized within the group; use the existing symbols like
openChannelHeaderMenu, enableBookmarksBar, closeOverlayWindowsIfOpen,
loginToMattermost, prepareMattermostServerView, and waitForMattermostShell as
the location to update.
- Around line 164-184: The bookmark URL check in the server view inspection uses
substring matching, which can falsely match app/server URLs hosted on the same
domain. Update the assertion around serverViewURLs to compare parsed URL
components instead of using includes('mattermost.com'), and reuse the same
origin/path matching approach used for the actual external bookmark URL so the
test only flags real navigations to the bookmark target.
In `@e2e/specs/mattermost/custom_groups.test.ts`:
- Around line 25-36: The current custom_groups.test.ts beforeEach only skips
when MM_TEST_SERVER_URL is missing, but it still calls loginToMattermost even
when MM_TEST_USER_NAME or MM_TEST_PASSWORD are unset. Update the beforeEach
setup to validate the Mattermost login credentials before invoking
loginToMattermost, and skip the test suite gracefully when either credential is
missing, similar to the existing MM_TEST_SERVER_URL guard. Keep the check near
the existing serverMap/demoMattermostConfig login setup so the behavior is
handled consistently before firstServer is used.
---
Nitpick comments:
In `@e2e/specs/mattermost/bookmarks.test.ts`:
- Around line 45-48: The serial `mattermost/bookmarks` suite is repeating
expensive setup by calling `loginToOffTopicChannel()` in each test instead of
sharing it once. Move the shared login/channel preparation into `beforeAll()` in
this suite, store the resulting `ServerView`, and have the tests reuse that
prepared state rather than logging in again. Update the affected tests that
currently call `loginToOffTopicChannel()` so they consume the shared setup
instead.
In `@e2e/specs/mattermost/custom_groups.test.ts`:
- Around line 56-87: The product switcher lookup is duplicated in the
custom_groups test, with the same selector and text-matching logic used for both
`hasGroupsMenuItem` and `groupsClicked`. Refactor the `firstServer!.evaluate()`
flow to locate and click the User Groups item in one pass, returning whether it
was found/clicked and using that result to decide whether to `test.skip()`. Keep
the logic centered around the existing `hasGroupsMenuItem`/`groupsClicked` block
in `custom_groups.test.ts`.
- Line 45: The product switcher lookup in the custom groups test is using a
static ElementHandle from $(), which bypasses Playwright’s auto-waiting. Update
the query in the test around the product switcher selection to use locator() on
firstServer instead, and keep the subsequent click path working off that locator
so retries and waiting are preserved.
🪄 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: d28cae97-6274-4d88-8b52-5d1d31bf03be
📒 Files selected for processing (4)
e2e/helpers/mattermostShell.tse2e/helpers/team.tse2e/specs/mattermost/bookmarks.test.tse2e/specs/mattermost/custom_groups.test.ts
Scope post textbox selectors, use native value setter for inputs, center textarea spell-check coordinates, wait for team sidebar after API fallback, and harden bookmarks/custom_groups test cleanup and skips. Co-authored-by: Cursor <cursoragent@cursor.com>
github-actions
Bot
added
E2E/Run
|
❌ E2E Test Setup Failed Failed to create E2E test instances: failed to create installation: failed with status code 409 |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
yasserfaraazkhan
added
E2E/Reset-Servers
* E2E: channelMenu helper and Mattermost UI specs (4/10). * Remove local-only files accidentally included in master merge 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. Fail bookmark cleanup after retry exhaustion and guard serverEntry before prepareMattermostServerView in copy_link.test.ts.
* E2E: Downloads helpers and specs (5/10). * Address CodeRabbit review on downloads E2E helpers and specs Handle download server bind errors, use closeElectronAppFast, close the dropdown via IPC, guard teardown with allSettled, and store openPath captures on __e2eTestRefs.
github-actions
Bot
added
E2E/Run
…res. Extract shared ErrorView wait logic that accounts for retry timing and transient ERR_ABORTED during reloads, accept modern Chromium connection-reset errors for RC4 endpoints, and restore Mattermost shell readiness after drag-and-drop state resets. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/e2e-functional.yml (1)
118-130: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winScript-injection risk:
${{ inputs.pr_number }}expanded directly into the JS template.Interpolating
${{ inputs.pr_number }}straight into thescript:string lets its raw value become literal JS source before execution — the classic GitHub Actions script-injection pattern (flagged by zizmor astemplate-injection). Theremove-e2e-labeljob later in this same file already uses the safe pattern (env: PR_NUMBER: ...+process.env.PR_NUMBER); this new code should follow the same convention instead.🔒️ Proposed fix: pass the input via env instead of template expansion
- name: Update final status for all platforms uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 with: github-token: ${{ github.token }} + env: + PR_NUMBER: ${{ inputs.pr_number }} script: | const { updateFinalStatus } = require('./e2e/utils/github-actions.js'); const platforms = ${{ needs.prepare-matrix.outputs.platforms }}; const outputs = ${{ toJSON(needs.e2e-tests.outputs) }}; - const prNumber = parseInt('${{ inputs.pr_number }}', 10) || null; + const prNumber = parseInt(process.env.PR_NUMBER, 10) || null; await updateFinalStatus({Flagging: zizmor static analysis reported
error 122-122: code injection via template expansion (template-injection)for this line.🤖 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 @.github/workflows/e2e-functional.yml around lines 118 - 130, The `update-final-status` step is using unsafe template expansion for `inputs.pr_number`, which can inject raw content into the JavaScript source. Move the PR number into an environment variable in the workflow step and read it from `process.env` inside the script, following the same safe pattern already used by the `remove-e2e-label` job; keep the rest of the `updateFinalStatus` call unchanged.Source: Linters/SAST tools
♻️ Duplicate comments (1)
e2e/helpers/team.ts (1)
17-43: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSidebar readiness wait is still skipped when no team was created.
This is the same class of issue flagged on a prior commit for this function (then addressed for the renderer+API-fallback version). The function has been rewritten to only use the API path, but the readiness wait (
#sidebarItem_town-square/#teamSidebarWrapper) is still gated behindresult.created. If the user already had ≥2 teams before this call, the function returns immediately with no wait, and a caller could proceed before the sidebar has actually rendered.🩺 Proposed fix: always wait for sidebar readiness
if (result.created) { await app.evaluate(async ({webContents}, id) => { const wc = webContents.fromId(id); if (!wc || wc.isDestroyed()) { throw new Error(`webContents ${id} is not available`); } await wc.executeJavaScript('window.location.reload()', true); }, webContentsId); - - await win.waitForSelector('`#sidebarItem_town-square`', {timeout: 30_000}); - await win.waitForSelector('`#teamSidebarWrapper`', {state: 'visible', timeout: 30_000}); } + + await win.waitForSelector('`#sidebarItem_town-square`', {timeout: 30_000}); + await win.waitForSelector('`#teamSidebarWrapper`', {state: 'visible', timeout: 30_000});🤖 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/team.ts` around lines 17 - 43, The sidebar readiness wait in ensureMultipleTeams is still incorrectly tied to result.created, so callers can continue before the UI is ready when no team was newly created. Update ensureMultipleTeams to always perform the sidebar waits for `#sidebarItem_town-square` and `#teamSidebarWrapper` after the API check, while keeping the reload/webContents logic only for the created path. Use the existing ensureMultipleTeams, app.evaluate, and win.waitForSelector flow so the readiness check happens on every call.
🧹 Nitpick comments (6)
e2e/helpers/mattermostShell.ts (1)
74-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated post-textbox selector list risks drift from
POST_TEXTBOX_SELECTOR.This candidate list overlaps heavily with
POST_TEXTBOX_SELECTOR(used bypressPostTextboxKey) but is maintained separately. If one is updated (e.g., to scope the composer more tightly, as done in a prior fix) without updating the other,getPostTextboxValueand keyboard-based interactions could target different elements.Consider deriving both from a single shared array of selector strings, e.g.:
export const POST_TEXTBOX_CANDIDATES = [ '[data-slate-editor="true"]', '`#post_textbox`[contenteditable="true"]', // ... ]; export const POST_TEXTBOX_SELECTOR = POST_TEXTBOX_CANDIDATES.join(', ');Then use
POST_TEXTBOX_CANDIDATESinside the renderer script forgetPostTextboxValue.🤖 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/mattermostShell.ts` around lines 74 - 96, The post textbox selector logic is duplicated between getPostTextboxValue and POST_TEXTBOX_SELECTOR, which can drift and cause the value lookup and keyboard helpers to target different elements. Refactor mattermostShell.ts so both getPostTextboxValue and pressPostTextboxKey share a single source of truth, ideally a reusable POST_TEXTBOX_CANDIDATES array or equivalent helper, and have POST_TEXTBOX_SELECTOR derive from it while getPostTextboxValue uses the same shared candidates inside the renderer script.e2e/helpers/server_api/client.ts (1)
27-46: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSpreading
init.headerssilently drops headers when aHeadersinstance is passed.
RequestInit['headers']can be aHeadersobject, plain record, or tuple array. Object spread only copies own enumerable properties, andHeadersdoesn't expose entries that way, so...init.headersbecomes a no-op forHeadersinstances — any caller-supplied headers would be silently lost. No current caller passes aHeadersinstance, but this is a shared low-level helper intended for reuse by later PRs in the stack.♻️ Proposed fix to normalize headers before spreading
const response = await fetch(`${baseUrl}${path}`, { ...init, headers: { Authorization: `Bearer ${token}`, 'Content-Type': 'application/json', - ...init.headers, + ...Object.fromEntries(new Headers(init.headers ?? {})), }, });🤖 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/server_api/client.ts` around lines 27 - 46, The apiRequest helper merges headers by spreading init.headers, which breaks when callers pass a Headers instance because its entries are not enumerable and get dropped. Update apiRequest to normalize RequestInit['headers'] before merging in the fetch call, so Authorization and Content-Type are combined with any caller-supplied headers regardless of whether they are a Headers object, plain record, or tuple array.e2e/utils/github-actions.js (1)
185-230: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
cancelActiveE2ERunsmay miss active runs beyond the first page.
listWorkflowRunsis queried per status withper_page: 20and nobranchfilter, then filtered client-side byrunBelongsToPr. If more than 20 runs are active for a given status across the whole workflow (busy repo, many concurrent PRs), runs beyond the first page for this PR's branch are silently skipped and never cancelled. The API supports abranchquery parameter to filter server-side, which would avoid this pagination gap entirely.♻️ Proposed fix: filter server-side by branch
for (const status of ACTIVE_RUN_STATUSES) { const {data: {workflow_runs: workflowRuns}} = await github.rest.actions.listWorkflowRuns({ owner, repo, workflow_id: e2eWorkflow.id, status, + branch, per_page: 20, }); for (const run of workflowRuns) { - if (!runBelongsToPr(run, branch)) { - console.log(`Skipping E2E run ${run.id} (branch ${run.head_branch ?? 'unknown'} != ${branch})`); - continue; - } - try {Based on GitHub's REST API documentation, the "List workflow runs for a workflow" endpoint accepts a
branchparameter to narrow results server-side, which would eliminate this pagination risk.🤖 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/utils/github-actions.js` around lines 185 - 230, `cancelActiveE2ERuns` can skip active runs because `listWorkflowRuns` only fetches the first 20 results per status and filters by branch in memory. Update the workflow-runs lookup in `cancelActiveE2ERuns` to pass the resolved branch from `resolvePrHeadBranch` as a server-side `branch` filter when calling `github.rest.actions.listWorkflowRuns`, and keep the existing `runBelongsToPr` guard as a safety check. This will ensure all matching runs for the PR branch are considered before cancellation.e2e/specs/server_management/bad_servers.test.ts (1)
293-295: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a Locator instead of the legacy
page.$()handle.
mainWindow.$('.ErrorView')returns a one-shotElementHandlewith no auto-waiting/retrying, unlikemainWindow.locator('.ErrorView'). Purely stylistic here since the app is meant to be settled/trusted at this point.🤖 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/server_management/bad_servers.test.ts` around lines 293 - 295, The `bad_servers.test.ts` check is using the legacy one-shot `page.$()` handle for `.ErrorView`; switch this lookup in `getMainWindow(app)` usage to a `Locator` so it matches the newer Playwright pattern. Update the `errorView` assignment to use `mainWindow.locator('.ErrorView')` and keep the rest of the test flow unchanged.e2e/helpers/badServer.ts (2)
69-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated terminal-failure detection logic.
waitForTerminalLoadFailure(lines 76-90) andwaitForRendererReadyThenReload's inlinehasTerminalFailure(lines 102-111) implement the identicalisVisible('.ErrorView')+innerText('.ErrorView-techInfo')+ERR_ABORTEDcheck. Extracting a shared helper avoids the two copies drifting apart.♻️ Extract shared helper
+async function isTerminalFailure(mainWindow: Page, acceptedError: RegExp): Promise<{matched: boolean; errorInfo: string}> { + if (!(await mainWindow.isVisible('.ErrorView'))) { + return {matched: false, errorInfo: ''}; + } + const errorInfo = await mainWindow.innerText('.ErrorView-techInfo'); + if ((/ERR_ABORTED/).test(errorInfo) && !acceptedError.test(errorInfo)) { + return {matched: false, errorInfo}; + } + return {matched: acceptedError.test(errorInfo), errorInfo}; +}Then use it inside both
waitForTerminalLoadFailure's poll callback andwaitForRendererReadyThenReload'shasTerminalFailure.🤖 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/badServer.ts` around lines 69 - 118, The terminal-failure detection logic is duplicated between waitForTerminalLoadFailure and the inline hasTerminalFailure check inside waitForRendererReadyThenReload, so extract the shared isVisible('.ErrorView') plus innerText('.ErrorView-techInfo') with ERR_ABORTED filtering into a helper in badServer.ts and reuse it in both places. Keep the acceptedError RegExp handling and return shape the same, and have waitForTerminalLoadFailure’s expect.poll and waitForRendererReadyThenReload’s reload decision call that helper instead of maintaining separate copies.
29-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
ElectronApplicationdirectly here.e2e/helpers/badServer.tscan replace the computed alias with Playwright’s exported type, matching the rest of the e2e helpers and keeping the signature simpler.♻️ Simplify the type alias
-import {expect} from '`@playwright/test`'; -import type {Page} from 'playwright'; +import {expect} from '`@playwright/test`'; +import type {ElectronApplication, Page} from 'playwright'; ... -type ElectronApp = Awaited<ReturnType<typeof import('playwright')['_electron']['launch']>>; +type ElectronApp = ElectronApplication;🤖 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/badServer.ts` around lines 29 - 35, The helper type alias in getMainWindow is overcomplicated and should use Playwright’s exported ElectronApplication type directly instead of computing it from import('playwright')['_electron']['launch']. Update the ElectronApp alias to match the rest of the e2e helpers, then keep getMainWindow’s signature the same but backed by the simpler exported type.
🤖 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 @.github/workflows/e2e-pr-trigger.yml:
- Around line 26-35: The workflow-level permissions are too broad and should be
narrowed to each job’s actual needs. Move the GitHub permissions from the
top-level block into the individual jobs in e2e-pr-trigger.yml, especially
around cancel-on-manual-unlabel and the other job that handles label changes, so
each job only gets the specific scopes it uses. Remove any unused pull-requests:
write grant and keep actions/statuses only where the composite action or API
calls actually require them.
In `@e2e/helpers/badServer.ts`:
- Around line 22-27: The INSECURE_CIPHER_ERROR fallback in badServer.ts is using
the wrong Chromium error prefix, so it won’t match ERR_NETWORK_* terminal
failures. Update the INSECURE_CIPHER_ERROR regex to cover NETWORK_ (or
explicitly list the relevant ERR_NETWORK_* codes such as ERR_NETWORK_CHANGED and
ERR_NETWORK_IO_SUSPENDED) while keeping the existing SSL and connection-reset
cases.
In `@e2e/helpers/server_api/channel.ts`:
- Around line 39-64: The 404 detection in resolveChannelByName is brittle
because it relies on message.includes('404') inside the team loop. Update
apiGetChannelByName handling to use a typed error or explicit status code check
instead of parsing the error message, and only continue on a real not-found
response. Keep resolveChannelByName and its catch block as the lookup flow, but
make the decision based on structured error data so non-404 failures are
rethrown immediately.
---
Outside diff comments:
In @.github/workflows/e2e-functional.yml:
- Around line 118-130: The `update-final-status` step is using unsafe template
expansion for `inputs.pr_number`, which can inject raw content into the
JavaScript source. Move the PR number into an environment variable in the
workflow step and read it from `process.env` inside the script, following the
same safe pattern already used by the `remove-e2e-label` job; keep the rest of
the `updateFinalStatus` call unchanged.
---
Duplicate comments:
In `@e2e/helpers/team.ts`:
- Around line 17-43: The sidebar readiness wait in ensureMultipleTeams is still
incorrectly tied to result.created, so callers can continue before the UI is
ready when no team was newly created. Update ensureMultipleTeams to always
perform the sidebar waits for `#sidebarItem_town-square` and `#teamSidebarWrapper`
after the API check, while keeping the reload/webContents logic only for the
created path. Use the existing ensureMultipleTeams, app.evaluate, and
win.waitForSelector flow so the readiness check happens on every call.
---
Nitpick comments:
In `@e2e/helpers/badServer.ts`:
- Around line 69-118: The terminal-failure detection logic is duplicated between
waitForTerminalLoadFailure and the inline hasTerminalFailure check inside
waitForRendererReadyThenReload, so extract the shared isVisible('.ErrorView')
plus innerText('.ErrorView-techInfo') with ERR_ABORTED filtering into a helper
in badServer.ts and reuse it in both places. Keep the acceptedError RegExp
handling and return shape the same, and have waitForTerminalLoadFailure’s
expect.poll and waitForRendererReadyThenReload’s reload decision call that
helper instead of maintaining separate copies.
- Around line 29-35: The helper type alias in getMainWindow is overcomplicated
and should use Playwright’s exported ElectronApplication type directly instead
of computing it from import('playwright')['_electron']['launch']. Update the
ElectronApp alias to match the rest of the e2e helpers, then keep
getMainWindow’s signature the same but backed by the simpler exported type.
In `@e2e/helpers/mattermostShell.ts`:
- Around line 74-96: The post textbox selector logic is duplicated between
getPostTextboxValue and POST_TEXTBOX_SELECTOR, which can drift and cause the
value lookup and keyboard helpers to target different elements. Refactor
mattermostShell.ts so both getPostTextboxValue and pressPostTextboxKey share a
single source of truth, ideally a reusable POST_TEXTBOX_CANDIDATES array or
equivalent helper, and have POST_TEXTBOX_SELECTOR derive from it while
getPostTextboxValue uses the same shared candidates inside the renderer script.
In `@e2e/helpers/server_api/client.ts`:
- Around line 27-46: The apiRequest helper merges headers by spreading
init.headers, which breaks when callers pass a Headers instance because its
entries are not enumerable and get dropped. Update apiRequest to normalize
RequestInit['headers'] before merging in the fetch call, so Authorization and
Content-Type are combined with any caller-supplied headers regardless of whether
they are a Headers object, plain record, or tuple array.
In `@e2e/specs/server_management/bad_servers.test.ts`:
- Around line 293-295: The `bad_servers.test.ts` check is using the legacy
one-shot `page.$()` handle for `.ErrorView`; switch this lookup in
`getMainWindow(app)` usage to a `Locator` so it matches the newer Playwright
pattern. Update the `errorView` assignment to use
`mainWindow.locator('.ErrorView')` and keep the rest of the test flow unchanged.
In `@e2e/utils/github-actions.js`:
- Around line 185-230: `cancelActiveE2ERuns` can skip active runs because
`listWorkflowRuns` only fetches the first 20 results per status and filters by
branch in memory. Update the workflow-runs lookup in `cancelActiveE2ERuns` to
pass the resolved branch from `resolvePrHeadBranch` as a server-side `branch`
filter when calling `github.rest.actions.listWorkflowRuns`, and keep the
existing `runBelongsToPr` guard as a safety check. This will ensure all matching
runs for the PR branch are considered before cancellation.
🪄 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: a92ddf68-382b-41f5-a779-f1a4859fb236
📒 Files selected for processing (24)
.github/actions/cancel-e2e-runs/action.yml.github/workflows/e2e-functional-template.yml.github/workflows/e2e-functional.yml.github/workflows/e2e-pr-trigger.ymle2e/helpers/badServer.tse2e/helpers/channelMenu.tse2e/helpers/downloads.tse2e/helpers/downloadsDropdown.tse2e/helpers/mattermostShell.tse2e/helpers/server_api/channel.tse2e/helpers/server_api/client.tse2e/helpers/server_api/credentials.tse2e/helpers/server_api/team.tse2e/helpers/team.tse2e/specs/downloads/download_clear_all.test.tse2e/specs/downloads/video_download.test.tse2e/specs/mattermost/alt_enter.test.tse2e/specs/mattermost/context_menu.test.tse2e/specs/mattermost/custom_groups.test.tse2e/specs/notification_trigger/notification_click.test.tse2e/specs/server_management/bad_servers.test.tse2e/specs/server_management/drag_and_drop.test.tse2e/utils/analyze-flaky-test.jse2e/utils/github-actions.js
💤 Files with no reviewable changes (1)
- e2e/specs/mattermost/custom_groups.test.ts
✅ Files skipped from review due to trivial changes (2)
- e2e/helpers/server_api/credentials.ts
- e2e/specs/downloads/download_clear_all.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- e2e/specs/mattermost/alt_enter.test.ts
- e2e/helpers/downloadsDropdown.ts
- e2e/specs/mattermost/context_menu.test.ts
- e2e/helpers/downloads.ts
- e2e/helpers/channelMenu.ts
Label add/remove on PRs requires pull-requests:write in pull_request workflows; issues:write alone returns 403 from the labels API. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/e2e-functional.yml (1)
116-132: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winTemplate expansion of job outputs into inline script enables code injection.
platforms(Line 122) andoutputs(Line 123) are still interpolated directly via${{ }}into the script body rather than passed throughenv+process.env/JSON.parse, unlikePR_NUMBERwhich now correctly uses the safer pattern. If either job output contains characters that break out of the JS literal (e.g. backticks, quotes), an attacker-influenced value could inject arbitrary code that executes withgithub-token: ${{ github.token }}, which hasstatuses: writepermission in this job.🔒 Proposed fix: route platforms/outputs through env vars like PR_NUMBER
- name: Update final status for all platforms uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 env: PR_NUMBER: ${{ inputs.pr_number }} + PLATFORMS_JSON: ${{ needs.prepare-matrix.outputs.platforms }} + OUTPUTS_JSON: ${{ toJSON(needs.e2e-tests.outputs) }} with: github-token: ${{ github.token }} script: | const { updateFinalStatus } = require('./e2e/utils/github-actions.js'); - const platforms = ${{ needs.prepare-matrix.outputs.platforms }}; - const outputs = ${{ toJSON(needs.e2e-tests.outputs) }}; + const platforms = JSON.parse(process.env.PLATFORMS_JSON); + const outputs = JSON.parse(process.env.OUTPUTS_JSON); const prNumber = parseInt(process.env.PR_NUMBER, 10) || null;🤖 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 @.github/workflows/e2e-functional.yml around lines 116 - 132, The inline script in the e2e status update step still injects job outputs directly via template expansion, which can lead to code injection. Update the updateFinalStatus call site to pass platforms and outputs through env variables and read them with process.env plus JSON.parse inside the script, following the safer pattern already used for PR_NUMBER. Keep the logic in the same script block and ensure the identifiers updateFinalStatus, platforms, outputs, and PR_NUMBER are wired through environment values rather than direct `${{ }}` interpolation.Source: Linters/SAST tools
🧹 Nitpick comments (2)
e2e/helpers/server_api/client.ts (2)
33-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
ApiRequestErrorfor consistency withapiRequest.
apiLoginthrows a plainErroron failure, whileapiRequestthrowsApiRequestErrorwith astatusfield. Downstream code (e.g.resolveChannelByNameine2e/helpers/server_api/channel.ts) relies on catchingApiRequestErrorand inspecting.statusfor retry logic. Any caller wrappingapiLoginin similar status-based handling won't work since it never gets a typed error.♻️ Proposed fix
if (!response.ok) { - throw new Error(`POST /api/v4/users/login failed: ${response.status} ${await response.text()}`); + throw new ApiRequestError('POST', '/api/v4/users/login', response.status, await response.text()); }🤖 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/server_api/client.ts` around lines 33 - 35, apiLogin currently throws a plain Error on failed login, unlike apiRequest which throws ApiRequestError with a status field. Update apiLogin in client.ts to throw ApiRequestError on non-ok responses, preserving the response status and response body text so callers like resolveChannelByName can reliably branch on .status. Keep the error shape consistent with apiRequest and use the same ApiRequestError symbol for the failure path.
27-68: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo timeout on
fetchcalls.Both
apiLoginandapiRequestissuefetchwithout any timeout/AbortController. If the test server hangs or is unreachable, these calls block indefinitely, which can stall E2E CI runs rather than failing fast.🤖 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/server_api/client.ts` around lines 27 - 68, Both apiLogin and apiRequest use fetch without any timeout handling, so hanging or unreachable servers can block E2E runs indefinitely. Update these helpers to use an AbortController with a configurable timeout, and ensure the controller is passed into the fetch options in both functions. Handle timeout aborts by throwing a clear error from apiLogin and ApiRequestError in apiRequest, and keep the logic localized to these two symbols so the timeout behavior is easy to find and maintain.
🤖 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.
Outside diff comments:
In @.github/workflows/e2e-functional.yml:
- Around line 116-132: The inline script in the e2e status update step still
injects job outputs directly via template expansion, which can lead to code
injection. Update the updateFinalStatus call site to pass platforms and outputs
through env variables and read them with process.env plus JSON.parse inside the
script, following the safer pattern already used for PR_NUMBER. Keep the logic
in the same script block and ensure the identifiers updateFinalStatus,
platforms, outputs, and PR_NUMBER are wired through environment values rather
than direct `${{ }}` interpolation.
---
Nitpick comments:
In `@e2e/helpers/server_api/client.ts`:
- Around line 33-35: apiLogin currently throws a plain Error on failed login,
unlike apiRequest which throws ApiRequestError with a status field. Update
apiLogin in client.ts to throw ApiRequestError on non-ok responses, preserving
the response status and response body text so callers like resolveChannelByName
can reliably branch on .status. Keep the error shape consistent with apiRequest
and use the same ApiRequestError symbol for the failure path.
- Around line 27-68: Both apiLogin and apiRequest use fetch without any timeout
handling, so hanging or unreachable servers can block E2E runs indefinitely.
Update these helpers to use an AbortController with a configurable timeout, and
ensure the controller is passed into the fetch options in both functions. Handle
timeout aborts by throwing a clear error from apiLogin and ApiRequestError in
apiRequest, and keep the logic localized to these two symbols so the timeout
behavior is easy to find and maintain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0a08dbe7-82de-41ce-99c1-fb07333d4496
📒 Files selected for processing (9)
.github/workflows/e2e-functional.yml.github/workflows/e2e-pr-trigger.ymle2e/helpers/badServer.tse2e/helpers/mattermostShell.tse2e/helpers/server_api/channel.tse2e/helpers/server_api/client.tse2e/helpers/team.tse2e/specs/server_management/bad_servers.test.tse2e/utils/github-actions.js
🚧 Files skipped from review as they are similar to previous changes (7)
- e2e/helpers/server_api/channel.ts
- e2e/helpers/team.ts
- .github/workflows/e2e-pr-trigger.yml
- e2e/utils/github-actions.js
- e2e/specs/server_management/bad_servers.test.ts
- e2e/helpers/mattermostShell.ts
- e2e/helpers/badServer.ts
github-actions
Bot
added
E2E/Run
yasserfaraazkhan
enabled auto-merge (squash)
July 1, 2026 05:28
yasserfaraazkhan
added
E2E/Reset-Servers
…helper - Narrow INSECURE_CIPHER_ERROR to SSL handshake errors + the documented ERR_CONNECTION_RESET case; drop ERR_CONNECTION_CLOSED/ERR_NETWORK_* since those are generic and could let the RC4/TLS1.1 tests pass for the wrong reason (e.g. an unrelated CI network blip). - Harden reloadServerViewsFromMainProcess to fail loudly if ServerManager/ ViewManager/WebContentsManager are renamed, instead of silently reloading nothing and leaving the caller to time out with a generic message. - ensureMultipleTeams now returns a cleanup() that deletes the team it created via the REST API, so context_menu.test.ts's MM-T1307_2 no longer leaves an "e2e-<random>" team on the shared test server on every run against an account with fewer than 2 teams. - Remove helpers/downloadsDropdown.ts's closeDownloadsDropdownIfOpen: dead code (never imported), and overlayWindows.ts already covers closing dropdown-style overlay windows generically. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
github-actions
Bot
added
E2E/Run
yasserfaraazkhan
added a commit
that referenced
this pull request
Jul 1, 2026
…picks MAIN FIX (the "10 did not run" tests): e2e/specs/menu_bar/window_menu.test.ts imports closeDownloadsDropdownIfOpen from e2e/helpers/downloadsDropdown, but that module wasn't on this branch — I deleted it in a fix commit on sibling PR #3857 without checking whether the stacked PRs above it referenced it. That broke this PR's build: Playwright couldn't load window_menu.test.ts, so its ~10 tests never ran (showed up as "did not run" instead of "failed" or "skipped", which is why it was easy to miss until the module-not-found error was surfaced). Restored the helper on this branch verbatim from git history so PR #3862 is self-contained regardless of sibling merge order. CodeRabbit nitpicks addressed: - methodSpy.ts: routed the dock/flash-frame spy install/restore through evaluateInMainProcess[WithArg] so they get the shared transient-context retry behavior used by the tray helpers — these spies wrap install/restore around notification triggers that navigate windows, exactly the case where transient evaluate failures happen. - settingsWindow.ts: dropped the local try/catch retry loop around ipcMain.emit(SHOW_SETTINGS_WINDOW) in favor of evaluateInMainProcessWithArg, centralizing the "Execution context was destroyed" retry logic instead of duplicating it. - notification_click.test.ts: replaced two inline duplicated MainWindow visibility polls with the existing isMainWindowVisible helper in tray.ts (identical predicate — pure dedup, no behavior change). - notifications/index.ts findActiveMentionByChannelId: now iterates through ALL matches and returns the most-recently-inserted one, in case older Mention notifications for the same channel haven't dismissed yet — Map preserves insertion order, so scanning to the end reliably gives us the notification the test just displayed. All 66 existing notification unit tests still pass. Skipped one CodeRabbit nitpick: the suggestion to extend evaluateInMainProcess itself to expose SHOW_SETTINGS_WINDOW as a param — the current usage via evaluateInMainProcessWithArg already achieves the goal with no helper change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
saturninoabril
approved these changes
Jul 2, 2026
saturninoabril
left a comment
saturninoabril
left a comment
Contributor
There was a problem hiding this comment.
Hi @yasserfaraazkhan, lots to digest but looks good to me. Left comment to understand the idea behind the runInRenderer (non-blocking).
| webContentsId: number, | ||
| ): Promise<void> { | ||
| await win.waitForSelector(TEAM_SIDEBAR_BUTTON, {timeout: 15_000}); | ||
| const point = await win.runInRenderer(` |
Contributor
There was a problem hiding this comment.
Can you explain why it needs to do win.runInRenderer? Just curious since they are customize and I wonder how it helps with the testing of the app.
Contributor
Author
There was a problem hiding this comment.
@saturninoabril , win.runInRenderer is the Electron's webContents.executeJavaScript().
in the side bar we can right click to get the context menu. But they menu is not accessible through the playwright.
This was referenced Jul 2, 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.
Summary:
New shared helpers (
mattermostShell,team,channelMenu,downloads,downloadsDropdown) — these are the interaction layer for anything that touches the loaded Mattermost webapp or file downloads.mattermostShell/team— login recovery, shell readiness, team sidebar after API fallback. Bookmarks and custom groups specs exercise the trickiest paths (session/API, sidebar state).Downloads — local HTTP server in
downloads.tsplus IPC to drive the downloads dropdown/manager. Teardown usesPromise.allSettledand fast app closeSpecs are the contract — if a helper API feels awkward, check whether the spec reads clearly
Release Note
Change Impact: 🟡 Medium
Regression Risk: Shared E2E helpers were added/expanded across many Mattermost and downloads specs, including complex renderer DOM/editor interactions, shell/server-view readiness + reload recovery, and authenticated API flows (login, channel/team resolution, team creation). While scoped to the E2E harness (not production code), selector/IPC/browser-context assumptions and session/token handling could affect multiple test suites and introduce flakiness.
QA Recommendation: Prefer full CI execution of all affected Mattermost UI/specs (copy link/context menus/bookmarks/custom groups/alt-enter) and downloads E2E suites (open/cancel/clear/all/manager/dropdown items/video). Minimal manual QA only if needed: one quick smoke run locally for (1) composing/post editor insertion and (2) a basic download flow (open or cancel) to catch obvious selector/IPC issues.
Generated by CodeRabbitAI