Desktop qa agent - #3834
Merged
yasserfaraazkhan merged 3 commits intoMay 27, 2026
Merged
Conversation
…tion - Add syncCursorAutomationServerLine in e2e/utils/github-actions.js - Optional workflow_call input pr_number; pass from Electron Playwright Tests - Linux job runs early github-script step after checkout (continue-on-error) - Grant pull-requests:write on caller workflow and reusable e2e job Line format: Server for Cursor Automation: <url> Append if missing; replace line if URL changed; no-op if unchanged. Co-authored-by: yasser khan <attitude3cena.yf@gmail.com>
|
@yasserfaraazkhan: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsI understand the commands that are listed here |
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR improves E2E test reliability across platforms, redesigns CMT provisioning to use direct HTTP dispatch, enhances test reporting with per-OS metrics and PR integration, and adds automatic popout cleanup when the main window closes. Changes span test infrastructure, CI/CD workflows, and application lifecycle management. ChangesE2E Test Infrastructure & Workflow Improvements
CMT Provisioner Workflow Redesign
Main Window & Popout Lifecycle Management
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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: 5
🧹 Nitpick comments (3)
.github/workflows/cmt-provisioner.yml (1)
70-82: 💤 Low valueRobust HTTP request with good retry and timeout handling.
The curl configuration follows best practices with retry logic, timeouts, and
--fail-with-bodyfor debugging. The user feedback clearly explains the async provisioning workflow.Optional: Add payload logging for debugging
Consider echoing the constructed payload before sending it to help with debugging (the token is in the header, not the payload, so this is safe):
payload="$(jq -nc \ --arg owner "${OWNER}" \ --arg repo "${REPO}" \ --arg sha "${SHA}" \ --arg ref "${REF}" \ --argjson run_id "${RUN_ID}" \ --arg versions "${SERVER_VERSIONS}" \ '{owner:$owner, repo:$repo, sha:$sha, ref:$ref, run_id:$run_id, server_versions:$versions}')" + echo "Payload: ${payload}" echo "Requesting CMT provisioning for server versions: ${SERVER_VERSIONS}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/cmt-provisioner.yml around lines 70 - 82, Echo the constructed payload just before the curl POST so you can inspect what is being sent (the token is in the X-Trigger-Token header, not the payload), e.g. add an echo of the ${payload} variable before the curl command referenced in the cmt_dispatch POST; keep existing retry/timeout flags (--fail-with-body, --retry, --connect-timeout, --max-time) intact and do not print the CMT_TRIGGER_TOKEN or MATTERWICK_URL to avoid leaking secrets.src/app/windows/popoutManager.ts (1)
87-100: ⚡ Quick winRegister close cleanup immediately for already-created main windows.
The cleanup wiring currently depends on
MAIN_WINDOW_CREATEDfiring after this manager is constructed. If the window already exists, the close handler may never be attached. Add an eager registration call in the constructor as a fallback.Proposed patch
// When the main window closes (e.g., user confirms quit, or willAppQuit is // true), destroy all popout windows so they do not outlive the main window. MainWindow.on(MAIN_WINDOW_CREATED, this.registerMainWindowCloseHandler); + this.registerMainWindowCloseHandler();🤖 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/windows/popoutManager.ts` around lines 87 - 100, The constructor of PopoutManager should eagerly attach the main-window close handler in addition to listening for MAIN_WINDOW_CREATED: call the existing registerMainWindowCloseHandler from the constructor (so it will attach to an already-existing MainWindow via MainWindow.get()) and keep the existing MainWindow.on(MAIN_WINDOW_CREATED, this.registerMainWindowCloseHandler) subscription; update the constructor in popoutManager.ts to invoke registerMainWindowCloseHandler immediately to ensure closeAllPopouts is bound even when the main window was created earlier.e2e/specs/server_management/popout_windows.test.ts (1)
278-300: ⚡ Quick winReuse
clickFileMenuItemhere to avoid helper drift.This block duplicates the same File-menu lookup/target-window logic already implemented in
clickFileMenuItem, which increases maintenance risk.Suggested refactor
- await app.evaluate(({app: electronAppInstance, BrowserWindow}) => { - const fileMenu = (electronAppInstance as any).applicationMenu.getMenuItemById('file'); - const items = fileMenu?.submenu?.items ?? []; - const newWindowItem = items.find((candidate: any) => { - const candidateLabel = typeof candidate.label === 'string' ? candidate.label.trim() : ''; - return candidateLabel === 'New Window'; - }); - - if (!newWindowItem) { - throw new Error('New Window menu item not found'); - } - - const refs = (global as any).__e2eTestRefs; - const targetWindow = BrowserWindow.getFocusedWindow() ?? - refs?.MainWindow?.get?.() ?? - BrowserWindow.getAllWindows().find((w) => !w.isDestroyed()) ?? - null; - newWindowItem.click(undefined, targetWindow, undefined); - }); + await clickFileMenuItem(app, 'New Window');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/specs/server_management/popout_windows.test.ts` around lines 278 - 300, The File-menu lookup and target-window selection duplicated here should be replaced with the existing helper clickFileMenuItem to avoid drift; locate the anonymous app.evaluate block that searches applicationMenu and clicks the 'New Window' item (it constructs newWindowItem, references BrowserWindow and __e2eTestRefs and calls newWindowItem.click) and instead call the shared helper clickFileMenuItem with the label 'New Window' (or the helper's API for selecting by id/label) ensuring you await it and preserve the same test context so the target window resolution performed by clickFileMenuItem is used.
🤖 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/global-setup.ts`:
- Around line 48-63: The test setup currently calls execFileSync to write
persistent macOS defaults for com.apple.LaunchServices LSQuarantine and
com.apple.CrashReporter DialogType without restoring prior values; update
global-setup.ts to first read and store the existing values (via execFileSync
'defaults read' for LSQuarantine and DialogType) before writing, and then
restore those saved values in the teardown/cleanup path (or conditionally remove
the keys) so the changes made by the try blocks around execFileSync are
reverted; reference the existing execFileSync calls and the LSQuarantine and
DialogType keys when implementing snapshot-and-restore logic.
In `@e2e/specs/menu_bar/full_screen.test.ts`:
- Around line 81-94: The current evaluate callback silently no-ops when the
fullscreen toggle item is missing; change it to fail fast by throwing an error
if toggleItem is undefined. In the electronApp.evaluate block (the callback that
references viewMenu and computes toggleItem by checking item.role ===
'togglefullscreen' || item.accelerator === 'F11'), replace the no-op branch with
a thrown Error (including a clear message like "exit fullscreen menu item not
found") so the test fails immediately instead of timing out later.
In `@e2e/specs/notification_trigger/notification_badge_windows_linux.test.ts`:
- Around line 74-99: The loop waiting for the badge-reset hook may exit due to
timeout without actually resetting the badge, so change the logic in the block
that uses deadline/while Date.now() to detect if the reset completed and fail
fast if not: track a local boolean (e.g., resetDone) that is set to true
immediately after successfully calling (global as
any).__testTriggerSetUnreadBadgeSetting(false) and clearing (global as
any).__testBadgeState, and after the loop finishes check that flag and throw a
descriptive Error (including the deadline or elapsed time) if resetDone is false
so the test fails immediately instead of leaking state across tests; update
references to the existing deadline, electronApp.evaluate calls, and the
__testTriggerSetUnreadBadgeSetting / __testBadgeState symbols to implement this.
In `@e2e/utils/analyze-flaky-test.js`:
- Around line 54-68: The current logic only skips counting failures when the
failing entry itself has a "(retry `#n`)" suffix; update the filter so that for
non-retry entries (when retryMatch is null) you also look for any passing retry
with the same base name. Specifically, keep the existing check for retryMatch
and hasPassingRetry, and add the inverse check: when retryMatch is null, compute
baseName = name and set hasPassingRetry = cases.some(c => c.name.match(new
RegExp(`^${escapeRegex(baseName)} \\(retry #\\d+\\)$`)) && c.failure ===
undefined && c.error === undefined), then return false if hasPassingRetry; use
the existing variables (name, retryMatch, baseName, hasPassingRetry, cases) to
implement this.
In `@src/main/app/intercom.ts`:
- Around line 108-118: The polling interval created in pollInterval can leak if
the window never becomes visible; update the logic around MainWindow.get(),
pollInterval, markReady, and done to ensure explicit teardown by (1) adding a
max timeout (e.g., setTimeout fallback) that clears pollInterval and marks done
when reached, and (2) registering listeners on the MainWindow instance (e.g.,
its 'close'/'closed' event) and app lifecycle (e.g., before-quit/quit) to
clearInterval(pollInterval) and remove those listeners; ensure any path that
stops polling sets done and clears the interval so the timer is never left
running.
---
Nitpick comments:
In @.github/workflows/cmt-provisioner.yml:
- Around line 70-82: Echo the constructed payload just before the curl POST so
you can inspect what is being sent (the token is in the X-Trigger-Token header,
not the payload), e.g. add an echo of the ${payload} variable before the curl
command referenced in the cmt_dispatch POST; keep existing retry/timeout flags
(--fail-with-body, --retry, --connect-timeout, --max-time) intact and do not
print the CMT_TRIGGER_TOKEN or MATTERWICK_URL to avoid leaking secrets.
In `@e2e/specs/server_management/popout_windows.test.ts`:
- Around line 278-300: The File-menu lookup and target-window selection
duplicated here should be replaced with the existing helper clickFileMenuItem to
avoid drift; locate the anonymous app.evaluate block that searches
applicationMenu and clicks the 'New Window' item (it constructs newWindowItem,
references BrowserWindow and __e2eTestRefs and calls newWindowItem.click) and
instead call the shared helper clickFileMenuItem with the label 'New Window' (or
the helper's API for selecting by id/label) ensuring you await it and preserve
the same test context so the target window resolution performed by
clickFileMenuItem is used.
In `@src/app/windows/popoutManager.ts`:
- Around line 87-100: The constructor of PopoutManager should eagerly attach the
main-window close handler in addition to listening for MAIN_WINDOW_CREATED: call
the existing registerMainWindowCloseHandler from the constructor (so it will
attach to an already-existing MainWindow via MainWindow.get()) and keep the
existing MainWindow.on(MAIN_WINDOW_CREATED, this.registerMainWindowCloseHandler)
subscription; update the constructor in popoutManager.ts to invoke
registerMainWindowCloseHandler immediately to ensure closeAllPopouts is bound
even when the main window was created earlier.
🪄 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: 728aae57-b4ce-4892-a929-cb49f0cf7038
📒 Files selected for processing (26)
.github/workflows/cmt-provisioner.yml.github/workflows/e2e-functional-template.yml.github/workflows/e2e-functional.yml.github/workflows/e2e-nightly-trigger.ymle2e/fixtures/index.tse2e/global-setup.tse2e/global-teardown.tse2e/helpers/appReadiness.tse2e/merge.playwright.config.tse2e/playwright.config.tse2e/specs/deep_linking/deeplink.test.tse2e/specs/mattermost/copy_link.test.tse2e/specs/menu_bar/full_screen.test.tse2e/specs/menu_bar/window_menu.test.tse2e/specs/notification_trigger/notification_badge_windows_linux.test.tse2e/specs/server_management/bad_servers.test.tse2e/specs/server_management/popout_windows.test.tse2e/specs/server_management/remove_server_modal.test.tse2e/specs/startup/window.test.tse2e/specs/system/tray_restore.test.tse2e/utils/analyze-flaky-test.jse2e/utils/github-actions.jssrc/app/windows/popoutManager.test.jssrc/app/windows/popoutManager.tssrc/main/app/intercom.test.jssrc/main/app/intercom.ts
💤 Files with no reviewable changes (1)
- e2e/merge.playwright.config.ts
Comment on lines
+81
to
94
| await electronApp.evaluate(({app, BrowserWindow}) => { | ||
| const viewMenu = (app as any).applicationMenu?.getMenuItemById('view'); | ||
| const toggleItem = viewMenu?.submenu?.items?.find( | ||
| (item: any) => item.role === 'togglefullscreen' || item.accelerator === 'F11', | ||
| ); | ||
| toggleItem?.click(); | ||
| if (toggleItem) { | ||
| const refs = (global as any).__e2eTestRefs; | ||
| const targetWindow = BrowserWindow.getFocusedWindow() ?? | ||
| refs?.MainWindow?.get?.() ?? | ||
| BrowserWindow.getAllWindows().find((w) => !w.isDestroyed()) ?? | ||
| null; | ||
| toggleItem.click(undefined, targetWindow, undefined); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
Fail fast if the exit fullscreen menu item is missing
The current no-op branch turns a clear setup failure into a later timeout. Mirror the earlier guard and throw immediately when the item is unavailable.
Suggested fix
- if (toggleItem) {
- const refs = (global as any).__e2eTestRefs;
- const targetWindow = BrowserWindow.getFocusedWindow() ??
- refs?.MainWindow?.get?.() ??
- BrowserWindow.getAllWindows().find((w) => !w.isDestroyed()) ??
- null;
- toggleItem.click(undefined, targetWindow, undefined);
- }
+ if (!toggleItem) {
+ throw new Error('Toggle Full Screen menu item not found');
+ }
+ const refs = (global as any).__e2eTestRefs;
+ const targetWindow = BrowserWindow.getFocusedWindow() ??
+ refs?.MainWindow?.get?.() ??
+ BrowserWindow.getAllWindows().find((w) => !w.isDestroyed()) ??
+ null;
+ toggleItem.click(undefined, targetWindow, undefined);🤖 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/menu_bar/full_screen.test.ts` around lines 81 - 94, The current
evaluate callback silently no-ops when the fullscreen toggle item is missing;
change it to fail fast by throwing an error if toggleItem is undefined. In the
electronApp.evaluate block (the callback that references viewMenu and computes
toggleItem by checking item.role === 'togglefullscreen' || item.accelerator ===
'F11'), replace the no-op branch with a thrown Error (including a clear message
like "exit fullscreen menu item not found") so the test fails immediately
instead of timing out later.
Comment on lines
74
to
99
| const deadline = Date.now() + 10_000; | ||
| while (Date.now() < deadline) { | ||
| try { | ||
| const isReady = await electronApp.evaluate( | ||
| () => typeof (global as any).__testTriggerSetUnreadBadgeSetting === 'function', | ||
| ); | ||
| if (!isReady) { | ||
| await new Promise((resolve) => setTimeout(resolve, 200)); | ||
| continue; | ||
| } | ||
| await electronApp.evaluate(() => { | ||
| (global as any).__testTriggerSetUnreadBadgeSetting(false); | ||
| }); | ||
| await electronApp.evaluate(() => { | ||
| (global as any).__testBadgeState = null; | ||
| }); | ||
| break; | ||
| } catch (err) { | ||
| const msg = err instanceof Error ? err.message : String(err); | ||
| if (!msg.includes('Execution context was destroyed') && !msg.includes('Unable to find context')) { | ||
| throw err; | ||
| } | ||
| await new Promise((resolve) => setTimeout(resolve, 200)); | ||
| } | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
Fail fast when badge reset hook never becomes ready.
The loop can time out and continue without resetting badge settings/state, which can leak state across tests and create nondeterministic failures.
Suggested fix
test.beforeEach(async ({electronApp}) => {
// Poll for the badge-setting hook to be registered before calling it —
// using optional chaining (?.) would silently succeed (no-op) before
// setup completes and leave the setting unreset between tests.
const deadline = Date.now() + 10_000;
+ let resetApplied = false;
while (Date.now() < deadline) {
try {
const isReady = await electronApp.evaluate(
() => typeof (global as any).__testTriggerSetUnreadBadgeSetting === 'function',
);
@@
await electronApp.evaluate(() => {
(global as any).__testBadgeState = null;
});
- break;
+ resetApplied = true;
+ break;
} catch (err) {
const msg = err instanceof Error ? err.message : String(err);
if (!msg.includes('Execution context was destroyed') && !msg.includes('Unable to find context')) {
throw err;
}
await new Promise((resolve) => setTimeout(resolve, 200));
}
}
+ if (!resetApplied) {
+ throw new Error('Timed out waiting for __testTriggerSetUnreadBadgeSetting to be registered');
+ }
});🤖 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/notification_badge_windows_linux.test.ts`
around lines 74 - 99, The loop waiting for the badge-reset hook may exit due to
timeout without actually resetting the badge, so change the logic in the block
that uses deadline/while Date.now() to detect if the reset completed and fail
fast if not: track a local boolean (e.g., resetDone) that is set to true
immediately after successfully calling (global as
any).__testTriggerSetUnreadBadgeSetting(false) and clearing (global as
any).__testBadgeState, and after the loop finishes check that flag and throw a
descriptive Error (including the deadline or elapsed time) if resetDone is false
so the test fails immediately instead of leaking state across tests; update
references to the existing deadline, electronApp.evaluate calls, and the
__testTriggerSetUnreadBadgeSetting / __testBadgeState symbols to implement this.
Comment on lines
+54
to
+68
| // If this test name ends with a retry suffix like " (retry #1)", | ||
| // and the base test (without suffix) also appears as a passing case, | ||
| // this failure was retried and resolved — don't count it. | ||
| const name = testcase.name || ''; | ||
| const retryMatch = name.match(/^(.*) \(retry #\d+\)$/); | ||
| if (retryMatch) { | ||
| const baseName = retryMatch[1]; | ||
| const hasPassingRetry = cases.some( | ||
| (c) => c.name === baseName && c.failure === undefined && c.error === undefined, | ||
| ); | ||
| if (hasPassingRetry) { | ||
| return false; | ||
| } | ||
| } | ||
| return true; |
Contributor
There was a problem hiding this comment.
Retry-pass detection doesn't cover original failures.
When an original test (without (retry #N) suffix) fails but a retry passes, this filter doesn't exclude it. The check at lines 57-67 only applies to entries WITH the retry suffix, so original failures are always counted even if a subsequent retry passed.
Example: TestA fails, TestA (retry #1) passes → TestA is still counted as a failure because retryMatch is null for names without the suffix.
Consider also checking, for non-retry entries, whether any retry with the same base name passed:
Proposed fix
const name = testcase.name || '';
const retryMatch = name.match(/^(.*) \(retry #\d+\)$/);
if (retryMatch) {
const baseName = retryMatch[1];
const hasPassingRetry = cases.some(
(c) => c.name === baseName && c.failure === undefined && c.error === undefined,
);
if (hasPassingRetry) {
return false;
}
- }
+ } else {
+ // For original (non-retry) failures, check if any retry passed
+ const hasPassingRetry = cases.some(
+ (c) =>
+ c.name?.startsWith(`${name} (retry #`) &&
+ c.failure === undefined &&
+ c.error === undefined,
+ );
+ if (hasPassingRetry) {
+ return false;
+ }
+ }
return true;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // If this test name ends with a retry suffix like " (retry #1)", | |
| // and the base test (without suffix) also appears as a passing case, | |
| // this failure was retried and resolved — don't count it. | |
| const name = testcase.name || ''; | |
| const retryMatch = name.match(/^(.*) \(retry #\d+\)$/); | |
| if (retryMatch) { | |
| const baseName = retryMatch[1]; | |
| const hasPassingRetry = cases.some( | |
| (c) => c.name === baseName && c.failure === undefined && c.error === undefined, | |
| ); | |
| if (hasPassingRetry) { | |
| return false; | |
| } | |
| } | |
| return true; | |
| const name = testcase.name || ''; | |
| const retryMatch = name.match(/^(.*) \(retry #\d+\)$/); | |
| if (retryMatch) { | |
| const baseName = retryMatch[1]; | |
| const hasPassingRetry = cases.some( | |
| (c) => c.name === baseName && c.failure === undefined && c.error === undefined, | |
| ); | |
| if (hasPassingRetry) { | |
| return false; | |
| } | |
| } else { | |
| // For original (non-retry) failures, check if any retry passed | |
| const hasPassingRetry = cases.some( | |
| (c) => | |
| c.name?.startsWith(`${name} (retry #`) && | |
| c.failure === undefined && | |
| c.error === undefined, | |
| ); | |
| if (hasPassingRetry) { | |
| return false; | |
| } | |
| } | |
| return true; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/utils/analyze-flaky-test.js` around lines 54 - 68, The current logic only
skips counting failures when the failing entry itself has a "(retry `#n`)" suffix;
update the filter so that for non-retry entries (when retryMatch is null) you
also look for any passing retry with the same base name. Specifically, keep the
existing check for retryMatch and hasPassingRetry, and add the inverse check:
when retryMatch is null, compute baseName = name and set hasPassingRetry =
cases.some(c => c.name.match(new RegExp(`^${escapeRegex(baseName)} \\(retry
#\\d+\\)$`)) && c.failure === undefined && c.error === undefined), then return
false if hasPassingRetry; use the existing variables (name, retryMatch,
baseName, hasPassingRetry, cases) to implement this.
Comment on lines
+108
to
+118
| const pollInterval = setInterval(() => { | ||
| if (done) { | ||
| clearInterval(pollInterval); | ||
| return; | ||
| } | ||
| const mw = MainWindow.get(); | ||
| if (mw?.isVisible()) { | ||
| clearInterval(pollInterval); | ||
| markReady(true); | ||
| } | ||
| }, 250); |
Contributor
There was a problem hiding this comment.
Bound the polling interval lifecycle to avoid leaked timers.
This interval can run forever when the window never becomes visible and no show event arrives. Please add explicit teardown (e.g., on window close/app quit and/or max timeout) to prevent timer leaks in the main process.
Proposed patch
const pollInterval = setInterval(() => {
if (done) {
clearInterval(pollInterval);
return;
}
const mw = MainWindow.get();
if (mw?.isVisible()) {
clearInterval(pollInterval);
markReady(true);
}
}, 250);
+ mainWindow.once('closed', () => clearInterval(pollInterval));🤖 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/app/intercom.ts` around lines 108 - 118, The polling interval
created in pollInterval can leak if the window never becomes visible; update the
logic around MainWindow.get(), pollInterval, markReady, and done to ensure
explicit teardown by (1) adding a max timeout (e.g., setTimeout fallback) that
clears pollInterval and marks done when reached, and (2) registering listeners
on the MainWindow instance (e.g., its 'close'/'closed' event) and app lifecycle
(e.g., before-quit/quit) to clearInterval(pollInterval) and remove those
listeners; ensure any path that stops polling sets done and clears the interval
so the timer is never left running.
When MM_TEST_SERVER_URL is unset, global setup fetches the PR body and reads the Server for Cursor Automation URL (GitHub API or gh CLI). Document the split: URL from PR, credentials from Cursor env. Also snapshot/restore macOS LaunchServices/CrashReporter defaults around E2E so global-setup changes pair with global-teardown cleanup. Co-authored-by: yasser khan <attitude3cena.yf@gmail.com>
github-actions
Bot
added
E2E/Run
Documentation Impact Analysis — updates neededDocumentation Impact AnalysisOverall Assessment: Documentation Updates Recommended Changes SummaryThis PR adds automatic popout window cleanup functionality to the Mattermost desktop app. When the main window is closed, all popout windows are now automatically destroyed rather than remaining open independently. Documentation Impact Details
Recommended Actions
ConfidenceHigh — The popout window functionality is already documented in the end-user guide, and the PR clearly adds new user-visible behavior (automatic cleanup of popout windows on main window close) that would help users understand what to expect when quitting the application with open popout windows. |
yasserfaraazkhan
changed the base branch from
master
to
fix/cmt-direct-dispatch-and-cleanup-endpoint
May 27, 2026 02:35
Contributor
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/helpers/resolveMmTestServerUrlFromPr.ts`:
- Around line 83-88: The GH CLI call in fetchPrBodyFromGhCli currently invokes
`gh pr view <pr>` without scoping to the resolved repository, which can return
the wrong PR body in detached checkouts; update the execFileSync args in
`fetchPrBodyFromGhCli` to include `-R` (or `--repo`) with the resolved
`ownerRepo` when `ownerRepo` is available (i.e., prepend/insert `-R, ownerRepo`
into the arguments array passed to `execFileSync`), ensuring the function uses
the `ownerRepo` variable to target the correct repository.
🪄 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: 738dcd58-0855-4caf-ac55-6a9479ef0c66
📒 Files selected for processing (6)
AGENTS.mde2e/AGENTS.mde2e/global-setup.tse2e/global-teardown.tse2e/helpers/resolveMmTestServerUrlFromPr.tse2e/utils/github-actions.js
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/utils/github-actions.js
Comment on lines
+83
to
+88
| function fetchPrBodyFromGhCli(prNumber: number): string | null { | ||
| try { | ||
| const out = execFileSync( | ||
| 'gh', | ||
| ['pr', 'view', String(prNumber), '--json', 'body', '-q', '.body'], | ||
| {encoding: 'utf8', stdio: ['pipe', 'pipe', 'pipe'], maxBuffer: 10 * 1024 * 1024}, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify gh supports explicit repo targeting and confirm current call omits it.
gh pr view --help | rg -n -- '-R|--repo'
rg -n -C2 'fetchPrBodyFromGhCli|gh[[:space:]]*[,)]|--json|\\.body' e2e/helpers/resolveMmTestServerUrlFromPr.tsRepository: mattermost/desktop
Length of output: 872
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="e2e/helpers/resolveMmTestServerUrlFromPr.ts"
nl -ba "$file" | sed -n '1,220p'Repository: mattermost/desktop
Length of output: 105
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="e2e/helpers/resolveMmTestServerUrlFromPr.ts"
# Narrow search for how repo is derived / passed
rg -n "ownerRepo|GITHUB_REPOSITORY|repository|owner\\/repo|--repo|-R" "$file" || trueRepository: mattermost/desktop
Length of output: 233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="e2e/helpers/resolveMmTestServerUrlFromPr.ts"
cat -n "$file" | sed -n '1,220p'Repository: mattermost/desktop
Length of output: 5899
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="e2e/helpers/resolveMmTestServerUrlFromPr.ts"
rg -n "ownerRepo|GITHUB_REPOSITORY|repository|owner/repo|--repo|-R" "$file" || trueRepository: mattermost/desktop
Length of output: 530
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "fetchPrBodyFromGhCli\\(" -S . || trueRepository: mattermost/desktop
Length of output: 273
Scope gh pr view to the resolved repository
Fallback CLI lookup ignores the already-resolved ownerRepo and runs gh pr view <n> without -R/--repo, so it can read the wrong PR body (or fail) in detached/non-standard checkouts even when ownerRepo is known (e.g., at lines 113-120 with fetchPrBodyFromGhCli at 83-88). Add -R <owner>/<repo> when ownerRepo is available.
💡 Proposed fix
-function fetchPrBodyFromGhCli(prNumber: number): string | null {
+function fetchPrBodyFromGhCli(
+ prNumber: number,
+ ownerRepo?: {owner: string; repo: string},
+): string | null {
try {
+ const args = ['pr', 'view', String(prNumber), '--json', 'body', '-q', '.body'];
+ if (ownerRepo) {
+ args.push('-R', `${ownerRepo.owner}/${ownerRepo.repo}`);
+ }
const out = execFileSync(
'gh',
- ['pr', 'view', String(prNumber), '--json', 'body', '-q', '.body'],
+ args,
{encoding: 'utf8', stdio: ['pipe', 'pipe', 'pipe'], maxBuffer: 10 * 1024 * 1024},
);
const body = out.trim();
return body.length > 0 ? body : null;
} catch {
return null;
}
}
@@
- if (!body) {
- body = fetchPrBodyFromGhCli(prNumber);
+ if (!body) {
+ body = fetchPrBodyFromGhCli(prNumber, ownerRepo ?? undefined);
}🤖 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/resolveMmTestServerUrlFromPr.ts` around lines 83 - 88, The GH CLI
call in fetchPrBodyFromGhCli currently invokes `gh pr view <pr>` without scoping
to the resolved repository, which can return the wrong PR body in detached
checkouts; update the execFileSync args in `fetchPrBodyFromGhCli` to include
`-R` (or `--repo`) with the resolved `ownerRepo` when `ownerRepo` is available
(i.e., prepend/insert `-R, ownerRepo` into the arguments array passed to
`execFileSync`), ensuring the function uses the `ownerRepo` variable to target
the correct repository.
- full_screen: throw if exit-fullscreen menu item missing (fail fast) - notification_badge_windows_linux: require badge reset beforeEach to complete - analyze-flaky-test: ignore base failures when a passing retry exists - intercom: cap visibility poll, clear interval on timeout/close/quit - PopoutManager: register main-window close handler on construction - popout_windows: reuse clickFileMenuItem for New Window - cmt-provisioner: echo JSON payload before curl (no secrets in body) - popoutManager.test: satisfy no-new/no-void for side-effect constructor Co-authored-by: yasser khan <attitude3cena.yf@gmail.com>
yasserfaraazkhan
merged commit May 27, 2026
9d891fb
into
fix/cmt-direct-dispatch-and-cleanup-endpoint
6 of 9 checks passed
This was referenced Jun 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request contains changes generated by a Cursor Cloud Agent
Change Impact: 🟡 Medium
Regression Risk: The PR touches core startup and window-management flows (intercom.ts, PopoutManager) and broad E2E infrastructure (workflows, test fixtures, timeouts, and platform-specific defaults). These changes introduce new event dependencies, guarded control flow, polling fallbacks, and macOS defaults manipulation — increasing the chance of platform-specific regressions and behavioral shifts across multiple modules. Several widely-used utilities and CI workflows were modified, raising the potential blast radius beyond a single component.
QA Recommendation: Require targeted manual QA in addition to automated tests: validate app startup and onboarding readiness (esp. macOS/Windows), popout lifecycle cleanup when main window closes, fullscreen and window-menu interactions across platforms, notification badge behavior on Linux/Windows, and confirm macOS defaults/dialog-suppression do not cause side effects in CI or local runs. Verify E2E reporting/CI workflows behave as expected after workflow/template changes and that window restoration/positioning behaves correctly across displays.
Generated by CodeRabbitAI
Server for Cursor Automation: https://desktop-pr-3834-linux-fyt33y6r.test.mattermost.cloud