test(e2e): Server lifecycle, startup, tray/settings, and deep linking - #3860
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 new E2E helpers for error views, settings windows, deep links, and shell interactions, while updating server-management, startup, tray, popout, settings, and Linux specs to use shared launch, reload, and shutdown flows. ChangesServer management error handling
App lifecycle and window behavior
Deep linking and settings behavior
Linux-specific UI behavior
Estimated code review effort: 4 (Complex) | ~45 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 harness/hook conflicts by keeping merged #3855 changes. Co-authored-by: Cursor <cursoragent@cursor.com>
|
❌ E2E Test Setup Failed Failed to create E2E test instances: failed to create installation: failed with status code 409 |
![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: 3
🤖 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/server_management/bad_servers.test.ts`:
- Around line 381-390: The test currently checks for .ErrorView too early,
before the trusted-server navigation path in launchDirectTestApp() and the main
window has finished loading. Update the bad_servers.test.ts flow around
launchDirectTestApp, mainWindow, and ErrorView so the test first waits for the
server view to fully load expired.badssl.com, then assert that .ErrorView
remains absent after that navigation has completed.
- Around line 304-307: The server lookup in the Mattermost test is indexing
buildServerMap() too early and can still hit the unreachable view, so wait until
the Mattermost entry is actually present before reading
serverMap[demoMattermostConfig.servers[0].name][0]. Update the bad_servers test
around buildServerMap and mmEntry retrieval to poll for the Mattermost server
entry (using the existing demoMattermostConfig.servers[0].name key) before
accessing .win, so the URL check only runs after the correct view is available.
In `@e2e/specs/server_management/certificate_trust.test.ts`:
- Around line 37-52: The certificate trust spec is duplicating Electron startup
logic instead of using the shared direct-launch helper. Update the test to use
launchDirectTestApp from e2e/helpers/directLaunch.ts, passing the needed env
overrides and writeConfig: false so it matches the existing harness behavior.
Remove the open-coded electron.launch, waitForAppReady, and
registerElectronMainProcess setup from certificate_trust.test.ts and rely on the
helper’s launch flow instead.
🪄 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: b12f0295-564c-4498-9bbd-ee194e76f208
📒 Files selected for processing (10)
e2e/helpers/errorView.tse2e/specs/server_management/add_server_modal.test.tse2e/specs/server_management/bad_servers.test.tse2e/specs/server_management/certificate_trust.test.tse2e/specs/server_management/configure_server_modal.test.tse2e/specs/server_management/edit_server_modal.test.tse2e/specs/server_management/header.test.tse2e/specs/server_management/long_server_name.test.tse2e/specs/server_management/remove_server_modal.test.tse2e/specs/server_management/tab_management.test.ts
Poll for Mattermost server view registration, wait for trusted cert navigation before asserting ErrorView absence, and reuse launchDirectTestApp in certificate_trust.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 |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
yasserfaraazkhan
added
E2E/Reset-Servers
|
❌ E2E Test Setup Failed Failed to create E2E test instances: installation wait cancelled: context canceled |
* E2E: Tray, settings, deep linking, and misc specs (10/10). * Address CodeRabbit review on tray/deeplink/settings E2E specs Always use mattermost-dev deep links in E2E helpers, use shared fixtures for the macOS open-url test, and restore autostart after SET-01.
github-actions
Bot
added
E2E/Run
The diagnostic build from the previous commit found the actual cause: ServerDropdownButton is fully visible/enabled/unobstructed at click time (ruling out every DOM/focus theory from the prior 3 fix attempts), and the dropdown.html window never appears at all. Root cause is in production code: ServerDropdownView.handleOpen() (src/app/mainWindow/serverDropdownView.ts) silently no-ops if its WebContentsView doesn't exist yet. That view is created lazily by init(), which runs on the MAIN_WINDOW_CREATED event — a boot-time race that can still be pending when a freshly-launched shared app's first test clicks the button under CI load. A single click landing in that window is lost forever, since nothing else triggers dropdown creation afterward — this is why the single-click+20s-poll version (previous commit) could never succeed no matter how long the timeout, and why the earlier 3x click-and-poll retry (5s per attempt, 15s total) sometimes wasn't long enough. The earlier "it's a toggle, re-clicking closes it" concern only applies *after* a successful open sets state on both the main and renderer side — a click that lands before the view exists changes no state anywhere, so re-clicking is safe and just retries the open. Replaced with a retry loop (click, poll 3s, repeat) over a 40s budget, long enough to survive the boot-time race while still safely retrying instead of guessing a longer single timeout. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
yasserfaraazkhan
added a commit
that referenced
this pull request
Jul 1, 2026
…e catch settingsWindow.ts / autostart.test.ts: same bugs already found and fixed in the sibling PR #3860 (both branches independently introduced this file from the same base) — waitForLoadState() failures were being swallowed and returned as a possibly-broken Page outside the retry loop, and the autostart restore step could fail silently, leaving the real OS autostart entry toggled for subsequent CI runs with no warning. src/main/notifications/index.ts: the new __e2eDisplayAndClickMention test hook reached into NotificationManager's private allActiveNotifications field via `as unknown as {...}`, defeating type-checking — a future rename of that field would silently break the hook instead of failing to compile. Added a small, type-safe findActiveMentionByChannelId() method on the class instead, used by the test hook. Verified: `tsc` (full check-types) clean, all 66 existing notification unit tests still pass. desktop_notification_delivery.test.ts: readBadgeCount's bare `catch { return 0 }` masked any real failure as "no badge" — but the non-darwin call sites are already gated behind the unityRunning check at the call site, so app.getBadgeCount() isn't expected to throw there; removed the redundant catch so a real failure surfaces instead of being silently scored as a passing badge-count of 0. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…wn view Diagnosed from CI trace (28540903308 artifact 8020461659) after 6 failed guesses on the "dropdown click never opens the dropdown" symptom. The trace showed: - 13 clicks on .ServerDropdownButton over 40s, all producing no dropdown - A "Close page" API call from bad_servers.test.ts:127 (my beforeEach) at 200813ms — before any test-body code ran - Failure screenshot showed the .ServerDropdownButton visibly present, unobstructed, and clickable The root cause is the defensive beforeEach I added in commit 84e0523: `win.close()` on any window whose URL `.includes('dropdown')` matches: • dropdown.html ← the SERVER DROPDOWN's WebContentsView (loaded at MAIN_WINDOW_CREATED, needed for OPEN_SERVERS_DROPDOWN to render anything) • downloadsDropdown.html • downloadsDropdownMenu.html Playwright's Page.close() destroys the underlying WebContents. After that, every subsequent click on .ServerDropdownButton reaches ServerDropdownView.handleOpen() in the main process, which no-ops because this.view.webContents is destroyed — the setBounds/addChildView calls either throw and get swallowed or silently do nothing visible. That explains why 6 prior fix attempts (retry-click, bringToFront, longer timeouts, retry-loop, etc.) all failed identically: the target was correct, the button was clickable, the click was landing, but the dropdown's own infrastructure had already been destroyed at test setup. Removed the beforeEach entirely. The failure-recovery it defended against (stray modal windows persisting from a prior test's failure) is a theoretical edge case; the correctness cost of destroying the dropdown view at every test's start is real and confirmed. If serial-mode state leaks ever become a real problem, prefer sending CLOSE_SERVERS_DROPDOWN IPC (which properly closes without destroying the view) or a keyboard Escape at test end. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
github-actions
Bot
added
E2E/Run
Diagnosed from CI screenshot/trace (run 28548703597, macOS, RC4 cipher test): SyntaxError: Unexpected end of JSON input at bad_servers.test.ts:207, JSON.parse(fs.readFileSync(configPath, 'utf8')). This is not hypothetical — traced to the actual write path: Config. saveLocalConfigData() persists via JsonFileManager.write() (src/common/JsonFileManager.ts:24-37), which calls Node's fs.writeFile(). That truncates the file before writing the new content; it is not an atomic write-to-temp-then-rename. The E2E test reads the same file from a separate process (the Playwright runner) while the Electron app may still be mid-write, so an empty or partial read is a real, reproducible race, not a one-off. All 4 tests in the "Adding servers via Add Server Modal" block had the identical unguarded pattern; RC4 happened to hit it on this run, but any of the four could. Replaced with findServerInConfig(), which treats a read or parse failure as "not ready yet" (returns undefined) so expect.poll keeps retrying instead of the exception failing the test outright. Separately: this run also showed "(@ci-macos) Worker teardown timeout of 90000ms exceeded" affecting unrelated files (window_menu, notification_click, permissions_ipc, policy, popup) — left out of scope as a pre-existing CI infrastructure flake, not something introduced by this file's changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
github-actions
Bot
added
E2E/Run
saturninoabril
approved these changes
Jul 2, 2026
saturninoabril
left a comment
saturninoabril
left a comment
Contributor
There was a problem hiding this comment.
Thanks @yasserfaraazkhan, LGTM!
…ement # Conflicts: # e2e/helpers/mattermostShell.ts # e2e/specs/deep_linking/deeplink.test.ts # e2e/specs/mattermost/copy_link.test.ts # e2e/specs/server_management/bad_servers.test.ts # e2e/specs/server_management/drag_and_drop.test.ts
|
❌ E2E Test Setup Failed Failed to create E2E test instances: failed to create installation: failed with status code 409 |
yasserfaraazkhan
added
E2E/Reset-Servers
github-actions
Bot
added
E2E/Run
github-actions
Bot
added
E2E/Run
This was referenced Jul 3, 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:
errorView.ts— timing around ErrorView and certificate dialogs: reload server views after the renderer is up so load failures aren’t missed. Used heavily bybad_serversandcertificate_trust; if those look brittle, start here.deeplink.ts— buildsmattermost-dev://URLs (not tied to Playwright workerNODE_ENV). Deep link, OAuth callback, and “app already running” specs all go through this.serverMap, and cert auto-trust hooks from test(e2e): Playwright harness and main-process hooks for migration stack #3855.finallyrestores prior state). Platform guards matter — skim forprocess.platform/test.skipbefore assuming every file runs everywhere.Release Note
Change Impact: 🟠 Medium
Regression Risk: Shared E2E test harness utilities were added/expanded (renderer reload + error-view detection, deep-link navigation, and a faster app teardown helper) and are used across many specs; if any synchronization/teardown logic is off, it can cause widespread CI flakiness/hangs. While this targets test code (not production business logic), it touches certificate-trust/reload flows and multiple lifecycle paths with potentially timing-sensitive selectors and polling.
QA Recommendation: Re-run the full affected E2E suites end-to-end. If they’re green, a small manual smoke pass is recommended on (1) server error view reload/recovery (error appears then disappears) and (2) “trust invalid/expired certificate” resulting in server view loading.
Generated by CodeRabbitAI