E2E: Tray, settings, and deep linking - #3864
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: 35 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 (3)
📝 WalkthroughWalkthroughThis PR adds e2e deep-link helper utilities and refactors deep-linking tests to use them, migrates several specs to a faster Electron app teardown helper (closeElectronAppFast), narrows settings tests to platform-specific tags, and introduces new e2e suites for autostart, tray icon visibility, tray menu interaction, and window-close-to-tray behavior. ChangesDeep-link helpers and consuming tests
Electron app teardown migration to closeElectronAppFast
Settings test platform tag narrowing
New tray, autostart, and window-close system test suites
Sequence Diagram(s)sequenceDiagram
participant Test
participant ElectronApp
participant MainProcess
participant ServerView
Test->>ElectronApp: openDeepLinkInApp(url)
ElectronApp->>MainProcess: __e2eOpenDeepLink(url)
MainProcess->>ServerView: navigate to deep-linked channel/server
Test->>ElectronApp: waitForServerChannelNavigation(serverName, channelName)
ElectronApp->>ServerView: poll current URL
ServerView-->>ElectronApp: URL contains channelName
ElectronApp-->>Test: navigation confirmed
sequenceDiagram
participant Test
participant MainProcess
participant Tray
participant MainWindow
Test->>MainProcess: hide main window
Test->>Tray: emit tray icon click
Tray->>MainWindow: restore visibility
Test->>Tray: click tray menu item for target server
Tray->>MainWindow: switch active server view
MainWindow-->>Test: ServerDropdownButton reflects new server
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
e2e/specs/deep_linking/oauth_callback.test.ts (1)
31-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the
mainWindowfixture instead of re-deriving it.
electronApp.windows().find((window) => window.url().includes('index'))duplicates the existingmainWindowfixture, which already performs this lookup with retry/try-catch protection around.url()(avoiding a thrown error if a window's execution context is transiently destroyed). DestructuringmainWindowfrom the test fixture would remove this duplication and the unguarded.url()call.♻️ Proposed refactor
- async ({electronApp}) => { + async ({electronApp, mainWindow}) => { await waitForAppReady(electronApp); ... - const mainWindow = electronApp.windows().find((window) => window.url().includes('index')); - expect(mainWindow).toBeDefined(); await expect.poll( - () => mainWindow!.innerText('.ServerDropdownButton'), + () => mainWindow.innerText('.ServerDropdownButton'), {timeout: 15_000}, ).toBe(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/deep_linking/oauth_callback.test.ts` around lines 31 - 36, The test is re-deriving the Electron main window instead of using the existing mainWindow fixture, which duplicates lookup logic and bypasses the fixture’s safer retry/try-catch handling around .url(). Update the oauth_callback deep linking spec to destructure and use the mainWindow fixture directly in place of electronApp.windows().find(...), keeping the existing expect.poll assertions but removing the manual window lookup.e2e/specs/settings/autostart.test.ts (1)
43-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace arbitrary sleep with deterministic wait.
This backoff uses a fixed
setTimeoutsleep between retries. Since the next loop iteration already performs a boundedwaitForEvent(..., {timeout: 3_000}), this extra sleep is unnecessary and conflicts with the project preference for deterministic waits over arbitrary sleeps.As per coding guidelines, "Prefer deterministic selectors and explicit waits with
waitForSelector()orexpect().toContainText({timeout})over arbitrary sleep statements".♻️ Proposed fix
} catch (error) { if (attempt === 4) { throw error; } - await new Promise((resolve) => setTimeout(resolve, 250)); }🤖 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/settings/autostart.test.ts` at line 43, Remove the fixed `setTimeout` delay in the retry/backoff logic inside `autostart.test.ts` and rely on the existing bounded `waitForEvent(..., { timeout: 3_000 })` in the retry loop for deterministic waiting. Keep the retry behavior in the same test flow, but eliminate the arbitrary sleep so the `waitForEvent` path is the only wait mechanism used.Source: Coding guidelines
e2e/specs/system/tray_menu.test.ts (1)
48-86: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse the demo config URL here instead of hardcoding
github.com.
This keeps the assertion aligned withdemoConfig.servers[1]and avoids a brittle test if the fixture changes.🤖 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/system/tray_menu.test.ts` around lines 48 - 86, The tray menu e2e test currently hardcodes the expected server URL when verifying the switched view, which makes it brittle. Update the assertion in the TRAY-02 flow to derive the expected URL from demoConfig.servers[1] instead of matching github.com directly, and keep the check aligned with the existing targetServer/serverMap logic in tray_menu.test.ts so the test stays consistent with the fixture.
🤖 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/deeplink.ts`:
- Around line 9-21: The deeplink helper is selecting the protocol from the
Playwright process environment, which can produce the non-test scheme and break
e2e launches; update mattermostDeepLinkUrl to use the test deeplink scheme based
on the app launch mode rather than NODE_ENV in the helper process, and keep
channelDeepLinkUrl delegating through that same protocol selection so
openDeepLinkInApp and deeplink tests always receive the test scheme.
In `@e2e/specs/deep_linking/deeplink.test.ts`:
- Around line 98-149: This macOS open-url suite is bypassing the shared
`electronApp`/`mainWindow` fixtures by manually launching Electron and
re-discovering windows. Update the `test.describe('macOS open-url deep link')`
block to use `test.use({appConfig})` with the shared fixture instead of
`electron.launch`, so it inherits the darwin stability flags and longer timeout
already handled by the fixture. Also replace the inline
`app!.windows().find(...)` and `.url()`/dropdown polling with the existing
`mainWindow`/fixture-based helpers to avoid duplicating the discovery logic.
In `@e2e/specs/settings/autostart.test.ts`:
- Around line 50-68: The autostart settings test mutates a real login-item
setting and does not restore the original state, so update the SET-01 flow in
autostart.test.ts to capture the initial autostart value, perform the
toggle/assertion, and then revert it in a finally block or equivalent cleanup
path. Use the existing settings window interactions and config.json verification
around autostart to locate the test body, and ensure the cleanup runs even if
the assertion fails so subsequent tests and local runs see the original
OS/login-item state.
---
Nitpick comments:
In `@e2e/specs/deep_linking/oauth_callback.test.ts`:
- Around line 31-36: The test is re-deriving the Electron main window instead of
using the existing mainWindow fixture, which duplicates lookup logic and
bypasses the fixture’s safer retry/try-catch handling around .url(). Update the
oauth_callback deep linking spec to destructure and use the mainWindow fixture
directly in place of electronApp.windows().find(...), keeping the existing
expect.poll assertions but removing the manual window lookup.
In `@e2e/specs/settings/autostart.test.ts`:
- Line 43: Remove the fixed `setTimeout` delay in the retry/backoff logic inside
`autostart.test.ts` and rely on the existing bounded `waitForEvent(..., {
timeout: 3_000 })` in the retry loop for deterministic waiting. Keep the retry
behavior in the same test flow, but eliminate the arbitrary sleep so the
`waitForEvent` path is the only wait mechanism used.
In `@e2e/specs/system/tray_menu.test.ts`:
- Around line 48-86: The tray menu e2e test currently hardcodes the expected
server URL when verifying the switched view, which makes it brittle. Update the
assertion in the TRAY-02 flow to derive the expected URL from
demoConfig.servers[1] instead of matching github.com directly, and keep the
check aligned with the existing targetServer/serverMap logic in
tray_menu.test.ts so the test stays consistent with the fixture.
🪄 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: b2358c83-4406-4ca3-b9f0-4ff6e48f1d14
📒 Files selected for processing (12)
e2e/helpers/deeplink.tse2e/specs/deep_linking/deeplink.test.tse2e/specs/deep_linking/deeplink_running.test.tse2e/specs/deep_linking/oauth_callback.test.tse2e/specs/policy/policy.test.tse2e/specs/popup.test.tse2e/specs/settings.test.tse2e/specs/settings/autostart.test.tse2e/specs/settings/tray_icon_hide.test.tse2e/specs/system/tray_menu.test.tse2e/specs/system/tray_restore.test.tse2e/specs/system/window_close_tray.test.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
Always use mattermost-dev deep links in E2E helpers, use shared fixtures for the macOS open-url test, and restore autostart after SET-01. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
github-actions
Bot
added
E2E/Run
yasserfaraazkhan
added
E2E/Reset-Servers
dfda040
into
e2e/06-server-management
2 participants
Summary
Stacked PR 10/10 — final slice: tray, settings, deep linking, policy, popup.
Completes the #3847 split when merged after PRs 1–9.
Test plan
e2e/specs/system/tray_menu.test.tse2e/specs/deep_linking/e2e/specs/settings/Change Impact: 🟡 Medium
Regression Risk: Changes are isolated to end-to-end test helpers and test coverage, but they touch shared E2E utilities, app lifecycle/teardown helpers, and several scenarios around tray, settings, and deep linking. That creates moderate risk of brittle test regressions or flaky behavior, though production code is not directly affected.
QA Recommendation: Automated coverage should catch most issues; targeted manual QA is optional but recommended for the new tray and deep-link flows if there’s concern about platform-specific behavior.
Generated by CodeRabbitAI