e2e(ci): bootstrap public links and flaky-test reporting for Playwright CI - #3885
Conversation
… reporting. Co-authored-by: Cursor <cursoragent@cursor.com>
|
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 an E2E bootstrap script that enables public links on the test server, updates app menu actions to fall back to the active tab view, and expands tray menu item matching to support roles and normalized labels. ChangesEnable public links setup
App menu active tab fallback
Tray menu matching
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/scripts/enable-public-links.mjs`:
- Around line 51-68: The config update in main currently spreads the GET
/api/v4/config response back into PUT /api/v4/config, which can overwrite
sensitive settings with sanitized placeholders. Update enable-public-links.mjs
so it only sends the specific fields that need changing, or switch to an update
path that preserves existing secrets, and keep the logic around apiLogin,
apiRequest, and the ServiceSettings/FileSettings update focused on enabling
public links and setting SiteURL.
🪄 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: efcf2fce-6bfd-45d1-be00-66303ba7f109
📒 Files selected for processing (3)
.github/workflows/e2e-functional-template.ymle2e/scripts/enable-public-links.mjse2e/utils/analyze-flaky-test.js
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
github-actions
Bot
added
E2E/Run
Co-authored-by: Cursor <cursoragent@cursor.com>
github-actions
Bot
added
E2E/Run
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
e2e/scripts/enable-public-links.ts (2)
78-82: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead branch:
siteURLis always truthy.
siteURLfalls back tobaseUrl, which is already validated as non-empty at the top of the file, so thesiteURL ? {...} : {}conditional never takes the falsy path.🤖 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/scripts/enable-public-links.ts` around lines 78 - 82, The conditional spread in the enable-public-links script is dead code because `siteURL` is always non-empty after falling back to `baseUrl`. Simplify the `patch` construction in `enable-public-links.ts` by removing the `siteURL ? ... : {}` branch and always include `ServiceSettings.SiteURL` in the `ConfigPatch`, using the `siteURL` value resolved near `ConfigPatch` creation.
31-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
e2e/helpers/server_api/client.ts’sapiLogin
e2e/helpers/server_api/client.tsalready exports an API login helper, so this script shouldn’t duplicate the/api/v4/users/loginflow here. Import and use the shared helper 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/scripts/enable-public-links.ts` around lines 31 - 52, The enable-public-links script duplicates the login flow instead of using the shared helper already exported by e2e/helpers/server_api/client.ts. Replace the local apiLogin() implementation in enable-public-links.ts with an import of the shared apiLogin and update the call site to use it. Keep the script’s token retrieval behavior the same, but rely on the shared helper so the /api/v4/users/login logic lives in one place.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/scripts/enable-public-links.ts`:
- Around line 31-67: The network calls in apiLogin and apiRequest have no
timeout, so the script can hang indefinitely if the test server is slow or
unresponsive. Add an AbortController-based timeout wrapper around each fetch
call in these helpers, and make sure the timeout is surfaced in the thrown error
so CI fails fast instead of stalling.
---
Nitpick comments:
In `@e2e/scripts/enable-public-links.ts`:
- Around line 78-82: The conditional spread in the enable-public-links script is
dead code because `siteURL` is always non-empty after falling back to `baseUrl`.
Simplify the `patch` construction in `enable-public-links.ts` by removing the
`siteURL ? ... : {}` branch and always include `ServiceSettings.SiteURL` in the
`ConfigPatch`, using the `siteURL` value resolved near `ConfigPatch` creation.
- Around line 31-52: The enable-public-links script duplicates the login flow
instead of using the shared helper already exported by
e2e/helpers/server_api/client.ts. Replace the local apiLogin() implementation in
enable-public-links.ts with an import of the shared apiLogin and update the call
site to use it. Keep the script’s token retrieval behavior the same, but rely on
the shared helper so the /api/v4/users/login logic lives in one place.
🪄 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: bd0d71ee-c67b-4d90-ab30-23ec7b5932e1
⛔ Files ignored due to path filters (1)
e2e/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
.github/workflows/e2e-functional-template.ymle2e/package.jsone2e/scripts/enable-public-links.tse2e/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/e2e-functional-template.yml
…inks. Co-authored-by: Cursor <cursoragent@cursor.com>
github-actions
Bot
added
E2E/Run
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e/scripts/enable-public-links.ts (1)
26-26: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueTrailing-slash trim only strips one slash.
replace(/\/$/, '')won't normalize a URL with multiple trailing slashes (e.g.,https://host//). Low risk since this is CI-controlled input, but a+quantifier is more robust.♻️ Optional fix
- const baseUrl = (process.env.MM_TEST_SERVER_URL ?? '').replace(/\/$/, ''); + const baseUrl = (process.env.MM_TEST_SERVER_URL ?? '').replace(/\/+$/, '');🤖 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/scripts/enable-public-links.ts` at line 26, The base URL normalization in enable-public-links.ts only removes a single trailing slash, so update the baseUrl assignment to trim any number of trailing slashes instead of just one. Use the existing baseUrl logic in the e2e/scripts/enable-public-links.ts flow and adjust the replace pattern to handle repeated slashes robustly while preserving the rest of the URL handling.
🤖 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/scripts/enable-public-links.ts`:
- Line 26: The base URL normalization in enable-public-links.ts only removes a
single trailing slash, so update the baseUrl assignment to trim any number of
trailing slashes instead of just one. Use the existing baseUrl logic in the
e2e/scripts/enable-public-links.ts flow and adjust the replace pattern to handle
repeated slashes robustly while preserving the rest of the URL handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fec34423-30be-4cb5-ba52-261b534e3a5a
📒 Files selected for processing (1)
e2e/scripts/enable-public-links.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/app/menus/appMenu/view.test.js (1)
283-283: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing equivalent fallback test for "Clear Cache and Reload".
Good fix on line 283 and the new active-tab fallback test for Reload. However,
view.tsapplies the identicalgetFocusedView() ?? getCurrentActiveTabView()fallback to "Clear Cache and Reload" (line 123), but no test covers that handler's active-tab fallback path.✅ Suggested additional test
+ it('should clear cache and reload active tab when focused view is unavailable', () => { + WebContentsManager.getFocusedView.mockReturnValue(null); + TabManager.getCurrentActiveTabView.mockReturnValue(mockView); + + localizeMessage.mockImplementation((id) => { + if (id === 'main.menus.app.view.clearCacheAndReload') { + return 'Clear Cache and Reload'; + } + return id; + }); + + const menu = createViewMenu(); + const clearCacheMenuItem = menu.submenu.find((item) => item.label === 'Clear Cache and Reload'); + clearCacheMenuItem.click(); + expect(WebContentsManager.clearCacheAndReloadView).toHaveBeenCalledWith(mockView.id); + });Also applies to: 300-316
🤖 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 `@src/app/menus/appMenu/view.test.js` at line 283, Add a missing fallback test for the “Clear Cache and Reload” menu action in view.test.js. The handler in view.ts uses the same getFocusedView() ?? getCurrentActiveTabView() fallback as Reload, so extend the existing test coverage around the Clear Cache and Reload path to simulate no focused view and a non-null TabManager.getCurrentActiveTabView() result. Verify the Clear Cache and Reload handler uses the active tab view and behaves correctly when focus is absent, referencing the same menu-action wiring used by the Reload tests.src/app/menus/appMenu/view.ts (1)
114-114: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate fallback logic across files — consider extracting a shared helper.
The same
getFocusedView() ?? getCurrentActiveTabView()pattern appears twice here and is duplicated asgetHistoryNavigationView()inhistory.ts. Extracting this into a single shared utility (e.g., inwebContentsManageror a small menu-utils module) would avoid divergence if the fallback logic changes later.♻️ Proposed refactor sketch
+// e.g. in a shared module +export function getActiveNavigationView() { + return WebContentsManager.getFocusedView() ?? TabManager.getCurrentActiveTabView(); +}click() { - const view = WebContentsManager.getFocusedView() ?? TabManager.getCurrentActiveTabView(); + const view = getActiveNavigationView(); if (view) { view.reload(view.currentURL); } },Also applies to: 123-123
🤖 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 `@src/app/menus/appMenu/view.ts` at line 114, The focused-view fallback logic is duplicated between the app menu view code and the history navigation helper, so extract it into one shared utility and reuse it from both places. Add a helper with a clear name such as getHistoryNavigationView or a shared webContentsManager/menu utility, and update the existing use in view.ts to call that helper instead of inlining WebContentsManager.getFocusedView() ?? TabManager.getCurrentActiveTabView(). Make sure history.ts and any other callers use the same shared function so the fallback behavior stays consistent in one place.src/main/e2e/trayMenu.ts (1)
80-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftNo test coverage for the new click-routing logic.
createClickTrayMenuItemnow has three distinct branches (role:,tray:settings, and fallback label matching) plus new recursive/normalization helpers, but no test file is included in this cohort fortrayMenu.ts. This is critical-path logic for the E2E harness (silent mismatches here would cause flaky/broken E2E runs rather than clear failures). Consider adding unit tests covering: role-based click success/failure, settings/preferences matching (raw and normalized), and truncated-label fallback matching.🤖 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 `@src/main/e2e/trayMenu.ts` around lines 80 - 98, Add unit tests for createClickTrayMenuItem to cover all routing branches and helper behavior. Verify the role: path succeeds and throws when clickTrayMenuItemsByRole cannot find a role, the tray:settings path matches the settings/preferences item through clickTraySettingsMenuItem, and the fallback path uses clickTrayMenuItems with both exact and truncated labels. Use the createClickTrayMenuItem, clickTrayMenuItemsByRole, clickTraySettingsMenuItem, and clickTrayMenuItems symbols to locate the logic and validate success/failure cases.
🤖 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 `@src/main/e2e/trayMenu.ts`:
- Around line 18-29: Update clickTrayMenuItemsByRole() so it behaves like
findTrayMenuItemByLabelPredicate() by ignoring disabled or hidden menu items
before clicking them, and normalize the requested role before comparing it with
item.role so Electron role values are matched case-insensitively (for example,
treating Quit and quit as the same). Make the check inside
clickTrayMenuItemsByRole() use the normalized role and preserve the recursive
submenu traversal.
---
Nitpick comments:
In `@src/app/menus/appMenu/view.test.js`:
- Line 283: Add a missing fallback test for the “Clear Cache and Reload” menu
action in view.test.js. The handler in view.ts uses the same getFocusedView() ??
getCurrentActiveTabView() fallback as Reload, so extend the existing test
coverage around the Clear Cache and Reload path to simulate no focused view and
a non-null TabManager.getCurrentActiveTabView() result. Verify the Clear Cache
and Reload handler uses the active tab view and behaves correctly when focus is
absent, referencing the same menu-action wiring used by the Reload tests.
In `@src/app/menus/appMenu/view.ts`:
- Line 114: The focused-view fallback logic is duplicated between the app menu
view code and the history navigation helper, so extract it into one shared
utility and reuse it from both places. Add a helper with a clear name such as
getHistoryNavigationView or a shared webContentsManager/menu utility, and update
the existing use in view.ts to call that helper instead of inlining
WebContentsManager.getFocusedView() ?? TabManager.getCurrentActiveTabView().
Make sure history.ts and any other callers use the same shared function so the
fallback behavior stays consistent in one place.
In `@src/main/e2e/trayMenu.ts`:
- Around line 80-98: Add unit tests for createClickTrayMenuItem to cover all
routing branches and helper behavior. Verify the role: path succeeds and throws
when clickTrayMenuItemsByRole cannot find a role, the tray:settings path matches
the settings/preferences item through clickTraySettingsMenuItem, and the
fallback path uses clickTrayMenuItems with both exact and truncated labels. Use
the createClickTrayMenuItem, clickTrayMenuItemsByRole,
clickTraySettingsMenuItem, and clickTrayMenuItems symbols to locate the logic
and validate success/failure cases.
🪄 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: fedaaaea-86b5-4c54-80ce-69125ef4680f
📒 Files selected for processing (5)
src/app/menus/appMenu/history.test.jssrc/app/menus/appMenu/history.tssrc/app/menus/appMenu/view.test.jssrc/app/menus/appMenu/view.tssrc/main/e2e/trayMenu.ts
Use atomic badge setup and refresh polling for Windows E2E, fall back to the active tab for History/View menu actions, tighten tray role matching, and extend ESLint jest overrides to *.test.ts. Co-authored-by: Cursor <cursoragent@cursor.com>
github-actions
Bot
added
E2E/Run
devinbinnie
left a comment
devinbinnie
left a comment
There was a problem hiding this comment.
Everything looks good, just one thing we should do.
| // See LICENSE.txt for license information. | ||
|
|
||
| import type {Menu} from 'electron'; | ||
| import type {Menu, MenuItem} from 'electron'; |
There was a problem hiding this comment.
Shouldn't this file and its test be in the e2e root folder? I don't see it being called from the main code.
There was a problem hiding this comment.
@devinbinnie register.ts:61 uses it. We wire createClickTrayMenuItem(createTrayMenu) into __e2eClickTrayMenuItem.
I tried moving it locally to the e2e/ folder, but that meant changing ESLint/webpack so the main bundle could compile stuff from e2e/, which felt like extra overhead ffor just a test hook.
Should I add a short comment in the file explaining why we keep it here?
There was a problem hiding this comment.
Okay, so this is all new stuff I wasn't aware of - guess it was merged while I was away.
I don't think we need a comment, but I would like to avoid E2E specific code in the main process. I guess since this is self-contained we can leave it for now, but if there's a way to pull it out into the e2e root folder we should.
I'll approve for now but we should think about that going forward.
3 participants
Rainforest migration needs ephemeral E2E servers to allow public file links before Playwright runs, and CI needs clearer per-OS flaky test reporting.
This PR adds a pre-test script that enables
FileSettings.EnablePublicLinkwhen the cloud server has it off, wires it into the E2E workflow template, and extends the flaky-test analyzer used when posting results.It also includes the hooks slice from #3886 (tray menu test hook and history/view menu fixes) because that PR merged into this branch.
Slice of #3853 for easier review. Merge order: #3885 → #3887 → #3888 → #3889.
Release Note