e2e(helpers): Playwright helpers for server views and desktop - #3887
Merged
Conversation
… reporting. Co-authored-by: Cursor <cursoragent@cursor.com>
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 expands Playwright/Electron E2E test infrastructure: new helpers for app/window readiness, channel navigation and readiness, server context activation, popout window lifecycle, process metrics, SSO login, tray/menu interactions, user attributes/profile popovers, and public link/post APIs, plus many new and renamed spec tests across startup, menu bar, tray, notifications, and settings. ChangesE2E helpers and specs
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
yasserfaraazkhan
changed the title
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
yasserfaraazkhan
force-pushed
the
stack/e2e-02-hooks
branch
from
July 7, 2026 09:51
7f34819 to
d8437ef
Compare
yasserfaraazkhan
force-pushed
the
stack/e2e-03-helpers
branch
from
July 7, 2026 09:51
e5b07f2 to
c600cbb
Compare
Co-authored-by: Cursor <cursoragent@cursor.com>
yasserfaraazkhan
force-pushed
the
stack/e2e-02-hooks
branch
from
July 7, 2026 09:51
d8437ef to
8a0e2a6
Compare
yasserfaraazkhan
force-pushed
the
stack/e2e-03-helpers
branch
from
July 7, 2026 09:51
c600cbb to
d430bcc
Compare
…inks. Co-authored-by: Cursor <cursoragent@cursor.com>
yasserfaraazkhan
force-pushed
the
stack/e2e-02-hooks
branch
from
July 7, 2026 10:06
8a0e2a6 to
9b29e11
Compare
yasserfaraazkhan
force-pushed
the
stack/e2e-03-helpers
branch
from
July 7, 2026 10:06
d430bcc to
b9af84f
Compare
…shell. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
yasserfaraazkhan
force-pushed
the
stack/e2e-03-helpers
branch
from
July 7, 2026 10:39
b9af84f to
2a229dc
Compare
This was referenced Jul 7, 2026
Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # src/app/menus/appMenu/history.ts # src/app/menus/appMenu/view.test.js # src/app/menus/appMenu/view.ts # src/main/e2e/trayMenu.ts
![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: 11
🧹 Nitpick comments (15)
e2e/helpers/loginSso.ts (1)
156-180: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPrefer deterministic back-navigation over an OS-dependent keyboard shortcut.
navigateBackInServerViewfirst attempts a keyboard shortcut (Meta+[/Alt+ArrowLeft) before falling back towindow.history.back()viaevaluate(). Since the reliable fallback already exists, consider skipping the keyboard-press attempt entirely and driving navigation directly throughevaluate(() => window.history.back()), which avoids OS-dependent keyboard delivery that is flaky in CI/headless runs.As per coding guidelines, "Prefer invoking menu actions through the main process or IPC instead of keyboard shortcuts, because keyboard delivery is OS-dependent and unreliable in CI/headless runs."
♻️ Suggested simplification
export async function navigateBackInServerView(serverWin: ServerView): Promise<void> { const usedWebappBack = await clickWebappHistoryBackIfVisible(serverWin); if (usedWebappBack) { return; } - const backShortcut = process.platform === 'darwin' ? 'Meta+[' : 'Alt+ArrowLeft'; - await serverWin.keyboard.press(backShortcut); - - const returnedToMattermost = await expect.poll( - () => serverWin.evaluate(() => { - return Boolean(document.querySelector('`#input_loginId`')) || - Boolean(document.querySelector('.DesktopAuthToken')) || - window.location.pathname.includes('/login'); - }).catch(() => false), - {timeout: 3_000}, - ).toBe(true).then(() => true).catch(() => false); - - if (!returnedToMattermost) { - await serverWin.evaluate(() => { - window.history.back(); - }); - } + await serverWin.evaluate(() => { + window.history.back(); + }); }🤖 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/loginSso.ts` around lines 156 - 180, navigateBackInServerView currently relies on an OS-dependent keyboard shortcut before falling back to history navigation, which is flaky in CI/headless runs. Remove the keyboard-press path in navigateBackInServerView and drive the back action deterministically through serverWin.evaluate(() => window.history.back()), keeping the existing post-navigation verification logic intact with clickWebappHistoryBackIfVisible and the returnedToMattermost check.Source: Coding guidelines
e2e/helpers/appReadiness.ts (1)
9-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate main-window lookup logic across 3 files.
The
url().includes('index')window-matching logic here is copy-pasted (not imported) ine2e/helpers/blockingOverlays.ts(lines 14-20) ande2e/helpers/serverContext.ts(lines 60-66). If the matching heuristic ever needs to change (e.g., tightening the substring match), all three copies must be updated in lockstep or they'll silently diverge.Export
findMainWindowfrom this file and reuse it in the other two call sites.♻️ Suggested consolidation
-function findMainWindow(app: ElectronApplication): Page | undefined { +export function findMainWindow(app: ElectronApplication): Page | undefined { return app.windows().find((window) => { try { return window.url().includes('index'); } catch { return false; } }); }Then in
blockingOverlays.tsandserverContext.ts, replace the inline.find(...)blocks withfindMainWindow(app)imported from./appReadiness.🤖 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/appReadiness.ts` around lines 9 - 17, The main-window lookup logic is duplicated across multiple helpers, so centralize it in appReadiness by exporting findMainWindow and reusing it from blockingOverlays and serverContext. Keep the existing window.url().includes('index') heuristic in one place only, then replace the inline app.windows().find(...) blocks at the other call sites with the shared helper import.e2e/helpers/appMetrics.ts (1)
60-62: 📐 Maintainability & Code Quality | 🔵 TrivialMinor: unnamed fallback magic number.
?? 12fallback duplicates the win32 value coincidentally without an explicit named constant or comment explaining why an unmapped platform falls back to 12.♻️ Optional cleanup
+const DEFAULT_NON_TAB_PROCESS_MAX = 12; + export function getNonTabProcessMax(): number { - return NON_TAB_PROCESS_MAX[process.platform] ?? 12; + return NON_TAB_PROCESS_MAX[process.platform] ?? DEFAULT_NON_TAB_PROCESS_MAX; }🤖 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/appMetrics.ts` around lines 60 - 62, The fallback in getNonTabProcessMax currently uses an unnamed magic number, which makes the default platform behavior unclear. Replace the inline 12 fallback with a named constant or shared default in appMetrics so the intent is explicit, and update getNonTabProcessMax to use that symbol when process.platform is not in NON_TAB_PROCESS_MAX.e2e/helpers/electronApp.ts (1)
170-178: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: log swallowed cleanup errors for diagnosability.
Silently ignoring all
rmSyncfailures (not just missing-file cases) can mask persistent registry cleanup issues.♻️ Optional diagnostic logging
try { fs.rmSync(file, {force: true, recursive: true}); - } catch { + } catch (error) { // best-effort stale shard cleanup + console.debug(`Failed to remove registry shard ${file}:`, error); }🤖 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/electronApp.ts` around lines 170 - 178, The cleanup in clearAllRegistryFiles() is swallowing every fs.rmSync failure, which hides persistent registry cleanup problems. Update the try/catch around listRegistryFiles() iteration so only expected missing-file cases are ignored, and log any other rmSync errors with enough context to identify the file being removed. Keep the best-effort behavior, but make the failure path visible through clear diagnostic logging in clearAllRegistryFiles.e2e/helpers/channelReadiness.ts (1)
61-65: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winInterpolate
channelNamesafely.
channelNameis injected raw into a single-quoted JS string. While webapp channel URL-names are normally restricted to[a-z0-9-], a value containing a quote/backslash would break the evaluated script. UseJSON.stringifyfor robustness and consistency with the rest of this module.♻️ Proposed fix
export async function isOnChannelUrl(win: ServerView, channelName: string): Promise<boolean> { - return win.runInRenderer<boolean>(` - return window.location.pathname.includes('/channels/${channelName}'); - `).catch(() => false); + return win.runInRenderer<boolean>(` + return window.location.pathname.includes('/channels/' + ${JSON.stringify(channelName)}); + `).catch(() => false); }🤖 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/channelReadiness.ts` around lines 61 - 65, The isOnChannelUrl helper is interpolating channelName directly into the evaluated script, which can break the string if the value contains quotes or backslashes. Update isOnChannelUrl to build the renderer snippet using safe string serialization, such as JSON.stringify, so the channel path check remains robust and consistent with the rest of channelReadiness.ts.e2e/helpers/webappMenu.ts (1)
21-31: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueString labels are compiled as regex sources.
A
stringlabel is passed straight through as aRegExpsource, so metacharacters (.,(,+, etc.) in a literal string label would be interpreted rather than matched literally. Current callers passRegExpliterals, but sinceLabelPatternis exported and acceptsstring, consider escaping string inputs to avoid surprising matches for future callers.🤖 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/webappMenu.ts` around lines 21 - 31, String labels in serializeLabelPattern/labelMatchJs are currently treated as raw RegExp sources, so literal string labels can match metacharacters unexpectedly. Update serializeLabelPattern to distinguish string inputs from RegExp inputs and escape string labels before building the pattern, while keeping RegExp instances unchanged. Use the existing serializeLabelPattern and labelMatchJs helpers in webappMenu.ts to make the fix local and preserve current RegExp behavior.e2e/helpers/mattermostShell.ts (1)
269-290: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueStale listener may persist across different webContents.
previousListeneris removed viawc.off(...)on the currentwebContentsIdonly. If a priorlistenForNativeContextMenucall registered the listener on a differentwc, that old listener stays attached there and can still writeglobal.__e2eNativeContextMenu, racing with the new one. Tracking the previouswc/id alongside the listener (or storing per-id) would make teardown reliable.🤖 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 269 - 290, The native context menu listener cleanup in listenForNativeContextMenu is only detaching from the current webContents, so a listener registered on a different webContents can remain active and keep writing to global.__e2eNativeContextMenu. Update the helper to track the previously registered webContents (or webContentsId) together with __e2eNativeContextMenuListener, and remove the old listener from that original wc before attaching the new one. Ensure the teardown logic in listenForNativeContextMenu is reliable across repeated calls with different webContentsId values.e2e/helpers/channelNavigation.ts (1)
78-81: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPrefer polling over fixed
sleep(750)for popout detection.The two
await sleep(750)gates followed by a one-shotpopoutWindowCountread are timing-fragile: on a slow CI runner the popout may not register within 750 ms (false negative → unnecessary fallback), and on a fast one this wastes suite time. Considerexpect.poll(() => popoutWindowCount(app)).toBeGreaterThan(baseline)with a short timeout to make it deterministic.As per coding guidelines: "Prefer deterministic selectors and explicit waits over arbitrary sleeps when interacting with the UI."
Also applies to: 122-125
🤖 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/channelNavigation.ts` around lines 78 - 81, The popout detection logic in the channel navigation helper relies on fixed sleep delays followed by a single popoutWindowCount check, which is timing-fragile. Replace both sleep(750) gates with a deterministic polling wait using popoutWindowCount(app) until it becomes greater than the baseline, with a short timeout, so the helper no longer depends on arbitrary delays and remains stable on both slow and fast runners.Source: Coding guidelines
e2e/helpers/rendererUtils.ts (2)
29-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
POST_LIST_COMPOSER_SELECTORSduplicatesPOST_TEXTBOX_SELECTOR.Both are
POST_TEXTBOX_CANDIDATES.join(', '). Reuse the existingPOST_TEXTBOX_SELECTORconstant instead of recomputing an identical value.Also applies to: 100-100
🤖 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/rendererUtils.ts` at line 29, The selector constant for POST_LIST_COMPOSER_SELECTORS is duplicating the existing POST_TEXTBOX_SELECTOR value in rendererUtils; update the shared selector definitions so POST_LIST_COMPOSER_SELECTORS reuses POST_TEXTBOX_SELECTOR instead of calling POST_TEXTBOX_CANDIDATES.join(', ') again. Keep the fix in the selector constant block where POST_TEXTBOX_SELECTOR is defined so both names stay in sync and there is only one source of truth.
156-161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInconsistent contract:
HAS_CLIENT_JS_ERROR_JSis a bare expression, not a self-contained probe body.Every other exported probe (
IS_COMPOSER_INTERACTIVE_JS,IS_CHANNEL_POST_LIST_LOADED_JS,IS_CHANNEL_VIEW_LOADED_JS) is a full statement block with an explicitreturn, meant to be passed directly torunInRenderer.HAS_CLIENT_JS_ERROR_JSis a bareBoolean(...)expression that only works because the one known caller wraps it withreturn (...). A future caller following the established pattern of callingrunInRenderer(HAS_CLIENT_JS_ERROR_JS)directly would silently get no return value instead of an error.Consider wrapping it with
returnfor consistency with sibling exports.🤖 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/rendererUtils.ts` around lines 156 - 161, `HAS_CLIENT_JS_ERROR_JS` is inconsistent with the other renderer probe constants because it is only a bare expression instead of a self-contained snippet that returns a value. Update the exported template string in `rendererUtils.ts` so it follows the same pattern as `IS_COMPOSER_INTERACTIVE_JS`, `IS_CHANNEL_POST_LIST_LOADED_JS`, and `IS_CHANNEL_VIEW_LOADED_JS`, with an explicit `return` inside the probe body so it can be passed directly to `runInRenderer` without relying on a wrapper.e2e/helpers/menu.ts (1)
149-164: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated menu-traversal logic.
The BFS traversal here (stack/shift, submenu push, label match) reimplements the same pattern already in
clickApplicationMenuItem(lines 44-56). Consider extendingMenuItemMatcher/clickApplicationMenuItemwith a predicate-style matcher so this new helper can reuse the existing retry-aware traversal instead of duplicating it.As per path instructions, "reuse helpers from
e2e/helpersbefore creating new launch, login, or server-discovery logic."🤖 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/menu.ts` around lines 149 - 164, The menu search in the helper duplicates the breadth-first traversal already implemented in clickApplicationMenuItem, so refactor this path to reuse that shared logic instead of reimplementing stack/shift submenu walking. Extend MenuItemMatcher and/or clickApplicationMenuItem to accept a predicate-style matcher for labels like the “Sign in” and “Server” check, then have the new helper call that existing retry-aware traversal.Source: Path instructions
e2e/helpers/historyMenu.ts (1)
8-39: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider adding retry/transient-error handling, consistent with other menu helpers.
clickApplicationMenuIteminmenu.tsretries for 15s and swallowsisTransientEvaluateErrorfailures; this helper performs a singleapp.evaluatecall with no such resilience, making it comparatively more prone to flaky failures against a shared main-process bridge.As per path instructions, "Prefer fixes that improve determinism, reduce suite runtime, preserve real-user behavior, and strengthen shared infrastructure over patching individual flaky specs."
🤖 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/historyMenu.ts` around lines 8 - 39, The history menu helper currently does a single app.evaluate lookup/click, which makes it fragile compared with other menu helpers. Update clickHistoryMenuItem to use the same retry and transient-error handling pattern as clickApplicationMenuItem in menu.ts, including retrying for up to the shared timeout and ignoring isTransientEvaluateError failures while searching for the '&History'/'History' submenu and clicking the Back/Forward item. Keep the existing activateServerView and menu traversal logic, but wrap the evaluate-based click flow in the shared resilient helper pattern so this helper behaves deterministically across the main-process bridge.Source: Path instructions
e2e/helpers/popoutWindow.ts (3)
187-223: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider merging the two desktop-API popout helpers.
openRhsPopoutViaDesktopApiandopenThreadPopoutViaDesktopApidiffer only in the options object passed todesktopAPI.openPopout.♻️ Proposed refactor
-export async function openRhsPopoutViaDesktopApi( +async function openPopoutViaDesktopApi( win: ServerView, app: ElectronApplication, - channelPath: string, + path: string, + options: Record<string, unknown>, + errorMessage: string, ): Promise<void> { const windowPromise = waitForPopoutWindowEvent(app); const opened = await win.runInRenderer<boolean>(` - const path = ${JSON.stringify(channelPath)}; + const path = ${JSON.stringify(path)}; const api = window.desktopAPI; if (!api?.openPopout) { return false; } - void api.openPopout(path, {isRHS: true}); + void api.openPopout(path, ${JSON.stringify(options)}); return true; `, true); - expect(opened, 'desktopAPI.openPopout must be available in the server view').toBe(true); + expect(opened, errorMessage).toBe(true); await windowPromise; } + +export function openRhsPopoutViaDesktopApi(win: ServerView, app: ElectronApplication, channelPath: string) { + return openPopoutViaDesktopApi(win, app, channelPath, {isRHS: true}, 'desktopAPI.openPopout must be available in the server view'); +} + +export function openThreadPopoutViaDesktopApi(win: ServerView, app: ElectronApplication, threadPath: string) { + return openPopoutViaDesktopApi(win, app, threadPath, {}, 'desktopAPI.openPopout must be available for thread popouts'); +}🤖 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/popoutWindow.ts` around lines 187 - 223, `openRhsPopoutViaDesktopApi` and `openThreadPopoutViaDesktopApi` duplicate the same desktopAPI popout flow and only differ by the options passed to `desktopAPI.openPopout`. Refactor these helpers in `popoutWindow` into a shared function that accepts the path, the openPopout options, and the appropriate expect message, then have both existing helpers delegate to it while preserving their current behavior and assertions.
13-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated popout-window filtering into a shared helper.
The same
app.windows().filter(window => window.url().includes(POPOUT_URL_FRAGMENT))block (with try/catch) is duplicated three times.♻️ Proposed refactor
+function getPopoutWindows(app: ElectronApplication): Page[] { + return app.windows().filter((window) => { + try { + return window.url().includes(POPOUT_URL_FRAGMENT); + } catch { + return false; + } + }); +} + export function popoutWindowCount(app: ElectronApplication): number { - return app.windows().filter((window) => { - try { - return window.url().includes(POPOUT_URL_FRAGMENT); - } catch { - return false; - } - }).length; + return getPopoutWindows(app).length; }Then reuse
getPopoutWindows(app)at lines 48-54 and 170-176 as well.Also applies to: 48-54, 170-176
🤖 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/popoutWindow.ts` around lines 13 - 21, The popout-window filtering logic is duplicated in multiple places and should be centralized into a shared helper. Extract the repeated app.windows().filter(window => window.url().includes(POPOUT_URL_FRAGMENT)) with its try/catch into a reusable getPopoutWindows(app) helper in popoutWindow.ts, then update popoutWindowCount and the other duplicated call sites to use that helper instead of inlining the filter logic.
144-167: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
closePopoutWindowshould return after the target window closes. The default still waits forpopoutWindowCount(app) === 0, so it’s only safe when no other popouts exist; the only in-repo caller already passesfalse, so the aggregate wait belongs incloseAllPopouts()instead.🤖 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/popoutWindow.ts` around lines 144 - 167, closePopoutWindow currently always waits for popoutWindowCount(app) to reach zero, which makes it unsafe when other popouts may still be open. Update closePopoutWindow to only close the provided popoutWindow and return once that specific window is closed, using the existing app.browserWindow and popoutWindow close flow; move the aggregate wait into closeAllPopouts() instead. Keep the optional waitForAllClosed parameter behavior aligned with the new responsibility and ensure the in-repo caller that already passes false continues to work.
🤖 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/appReadiness.ts`:
- Around line 51-63: `waitForMainWindowChrome` is reusing the full timeout for
both the main window wait and the selector wait, which can exceed the caller’s
configured budget. Update `waitForMainWindowChrome` to track the remaining time
after `waitForMainWindow(app, {timeout})` completes, then pass only that
remaining budget to `mainWindow.waitForSelector('.ServerDropdownButton', ...)`
when `requireServerDropdown` is set. Keep the logic localized to
`waitForMainWindowChrome` and use the existing `timeout` option to cap the total
wait across both steps.
In `@e2e/helpers/blockingOverlays.ts`:
- Around line 4-7: The sibling imports in blockingOverlays.ts are not
alphabetized, so reorder the imports in the module’s import block to follow the
project’s ESLint import ordering rules. Keep the sibling group together and sort
the referenced symbols activateServerView, closeDownloadsDropdownIfOpen, and
closeOverlayWindowsIfOpen alphabetically within that group, while preserving the
type import for ServerView in the appropriate order.
In `@e2e/helpers/menu.ts`:
- Around line 140-171: `openSignInToAnotherServerModal` creates
`newServerWindowPromise` before the menu click path and can exit on a thrown
fallback click with that promise left unhandled. Update the control flow so the
window-wait promise is only created after a successful click, or ensure it is
always awaited/caught on failure paths; if `app.evaluate` or
`clickApplicationMenuItem` fails, attach a handler or cancel the pending wait to
avoid an orphaned rejection.
In `@e2e/helpers/overlayWindows.ts`:
- Around line 8-25: The new polling in hasOverlayOpen/closeOverlayWindowsIfOpen
can hang because app.evaluate is awaited without a timeout, so an unresponsive
Electron main process blocks forever and bypasses the deadline. Update the
polling loop to bound each hasOverlayOpen(app) check against a timeout or race
it with sleep(timeoutMs), matching the existing protection used in
closeOverlayWindowsIfOpen. Keep the fix localized to hasOverlayOpen and the loop
in closeOverlayWindowsIfOpen so callers like activateServerView and
dismissBlockingOverlays inherit the fail-fast behavior.
In `@e2e/helpers/popoutWindow.ts`:
- Around line 40-56: The non-null assertion in waitForPopoutWindow can still
fail at runtime because app.windows() is queried again after the poll, creating
a TOCTOU gap. Keep the successful window reference from the poll path or
revalidate immediately before returning, and in waitForPopoutWindow ensure the
last popout is checked for existence before using it instead of relying on
popouts[popouts.length - 1]!.
In `@e2e/helpers/rendererUtils.ts`:
- Around line 127-153: IS_CHANNEL_VIEW_LOADED_JS is composing a script with
duplicate __mmIsVisible declarations because it injects IS_VISIBLE_JS directly
and also through POST_TEXTBOX_RESOLVER_JS. Update the renderer probe so only one
copy of IS_VISIBLE_JS is included in the final string, likely by removing the
redundant interpolation from IS_CHANNEL_VIEW_LOADED_JS and keeping the
dependency centralized in POST_TEXTBOX_RESOLVER_JS. Make sure the assembled
script in rendererUtils stays scope-safe and still uses
__mmResolvePostTextboxRoot, __mmIsVisible, and the channel loading checks
correctly.
In `@e2e/helpers/server_api/post.ts`:
- Around line 4-6: The import list in post.ts is not alphabetized within its
local group. Reorder the existing relative imports so the symbols resolve in
ESLint order: put resolveChannelByName from ./channel before apiLogin and
apiRequest from ./client, and keep getTestServerCredentials from ./credentials
after them; preserve the same import group and only adjust ordering.
In `@e2e/helpers/shell.ts`:
- Around line 6-14: `stubShellOpenExternal` is overwriting the real
`shell.openExternal` when it is called more than once, so add a guard before
stubbing to reuse the existing `__e2eOriginalOpenExternal` if it is already set.
Keep the first bound original from `shell.openExternal.bind(shell)` intact, only
reset `__e2eOpenExternalCalls` and replace `shell.openExternal` with the test
stub, and leave `restoreShellOpenExternal` able to restore the true original
even after repeated calls.
In `@e2e/helpers/userAttributes.ts`:
- Around line 4-25: The import list in userAttributes.ts is out of ESLint
import/order and alphabetization compliance. Reorder the grouped imports so the
./server_api/* imports are alphabetized within the group (start with
resolveChannelByName and apiRequest/apiLogin in the correct order), and move the
./serverContext import to sort after the ./server_api/* group while preserving
blank-line separation. Use the existing import blocks and symbols like
resolveChannelByName, apiRequest, apiLogin, and
activateServerView/loadServerViewUrl/reloadServerView to locate the affected
section.
- Around line 293-299: The Save action in the attribute editing flow is too
broad and can click the wrong button when multiple Save buttons are visible.
Update the logic in the user attribute edit helper around the fill-and-save
sequence to scope the click to the edited row using fieldId or the surrounding
row container, so the correct custom attribute is submitted before waiting for
the corresponding edit selector.
- Around line 174-187: The fixed 500ms sleep in userAttributes.ts should be
replaced with a deterministic wait before clicking the Profile menu item. In the
user account menu flow around the opened check and the win.runInRenderer call,
wait until the Profile entry exists and is visible/enabled using a polling or
explicit wait tied to the menu item selector, then click it. Keep the existing
locate-and-click logic for the profileEntry, but remove the arbitrary
setTimeout-based pause.
---
Nitpick comments:
In `@e2e/helpers/appMetrics.ts`:
- Around line 60-62: The fallback in getNonTabProcessMax currently uses an
unnamed magic number, which makes the default platform behavior unclear. Replace
the inline 12 fallback with a named constant or shared default in appMetrics so
the intent is explicit, and update getNonTabProcessMax to use that symbol when
process.platform is not in NON_TAB_PROCESS_MAX.
In `@e2e/helpers/appReadiness.ts`:
- Around line 9-17: The main-window lookup logic is duplicated across multiple
helpers, so centralize it in appReadiness by exporting findMainWindow and
reusing it from blockingOverlays and serverContext. Keep the existing
window.url().includes('index') heuristic in one place only, then replace the
inline app.windows().find(...) blocks at the other call sites with the shared
helper import.
In `@e2e/helpers/channelNavigation.ts`:
- Around line 78-81: The popout detection logic in the channel navigation helper
relies on fixed sleep delays followed by a single popoutWindowCount check, which
is timing-fragile. Replace both sleep(750) gates with a deterministic polling
wait using popoutWindowCount(app) until it becomes greater than the baseline,
with a short timeout, so the helper no longer depends on arbitrary delays and
remains stable on both slow and fast runners.
In `@e2e/helpers/channelReadiness.ts`:
- Around line 61-65: The isOnChannelUrl helper is interpolating channelName
directly into the evaluated script, which can break the string if the value
contains quotes or backslashes. Update isOnChannelUrl to build the renderer
snippet using safe string serialization, such as JSON.stringify, so the channel
path check remains robust and consistent with the rest of channelReadiness.ts.
In `@e2e/helpers/electronApp.ts`:
- Around line 170-178: The cleanup in clearAllRegistryFiles() is swallowing
every fs.rmSync failure, which hides persistent registry cleanup problems.
Update the try/catch around listRegistryFiles() iteration so only expected
missing-file cases are ignored, and log any other rmSync errors with enough
context to identify the file being removed. Keep the best-effort behavior, but
make the failure path visible through clear diagnostic logging in
clearAllRegistryFiles.
In `@e2e/helpers/historyMenu.ts`:
- Around line 8-39: The history menu helper currently does a single app.evaluate
lookup/click, which makes it fragile compared with other menu helpers. Update
clickHistoryMenuItem to use the same retry and transient-error handling pattern
as clickApplicationMenuItem in menu.ts, including retrying for up to the shared
timeout and ignoring isTransientEvaluateError failures while searching for the
'&History'/'History' submenu and clicking the Back/Forward item. Keep the
existing activateServerView and menu traversal logic, but wrap the
evaluate-based click flow in the shared resilient helper pattern so this helper
behaves deterministically across the main-process bridge.
In `@e2e/helpers/loginSso.ts`:
- Around line 156-180: navigateBackInServerView currently relies on an
OS-dependent keyboard shortcut before falling back to history navigation, which
is flaky in CI/headless runs. Remove the keyboard-press path in
navigateBackInServerView and drive the back action deterministically through
serverWin.evaluate(() => window.history.back()), keeping the existing
post-navigation verification logic intact with clickWebappHistoryBackIfVisible
and the returnedToMattermost check.
In `@e2e/helpers/mattermostShell.ts`:
- Around line 269-290: The native context menu listener cleanup in
listenForNativeContextMenu is only detaching from the current webContents, so a
listener registered on a different webContents can remain active and keep
writing to global.__e2eNativeContextMenu. Update the helper to track the
previously registered webContents (or webContentsId) together with
__e2eNativeContextMenuListener, and remove the old listener from that original
wc before attaching the new one. Ensure the teardown logic in
listenForNativeContextMenu is reliable across repeated calls with different
webContentsId values.
In `@e2e/helpers/menu.ts`:
- Around line 149-164: The menu search in the helper duplicates the
breadth-first traversal already implemented in clickApplicationMenuItem, so
refactor this path to reuse that shared logic instead of reimplementing
stack/shift submenu walking. Extend MenuItemMatcher and/or
clickApplicationMenuItem to accept a predicate-style matcher for labels like the
“Sign in” and “Server” check, then have the new helper call that existing
retry-aware traversal.
In `@e2e/helpers/popoutWindow.ts`:
- Around line 187-223: `openRhsPopoutViaDesktopApi` and
`openThreadPopoutViaDesktopApi` duplicate the same desktopAPI popout flow and
only differ by the options passed to `desktopAPI.openPopout`. Refactor these
helpers in `popoutWindow` into a shared function that accepts the path, the
openPopout options, and the appropriate expect message, then have both existing
helpers delegate to it while preserving their current behavior and assertions.
- Around line 13-21: The popout-window filtering logic is duplicated in multiple
places and should be centralized into a shared helper. Extract the repeated
app.windows().filter(window => window.url().includes(POPOUT_URL_FRAGMENT)) with
its try/catch into a reusable getPopoutWindows(app) helper in popoutWindow.ts,
then update popoutWindowCount and the other duplicated call sites to use that
helper instead of inlining the filter logic.
- Around line 144-167: closePopoutWindow currently always waits for
popoutWindowCount(app) to reach zero, which makes it unsafe when other popouts
may still be open. Update closePopoutWindow to only close the provided
popoutWindow and return once that specific window is closed, using the existing
app.browserWindow and popoutWindow close flow; move the aggregate wait into
closeAllPopouts() instead. Keep the optional waitForAllClosed parameter behavior
aligned with the new responsibility and ensure the in-repo caller that already
passes false continues to work.
In `@e2e/helpers/rendererUtils.ts`:
- Line 29: The selector constant for POST_LIST_COMPOSER_SELECTORS is duplicating
the existing POST_TEXTBOX_SELECTOR value in rendererUtils; update the shared
selector definitions so POST_LIST_COMPOSER_SELECTORS reuses
POST_TEXTBOX_SELECTOR instead of calling POST_TEXTBOX_CANDIDATES.join(', ')
again. Keep the fix in the selector constant block where POST_TEXTBOX_SELECTOR
is defined so both names stay in sync and there is only one source of truth.
- Around line 156-161: `HAS_CLIENT_JS_ERROR_JS` is inconsistent with the other
renderer probe constants because it is only a bare expression instead of a
self-contained snippet that returns a value. Update the exported template string
in `rendererUtils.ts` so it follows the same pattern as
`IS_COMPOSER_INTERACTIVE_JS`, `IS_CHANNEL_POST_LIST_LOADED_JS`, and
`IS_CHANNEL_VIEW_LOADED_JS`, with an explicit `return` inside the probe body so
it can be passed directly to `runInRenderer` without relying on a wrapper.
In `@e2e/helpers/webappMenu.ts`:
- Around line 21-31: String labels in serializeLabelPattern/labelMatchJs are
currently treated as raw RegExp sources, so literal string labels can match
metacharacters unexpectedly. Update serializeLabelPattern to distinguish string
inputs from RegExp inputs and escape string labels before building the pattern,
while keeping RegExp instances unchanged. Use the existing serializeLabelPattern
and labelMatchJs helpers in webappMenu.ts to make the fix local and preserve
current RegExp behavior.
🪄 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: 68a49d46-cafe-403a-8cf6-ba28d1689e52
📒 Files selected for processing (28)
e2e/fixtures/index.tse2e/helpers/appMetrics.tse2e/helpers/appReadiness.tse2e/helpers/blockingOverlays.tse2e/helpers/channelMenu.tse2e/helpers/channelNavigation.tse2e/helpers/channelReadiness.tse2e/helpers/downloads.tse2e/helpers/electronApp.tse2e/helpers/helpMenuLinks.tse2e/helpers/historyMenu.tse2e/helpers/login.tse2e/helpers/loginSso.tse2e/helpers/mainWindowFocus.tse2e/helpers/mattermostShell.tse2e/helpers/menu.tse2e/helpers/overlayWindows.tse2e/helpers/popoutWindow.tse2e/helpers/prepareServerView.tse2e/helpers/rendererUtils.tse2e/helpers/serverContext.tse2e/helpers/server_api/channel.tse2e/helpers/server_api/post.tse2e/helpers/server_api/publicLinks.tse2e/helpers/shell.tse2e/helpers/trayMenu.tse2e/helpers/userAttributes.tse2e/helpers/webappMenu.ts
Tighten timeout budgets, orphaned promise handling, overlay polling bounds, and scoped UI interactions while fixing import order and duplicate renderer probe code. Co-authored-by: Cursor <cursoragent@cursor.com>
Centralize main-window and popout helpers, harden menu/history/login flows, and tighten renderer probes and registry cleanup logging. Co-authored-by: Cursor <cursoragent@cursor.com>
github-actions
Bot
added
E2E/Run
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e/helpers/historyMenu.ts (1)
16-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the shared main-process retry helper.
This inlines the same deadline/catch/sleep loop already implemented ine2e/helpers/testRefs.ts(evaluateInMainProcessWithArg). Switching to that helper keeps timeout and transient-error handling consistent across menu clicks.🤖 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/historyMenu.ts` around lines 16 - 52, The History menu click logic in historyMenu.ts reimplements the same retry/deadline/transient-error loop already handled by evaluateInMainProcessWithArg in testRefs.ts. Refactor the menu click path to call that shared main-process retry helper instead of inlining the while/catch/sleep logic, while keeping the existing menu traversal and target.click behavior inside the evaluated callback.
🤖 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.
Nitpick comments:
In `@e2e/helpers/historyMenu.ts`:
- Around line 16-52: The History menu click logic in historyMenu.ts reimplements
the same retry/deadline/transient-error loop already handled by
evaluateInMainProcessWithArg in testRefs.ts. Refactor the menu click path to
call that shared main-process retry helper instead of inlining the
while/catch/sleep logic, while keeping the existing menu traversal and
target.click behavior inside the evaluated callback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 081b3d07-53ef-4c66-869a-5f59d42aa1e2
📒 Files selected for processing (15)
e2e/helpers/appMetrics.tse2e/helpers/appReadiness.tse2e/helpers/blockingOverlays.tse2e/helpers/channelNavigation.tse2e/helpers/channelReadiness.tse2e/helpers/electronApp.tse2e/helpers/historyMenu.tse2e/helpers/loginSso.tse2e/helpers/mattermostShell.tse2e/helpers/menu.tse2e/helpers/popoutWindow.tse2e/helpers/rendererUtils.tse2e/helpers/serverContext.tse2e/helpers/userAttributes.tse2e/helpers/webappMenu.ts
✅ Files skipped from review due to trivial changes (1)
- e2e/helpers/appMetrics.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- e2e/helpers/blockingOverlays.ts
- e2e/helpers/electronApp.ts
- e2e/helpers/appReadiness.ts
- e2e/helpers/menu.ts
- e2e/helpers/rendererUtils.ts
- e2e/helpers/channelNavigation.ts
- e2e/helpers/channelReadiness.ts
- e2e/helpers/webappMenu.ts
- e2e/helpers/loginSso.ts
- e2e/helpers/serverContext.ts
- e2e/helpers/mattermostShell.ts
- e2e/helpers/popoutWindow.ts
- e2e/helpers/userAttributes.ts
saturninoabril
approved these changes
Jul 8, 2026
Comment on lines
+32
to
+33
| const menuTimeout = Math.min(Math.max(Math.floor(timeout * 0.25), 500), 5_000); | ||
| const triggerTimeout = Math.max(timeout - menuTimeout, 500); |
Contributor
There was a problem hiding this comment.
Why these timeouts has to be computed?
Contributor
Author
There was a problem hiding this comment.
@saturninoabril this was mostly an AI suggestion while I was fixing repeated CI flakiness around the channel header menu, especially in enableBookmarksBar.
The idea is timeout should mean total wait for the helper, not per step. Earlier we had fixed 15s for the trigger + 5s for the menu. When enableBookmarksBar retries and passes remaining, each attempt could still burn the full 15s + 5s. So even with a 15s outer deadline, one retry could overshoot, eat into the test timeout, and fail in a confusing way.
With the split, if you pass 20s, trigger gets ~15s and menu gets ~5s — together ~20s, not 40s. That way we stay within the budget and it’s clearer whether it failed waiting for the trigger or for the menu to open.
saturninoabril
added
the
4: Reviews Complete
yasserfaraazkhan
enabled auto-merge (squash)
July 8, 2026 05:46
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
e2e/specs/windows_and_linux_only/startup_after_reboot.test.ts (1)
48-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated toggle-and-read-config logic; same write/UI race as
autostart.test.ts.This inline block duplicates
toggleAutostartfrome2e/specs/windows_and_linux_only/autostart.test.ts(same "Changes saved" toast race described there) instead of reusing a shared helper.As per coding guidelines, "reuse helpers from e2e/helpers before creating new launch, login, or server-discovery logic."
🤖 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/windows_and_linux_only/startup_after_reboot.test.ts` around lines 48 - 52, The autostart check in startup_after_reboot.test.ts duplicates the same toggle/read-config sequence and save-toast race already handled elsewhere. Replace the inline config.json read plus click/wait block with the shared autostart helper used by autostart.test.ts (or a helper in e2e/helpers if available), so the test reuses the existing toggleAutostart behavior and avoids the UI/write timing issue. Keep the logic centered around autostartToggle and the "Changes saved" wait, but move it behind the shared helper instead of repeating it here.Source: Coding guidelines
e2e/specs/notification_trigger/no_flash_taskbar.test.ts (1)
21-45: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConfig mutation isn't restored after the test.
notifications.flashWindowis set to0for the test but never restored, unlike the flash-frame spy which is cleaned up in its ownfinally. If more tests are later added to this file or config persists across specs sharing the app instance, this leaked state could affect subsequent assertions.🔧 Proposed fix: restore original config value
const releaseLock = await acquireExclusiveLock('flash-taskbar-state'); try { - await electronApp.evaluate(() => { + const original = await electronApp.evaluate(() => { const refs = (global as any).__e2eTestRefs; const Config = refs?.Config; - if (Config) { - Config.set('notifications', {...Config.notifications, flashWindow: 0}); - } + if (!Config) { + return null; + } + const prev = Config.notifications; + Config.set('notifications', {...Config.notifications, flashWindow: 0}); + return prev; }); @@ } finally { await restoreFlashFrameSpy(electronApp); + await electronApp.evaluate((prev) => { + const refs = (global as any).__e2eTestRefs; + if (refs?.Config && prev) { + refs.Config.set('notifications', prev); + } + }, original); }🤖 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/no_flash_taskbar.test.ts` around lines 21 - 45, The test in no_flash_taskbar.test.ts mutates Config.notifications.flashWindow but never restores the original value, leaving shared state behind. In the test body around acquireExclusiveLock, save the current flashWindow value before calling Config.set, then restore it in the outer finally after restoreFlashFrameSpy so subsequent tests see the original Config state. Use the existing Config reference from __e2eTestRefs and keep the restoration paired with the current setup/cleanup flow in triggerNotificationEffects.e2e/specs/system_tray_icon/tray_restore.test.ts (1)
14-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNear-duplicate of TRAY-01 in
tray_menu.test.ts.This test's body is essentially identical to
TRAY-01(hide → verify hidden → tray click → verify restored), overlapping on linux/win32 where both run. Consider consolidating into a single test (or a shared helper) and reserving platform-specific tags to avoid redundant execution and duplicate maintenance.🤖 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_icon/tray_restore.test.ts` around lines 14 - 41, The `tray_restore` Playwright test duplicates the same hide-then-restore flow already covered by `TRAY-01` in `tray_menu.test.ts`, causing redundant execution and maintenance overlap. Consolidate this behavior into a single shared test or extract the common steps into a helper used by the existing `tray_restore`/`TRAY-01` coverage, and keep the platform tags on only one place so linux/win32 do not run the same scenario twice.e2e/specs/startup/app.test.ts (1)
139-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract duplicated empty-app launch logic; missing
waitForAppReady.Both new tests repeat an identical launch block (mkdir, write config,
electron.launch, locatewelcomeScreen), differing only in the userDataDir subfolder name. The sibling specwelcome_screen_modal.test.tsalready factors this into a locallaunchEmptyApp()helper — which additionally callsawait waitForAppReady(app)before searching for the welcome screen window (line 25 there). These two new tests skip that readiness wait and go straight toemptyApp.windows().find(...), which is inconsistent with the established pattern and may be more prone to timing flakiness if the app isn't fully initialized when windows are first enumerated.Consider promoting
launchEmptyApp()to a shared helper (e.g. undere2e/helpers) and reusing it here, including thewaitForAppReadycall.As per path instructions, "reuse helpers from
e2e/helpersbefore creating new launch, login, or server-discovery logic."Also applies to: 182-223
🤖 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/startup/app.test.ts` around lines 139 - 180, The startup tests duplicate the empty-app launch flow and omit the readiness step used elsewhere, making them inconsistent and potentially flaky. Refactor the repeated launch logic in the affected startup spec(s) into a shared helper like the existing `launchEmptyApp()` pattern from `welcome_screen_modal.test.ts`, and ensure that helper includes `waitForAppReady(app)` before looking up `welcomeScreen`. Reuse that helper in both tests so the `electron.launch` and window खोज logic stay centralized and the app is fully ready before assertions.Source: Path instructions
🤖 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/menu_bar/history_menu.test.ts`:
- Around line 35-47: Replace the one-shot `#channelHeaderTitle` reads in the
history navigation test with the existing `expectChannelTitle` helper. The
current `waitForSelector` plus `$eval` pattern in the `Back` and `Forward`
assertions only checks attachment and can race the title update after
navigation; use `expectChannelTitle` for both assertions in
`history_menu.test.ts` so the test polls until the channel header text matches
the expected value.
In `@e2e/specs/menu_bar/menu.test.ts`:
- Around line 28-62: The menu click in this test bypasses the CI-safe window
resolution used by clickApplicationMenuItem, so item.click() may not target the
intended window on headless CI. Update the test to resolve the correct Electron
window/webContents first using the same fallback strategy as e2e/helpers/menu.ts
(focused window, then the main-window ref from the E2E hooks) and pass that
target into the menu item click path. Keep the logic anchored around the menu
traversal in electronApp.evaluate and the Show Servers item lookup, and rename
the test title if needed so it matches the programmatic click behavior.
In `@e2e/specs/menu_bar/quit_menu.test.ts`:
- Around line 58-77: The fallback in quit_menu.test around clickQuitFromMenuBar
and waitForAppClose is currently unconditional, which can hide real quit
failures on Windows/Linux by forcing shutdown through ipcMain.emit. Scope the
fallback path to macOS-only behavior in the test around electronApp.evaluate, so
non-macOS runs only validate the actual menu-bar quit path and fail when it
regresses. Keep the primary assertion on waitForAppClose as the real signal and
use the platform check to preserve the Playwright/macOS workaround without
masking failures elsewhere.
In `@e2e/specs/menu_bar/view_menu.test.ts`:
- Around line 171-182: The macOS path in the search-opening test still bypasses
the main-process helper and uses a raw shortcut via firstServer.keyboard.press,
which should be replaced with the same reliable menu/IPC flow used on other
platforms. Update the view_menu test to route both branches through
openServerSearch, and if needed extend openServerSearch so it supports darwin
before the waitForSearchBarFocused/assertions that follow.
In `@e2e/specs/settings/tray_icon_theme.test.ts`:
- Around line 19-21: The tray icon theme e2e check in tray_icon_theme.test.ts is
doing a one-shot read of config.json right after the SettingsModal “Changes
saved” signal, which can race the async write. Update the assertion around
settingsWindow.waitForSelector and the subsequent config validation to poll for
the persisted trayIconTheme value instead of reading the file once, using the
existing testInfo.outputDir/config.json path and expect-based retries until it
becomes "dark".
In `@e2e/specs/system_tray_icon/window_close_tray.test.ts`:
- Around line 4-8: The import block in window_close_tray.test.ts is not
alphabetized within the local helper group because evaluateInMainProcess from
testRefs appears after tray-related imports. Reorder the imports so the helper
imports are sorted alphabetically, placing evaluateInMainProcess before
isMainWindowVisible while keeping the existing grouping and other imports
unchanged.
In `@e2e/specs/windows_and_linux_only/autostart.test.ts`:
- Around line 11-20: The toggleAutostart helper is syncing on the “Changes
saved” toast instead of the actual persisted setting, which can race with the
config write on repeated toggles. Update toggleAutostart to wait for the on-disk
autostart value in configFilePath to reflect the expected new state after
clicking the `#CheckSetting_autostart` button, and use that as the determinism
point instead of relying on .SettingsModal__saving text. Keep the existing
before/after assertion, but drive the wait from the file state so the second
invocation in MM-T2952 cannot resolve early from a stale toast.
---
Nitpick comments:
In `@e2e/specs/notification_trigger/no_flash_taskbar.test.ts`:
- Around line 21-45: The test in no_flash_taskbar.test.ts mutates
Config.notifications.flashWindow but never restores the original value, leaving
shared state behind. In the test body around acquireExclusiveLock, save the
current flashWindow value before calling Config.set, then restore it in the
outer finally after restoreFlashFrameSpy so subsequent tests see the original
Config state. Use the existing Config reference from __e2eTestRefs and keep the
restoration paired with the current setup/cleanup flow in
triggerNotificationEffects.
In `@e2e/specs/startup/app.test.ts`:
- Around line 139-180: The startup tests duplicate the empty-app launch flow and
omit the readiness step used elsewhere, making them inconsistent and potentially
flaky. Refactor the repeated launch logic in the affected startup spec(s) into a
shared helper like the existing `launchEmptyApp()` pattern from
`welcome_screen_modal.test.ts`, and ensure that helper includes
`waitForAppReady(app)` before looking up `welcomeScreen`. Reuse that helper in
both tests so the `electron.launch` and window खोज logic stay centralized and
the app is fully ready before assertions.
In `@e2e/specs/system_tray_icon/tray_restore.test.ts`:
- Around line 14-41: The `tray_restore` Playwright test duplicates the same
hide-then-restore flow already covered by `TRAY-01` in `tray_menu.test.ts`,
causing redundant execution and maintenance overlap. Consolidate this behavior
into a single shared test or extract the common steps into a helper used by the
existing `tray_restore`/`TRAY-01` coverage, and keep the platform tags on only
one place so linux/win32 do not run the same scenario twice.
In `@e2e/specs/windows_and_linux_only/startup_after_reboot.test.ts`:
- Around line 48-52: The autostart check in startup_after_reboot.test.ts
duplicates the same toggle/read-config sequence and save-toast race already
handled elsewhere. Replace the inline config.json read plus click/wait block
with the shared autostart helper used by autostart.test.ts (or a helper in
e2e/helpers if available), so the test reuses the existing toggleAutostart
behavior and avoids the UI/write timing issue. Keep the logic centered around
autostartToggle and the "Changes saved" wait, but move it behind the shared
helper instead of repeating it here.
🪄 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: 5668c30c-16c9-4f6e-9950-d5c26bf35f5c
📒 Files selected for processing (27)
e2e/specs/menu_bar/clear_all_data.test.tse2e/specs/menu_bar/diagnostics.test.tse2e/specs/menu_bar/file_menu.test.tse2e/specs/menu_bar/help_menu.test.tse2e/specs/menu_bar/history_menu.test.tse2e/specs/menu_bar/menu.test.tse2e/specs/menu_bar/quit_menu.test.tse2e/specs/menu_bar/view_menu.test.tse2e/specs/notification_trigger/no_flash_taskbar.test.tse2e/specs/notification_trigger/notification_badge.test.tse2e/specs/notification_trigger/notification_click.test.tse2e/specs/settings/autostart.test.tse2e/specs/settings/download_location.test.tse2e/specs/settings/tray_icon_theme.test.tse2e/specs/startup/app.test.tse2e/specs/startup/config_integrity.test.tse2e/specs/startup/process_metrics.test.tse2e/specs/startup/session_persistence.test.tse2e/specs/startup/welcome_screen_modal.test.tse2e/specs/startup/window_position.test.tse2e/specs/startup/window_reposition.test.tse2e/specs/system_tray_icon/tray_menu.test.tse2e/specs/system_tray_icon/tray_restore.test.tse2e/specs/system_tray_icon/window_close_tray.test.tse2e/specs/windows_and_linux_only/autostart.test.tse2e/specs/windows_and_linux_only/startup_after_reboot.test.tse2e/specs/windows_and_linux_only/window_header.test.ts
✅ Files skipped from review due to trivial changes (8)
- e2e/specs/startup/config_integrity.test.ts
- e2e/specs/settings/autostart.test.ts
- e2e/specs/startup/window_reposition.test.ts
- e2e/specs/notification_trigger/notification_click.test.ts
- e2e/specs/settings/download_location.test.ts
- e2e/specs/startup/session_persistence.test.ts
- e2e/specs/menu_bar/clear_all_data.test.ts
- e2e/specs/notification_trigger/notification_badge.test.ts
Harden menu, settings, startup, and tray specs with shared helpers, config polling, and platform-scoped quit fallback. Co-authored-by: Cursor <cursoragent@cursor.com>
github-actions
Bot
added
E2E/Run
Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
e2e/helpers/emptyApp.ts (1)
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer static
fsimport over dynamicimport('fs').
config.tsalready importsfsstatically; usingawait import('fs')mid-function here is inconsistent and adds no benefit sincefsis always needed.🤖 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/emptyApp.ts` at line 14, Replace the dynamic fs load in emptyApp with a static import to match config.ts and avoid unnecessary async loading. Update the emptyApp helper to use the fs module directly instead of awaiting import('fs'), keeping the mkdirSync usage the same and preserving the surrounding helper logic.
🤖 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/emptyApp.ts`:
- Around line 19-25: In the empty app helper, the launched Electron instance can
be left running if waitForAppReady(app) throws before the function returns.
Update the helper around electron.launch and waitForAppReady to ensure the app
handle is always closed on failure, using the app variable from this helper so
callers never lose the only reference to clean it up. If readiness fails, catch
the error, close the Electron app, and then rethrow so the failure still
propagates.
- Around line 27-31: The welcomeScreen lookup in emptyApp’s startup flow uses a
check-then-wait pattern that can miss the window if it appears between calls.
Replace the current app.windows().find(...) ?? app.waitForEvent('window', ...)
logic with the polling pattern used elsewhere so the code continuously checks
for the welcomeScreen window until it appears or times out. Keep the change
localized to the welcomeScreen wait in emptyApp.
---
Nitpick comments:
In `@e2e/helpers/emptyApp.ts`:
- Line 14: Replace the dynamic fs load in emptyApp with a static import to match
config.ts and avoid unnecessary async loading. Update the emptyApp helper to use
the fs module directly instead of awaiting import('fs'), keeping the mkdirSync
usage the same and preserving the surrounding helper logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d0dcc9dd-cdc0-4808-ab86-607b0d5607a2
📒 Files selected for processing (14)
e2e/helpers/emptyApp.tse2e/helpers/settingsConfig.tse2e/specs/menu_bar/history_menu.test.tse2e/specs/menu_bar/menu.test.tse2e/specs/menu_bar/quit_menu.test.tse2e/specs/menu_bar/view_menu.test.tse2e/specs/notification_trigger/no_flash_taskbar.test.tse2e/specs/settings/tray_icon_theme.test.tse2e/specs/startup/app.test.tse2e/specs/startup/welcome_screen_modal.test.tse2e/specs/system_tray_icon/tray_menu.test.tse2e/specs/system_tray_icon/window_close_tray.test.tse2e/specs/windows_and_linux_only/autostart.test.tse2e/specs/windows_and_linux_only/startup_after_reboot.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- e2e/specs/system_tray_icon/window_close_tray.test.ts
- e2e/specs/settings/tray_icon_theme.test.ts
- e2e/specs/menu_bar/quit_menu.test.ts
- e2e/specs/notification_trigger/no_flash_taskbar.test.ts
- e2e/specs/menu_bar/history_menu.test.ts
- e2e/specs/windows_and_linux_only/startup_after_reboot.test.ts
- e2e/specs/menu_bar/view_menu.test.ts
- e2e/specs/system_tray_icon/tray_menu.test.ts
Close the Electron app when readiness fails, poll for the welcome screen window, and use a static fs import. Co-authored-by: Cursor <cursoragent@cursor.com>
github-actions
Bot
added
E2E/Run
github-actions
Bot
added
E2E/Run
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.
Migrating Rainforest cases to Playwright surfaced repeated patterns: waiting for a server view to be interactive, opening native menus, copying public links, and recovering from blank channel states.
This PR adds shared helpers and fixture wiring so specs stay focused on behavior instead of re-implementing Electron glue in every file.
Slice of #3853. E2E coverage validated on #3853. Merge after #3885.
Release Note
Change Impact: 🟡 Medium
Regression Risk: The changes are concentrated in shared E2E helpers and fixture wiring, so regressions would mostly affect desktop Playwright test stability and coverage rather than product runtime behavior. Risk is elevated because several widely reused helpers were refactored or added, but the blast radius is still limited to test code.
QA Recommendation: Light manual QA is sufficient; prioritize running the affected E2E suites and a few representative desktop flows (menu navigation, startup, settings, tray, and login) to confirm the new helpers behave consistently across platforms.
Generated by CodeRabbitAI