You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
CMT Provisioner runs on desktop succeed in GitHub Actions but never trigger the follow-up compatibility-matrix-testing.yml workflow. The two most recent manual CMT runs both showed this:
run 25040877733 — 2026-04-28, succeeded, no matrix follow-up
run 26026866029 — 2026-05-18, succeeded, no matrix follow-up
Matterwick was receiving the workflow_run.requested webhook (verified, 202 OK), but it was never dispatching the test workflow.
Root cause
The previous design relied on matterwick reading the server_versions workflow_dispatch input out of the workflow_run.requested webhook payload. GitHub does not include workflow_dispatch inputs on the workflow_run object in webhook payloads — verified against the Octokit webhook schema (35 fields, no inputs), the actual REST API for run 26026866029, and the official GitHub docs.
So matterwick received an empty input map, bailed out at if !ok || serverVersionsStr == "", and never called dispatchCMTWorkflow.
Fix (workflow side)
Replace the echo-only stub with a direct curl POST to matterwick's new /cmt_dispatch endpoint, sending the full context the webhook payload cannot carry:
Matterwick returns 202 Accepted immediately and provisions instances + dispatches compatibility-matrix-testing.yml asynchronously in a goroutine (~30 min provisioning window, same as before).
The workflow also fails fast with a clear error if vars.MATTERWICK_URL or secrets.MATTERWICK_CMT_TRIGGER_SECRET is missing on the repo.
mobile workflow PR (planned, will be raised after this one)
gitops-platform secret PR (planned, adds CMTTriggerSecret to matterwick config)
Rollout order
This PR depends on:
mattermost/matterwick#90 merged + deployed
gitops-platform PR merged with real (non-placeholder) tokens
MATTERWICK_CMT_TRIGGER_SECRET added to this repo's GitHub Actions secrets (the value matches the CMTTriggerSecret set in step 2)
MATTERWICK_CLEANUP_SECRET (already present) rotated to match the new CleanupSecret in gitops, if rotated
Only after all four are in place is this PR safe to merge. Until then, dispatching CMT Provisioner will fail loudly with the explicit pre-flight error.
What this does NOT touch
E2E PR-label flow (unchanged, working today)
Nightly schedule flow (separate path, no inputs needed)
Release-* / master push flows (separate path)
Testing
✅ YAML validates with yaml.safe_load
✅ Pre-flight checks added for both MATTERWICK_URL and MATTERWICK_CMT_TRIGGER_SECRET (fail with explicit error message)
✅ --fail-with-body --retry 3 ensures the workflow step fails loudly if matterwick is unreachable or returns non-2xx
Smoke test plan: dispatch CMT Provisioner with v11.1.0, v11.2.0 → confirm matterwick log shows endpoint=/cmt_dispatch Accepted CMT dispatch request → wait ~30 min → confirm compatibility-matrix-testing.yml dispatched
Test plan
Reviewer reads new .github/workflows/cmt-provisioner.yml
Confirm vars.MATTERWICK_URL is set on repo (already used by existing cleanup curl)
Add MATTERWICK_CMT_TRIGGER_SECRET to repo secrets (synced with gitops CMTTriggerSecret)
After matterwick deploys with the new endpoint, dispatch CMT Provisioner manually
Verify compatibility-matrix-testing.yml fires ~30 min later with correct CMT_MATRIX
Regression Risk:
The largest risk is CI/dispatch reliability: replacing a webhook-driven stub with a direct POST to Matterwick adds a runtime dependency (MATTERWICK_URL, MATTERWICK_CMT_TRIGGER_SECRET) and requires Matterwick deployment (mattermost/matterwick#90). Although pre-flight variable/secret checks and curl retry/fail flags reduce silent failures, network issues, misconfiguration, or deployment sequencing can cause workflow failures or missed dispatches. The changes also modify multiple CI workflows and E2E test infrastructure (moderate blast radius). Application code changes are limited and covered by added unit tests (popout cleanup, intercom), lowering regression risk for runtime behavior.
QA Recommendation:
Perform targeted manual validation prior to merge: ensure matterwick endpoint is deployed and required secrets/vars are configured, run an end-to-end provisioner execution to confirm Matterwick returns 202 and dispatches compatibility-matrix-testing.yml (~30 minutes). Verify pre-flight checks and that curl retry/fail behavior surfaces clear diagnostics on failures. Run affected E2E suites and new/updated unit tests to validate test infra, popout cleanup, and related behaviors.
…run webhook
GitHub's workflow_run webhook payload does not include workflow_dispatch
inputs, so matterwick was never receiving the server_versions value and
the CMT Provisioner runs (e.g. 2026-04-28, 2026-05-18) returned 202 OK
on the webhook but never dispatched compatibility-matrix-testing.yml.
Replace the echo-only stub with a curl POST to matterwick's new
/cmt_dispatch endpoint, sending the full context the webhook payload
cannot carry: server_versions, run_id, sha, ref, owner, repo. Matterwick
returns 202 immediately and runs provisioning asynchronously.
Requires two repository configuration items:
- vars.MATTERWICK_URL (already set, used by /cleanup_e2e callback)
- secrets.MATTERWICK_CMT_TRIGGER_SECRET (new, shared with matterwick)
Co-Authored-By: Claude <noreply@anthropic.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.
It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.
Use the following commands to manage reviews:
@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.
Use the checkboxes below for quick actions:
▶️ Resume reviews
🔍 Trigger review
📝 Walkthrough
Walkthrough
Adds a Matterwick direct-dispatch GitHub Actions job, makes nightly E2E schedule-only, and applies widespread E2E robustness changes: macOS dialog suppression, increased timeouts, teardown tweaks, improved flaky-test analysis/reporting, many test reliability fixes, and popout/main-window lifecycle cleanup.
Changes
Matterwick CMT Dispatch and E2E infra
Layer / File(s)
Summary
Workflow intent and docs .github/workflows/cmt-provisioner.yml, .github/workflows/e2e-nightly-trigger.yml
Workflow header/documentation describes the new direct-dispatch approach to Matterwick and makes the nightly E2E workflow schedule-only.
Adds trigger-matterwick job (replacing the stub): sets permissions: {}, validates MATTERWICK_URL and CMT_TRIGGER_TOKEN, constructs JSON payload with jq, and sends authenticated POST to ${MATTERWICK_URL}/cmt_dispatch using curl with retries, timeout, and logging.
Adds per-OS PASSED/TOTAL/SKIPPED workflow/job outputs, HTML report generation and S3 upload steps, and wires analyze-flaky-tests outputs into job/workflow outputs.
Removes mergedReportUrl usage, pins Windows runner to windows-2022, adds macOS suppress step, and switches status description to use formatStatusDescription.
Global setup & teardown (macOS) e2e/global-setup.ts, e2e/global-teardown.ts
Switch to execFileSync, snapshot and restore macOS defaults, apply non-fatal defaults writes across bundle IDs, increase macOS graceful-exit wait, and skip SIGKILL on darwin.
Rewrites failure counting to collapse retries into base outcomes, traverse suites for authoritative failureCount, and reconcile pass/skip/total with failures.
Deep-link and Copy Link tests e2e/specs/deep_linking/deeplink.test.ts, e2e/specs/mattermost/copy_link.test.ts
Windows deep-link uses mattermost-dev:// and polls server map; copy-link dispatches explicit contextmenu and broadens selector matching.
Menu bar fullscreen & window-minimize tests e2e/specs/menu_bar/full_screen.test.ts, e2e/specs/menu_bar/window_menu.test.ts
Fullscreen uses native menu click targeting a specific BrowserWindow and event-driven enter/leave waits; minimize test calls BrowserWindow.minimize() and polls isMinimized().
Adds polling/retries for triggerBadge and setup hooks to handle transient Electron evaluation/context errors.
Server management & popouts e2e/specs/server_management/*
Reloads all server views after renderer mounts; openPopoutWindow snapshots windows and polls for popout.html; uses native menu and sets willAppQuit before main-window close for cleanup.
Defensive test file reads e2e/specs/server_management/remove_server_modal.test.ts
Wraps readFileSync/JSON.parse in try/catch during polling to tolerate transient file states.
Registers MAIN_WINDOW_CREATED handler and once('closed') wiring to destroy tracked popouts when main window closes; adds unit and integration-style tests for cleanup.
Centralizes readiness handling with attachWindowListeners (show-event + 250ms polling fallback) and a markReady guard; expands tests with a makeWindow helper and scenarios for visible/show/polling/no-main-window.
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
mattermost/desktop#3834: Shares overlapping changes to cmt-provisioner direct /cmt_dispatch flow, Cursor automation PR-body parsing, and related E2E workflow/helper updates.
Suggested labels
github_actions
Suggested reviewers
devinbinnie
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
Check name
Status
Explanation
Resolution
Docstring Coverage
⚠️ Warning
Docstring coverage is 32.35% which is insufficient. The required threshold is 80.00%.
Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name
Status
Explanation
Description Check
✅ Passed
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
✅ Passed
The title clearly and specifically describes the main change: replacing the stub workflow_run webhook approach with direct POST requests to Matterwick, which is the primary fix driving this large, multi-file changeset.
Linked Issues check
✅ Passed
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
✅ Passed
Check skipped because no linked issues were found for this pull request.
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches📝 Generate docstrings
Create stacked PR
Commit on current branch
🧪 Generate unit tests (beta)
Create PR with unit tests
Commit unit tests in branch fix/cmt-direct-dispatch-and-cleanup-endpoint
Comment @coderabbitai help to get the list of available commands and usage tips.
The reason will be displayed to describe this comment to others. Learn more.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/cmt-provisioner.yml (1)
21-31: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Set explicit least-privilege permissions for this workflow.
No permissions block is defined, so this workflow inherits repository defaults for GITHUB_TOKEN, which is broader than needed here.
🔐 Proposed fix
name: CMT Provisioner
+permissions: {}
on:
workflow_dispatch:
🤖 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 21 - 31, Add an explicit
top-level permissions block to this workflow to avoid inheriting broad
repository defaults; update the workflow YAML (where the on:, workflow_dispatch
and job trigger-matterwick are defined) to include a minimal permissions map
such as setting permissions: contents: read and workflows: write (adjust further
only if the trigger-matterwick job needs additional scopes), so the GITHUB_TOKEN
only has the least-privilege required.
🤖 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.
Outside diff comments:
In @.github/workflows/cmt-provisioner.yml:
- Around line 21-31: Add an explicit top-level permissions block to this
workflow to avoid inheriting broad repository defaults; update the workflow YAML
(where the on:, workflow_dispatch and job trigger-matterwick are defined) to
include a minimal permissions map such as setting permissions: contents: read
and workflows: write (adjust further only if the trigger-matterwick job needs
additional scopes), so the GITHUB_TOKEN only has the least-privilege required.
ℹ️ Review info⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 33b71075-cd7f-4654-978e-a49a6b70c36d
📥 Commits
Reviewing files that changed from the base of the PR and between e699409 and fe9514e.
The workflow inherits the repo default GITHUB_TOKEN permissions, which
is broader than this workflow needs. The single step is an outbound
curl to matterwick authenticated by a separate shared secret -- there
is no checkout, no GitHub API call, and no interaction with workflow
files. permissions: {} (no scopes) is the correct least-privilege
setting; if a future step needs a specific scope, it should be added
explicitly rather than relying on repo-default inheritance.
Co-Authored-By: Claude <noreply@anthropic.com>
The reason will be displayed to describe this comment to others. Learn more.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/cmt-provisioner.yml (1)
61-68: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
Quote ${RUN_ID} in the jq command.
Line 66 uses --argjson run_id ${RUN_ID} without quoting ${RUN_ID}. While github.run_id should always be numeric, unquoted shell expansions can cause issues if the value is empty, contains whitespace, or has special characters. Quote it for safety.
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 61 - 68, The jq payload
construction is vulnerable to unquoted shell expansion for RUN_ID; update the
payload assignment so the jq invocation quotes the expansion for run id (the
line using --argjson run_id ${RUN_ID}) to --argjson run_id "${RUN_ID}" (i.e.,
modify the jq command in the payload assignment to quote ${RUN_ID}) to prevent
word-splitting or empty-value issues while keeping the --argjson usage.
🤖 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.
Outside diff comments:
In @.github/workflows/cmt-provisioner.yml:
- Around line 61-68: The jq payload construction is vulnerable to unquoted shell
expansion for RUN_ID; update the payload assignment so the jq invocation quotes
the expansion for run id (the line using --argjson run_id ${RUN_ID}) to
--argjson run_id "${RUN_ID}" (i.e., modify the jq command in the payload
assignment to quote ${RUN_ID}) to prevent word-splitting or empty-value issues
while keeping the --argjson usage.
ℹ️ Review info⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 087195d9-68cf-4e38-852e-8395a205a823
📥 Commits
Reviewing files that changed from the base of the PR and between fe9514e and c41490b.
Without quotes, an empty RUN_ID (e.g. GHA context expansion failing) would
collapse the --argjson token list and silently produce a malformed payload
or be consumed by the next --arg. Quoting forces jq to receive an empty
string and fail immediately with "invalid JSON text passed to --argjson",
which is what we want — fail loudly, don't ship a bad request.
Co-Authored-By: Claude <noreply@anthropic.com>
This trigger workflow had both a schedule (Thu/Fri 00:00 UTC) and a
push trigger on master + release-*. The push trigger was redundant
because matterwick's push-event handler also fires for master and
release-* pushes (E2EAutoTriggerOnMaster / E2EAutoTriggerOnRelease in
the deployed config). The result: every push to master or release-*
produced two concurrent matterwick paths -- direct push-event handler
(tracking key {repo}-push-{branch}-{sha}) AND workflow_run on this
trigger workflow (tracking key {repo}-scheduled-{runID}-{sha}) --
which provisioned 6 cloud instances instead of 3 and dispatched
e2e-functional.yml twice per commit.
Tracking keys differ, so the two paths don't collide in matterwick
state, but they race on commit status updates and waste cloud quota.
Leaving only the schedule trigger here makes matterwick's push handler
the single canonical driver for push-triggered E2E.
Co-Authored-By: Claude <noreply@anthropic.com>
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick comments (2)
e2e/global-setup.ts (2)
48-49: ⚡ Quick win
Misleading comment.
The comment states "Verify at least one bundle ID got the settings applied" but no verification code follows. The subsequent lines (52-63) apply additional settings to system-level domains (LaunchServices, CrashReporter) rather than verifying the bundle-specific settings.
📝 Clarify the comment
- // Verify at least one bundle ID got the settings applied.- // Also suppress the "verification of developer" dialog that can appear+ // Suppress the "verification of developer" dialog that can appear
🤖 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/global-setup.ts` around lines 48 - 49, The comment claiming "Verify at
least one bundle ID got the settings applied" is misleading — update the comment
above the subsequent settings block to accurately describe what the code does
(apply additional system-level settings and suppress the "verification of
developer" dialog) and remove the mention of bundle-ID verification; reference
the system domains mentioned (LaunchServices, CrashReporter) in the new comment
so it correctly documents that the following lines adjust system-level domains
rather than performing any bundle-specific verification.
30-64: 💤 Low value
Consider using execFileSync for security.
The current implementation uses execSync with string interpolation of bundleID. While the current code uses a hard-coded array, using execFileSync with argument arrays would be more robust against potential future changes and aligns with security best practices.
🔒 Proposed refactor using execFileSync
-import {execSync} from 'child_process';+import {execFileSync} from 'child_process';
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/global-setup.ts` around lines 30 - 64, Replace the execSync calls in the
macOS branch (process.platform === 'darwin') that interpolate bundleID into
command strings with execFileSync using argument arrays: locate the loop over
bundleIDs and the standalone execSync calls (symbols: bundleIDs, execSync) and
call execFileSync('defaults', ['write', bundleID, 'NSQuitAlwaysKeepsWindows',
'-bool', 'false'], {stdio:'pipe'}) and similarly for the other keys
(ApplePersistenceIgnoreState, LSQuarantine, DialogType) to avoid shell
interpolation; preserve the existing try/catch non-fatal behavior and same stdio
option.
🤖 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/global-setup.ts`:
- Around line 48-49: The comment claiming "Verify at least one bundle ID got the
settings applied" is misleading — update the comment above the subsequent
settings block to accurately describe what the code does (apply additional
system-level settings and suppress the "verification of developer" dialog) and
remove the mention of bundle-ID verification; reference the system domains
mentioned (LaunchServices, CrashReporter) in the new comment so it correctly
documents that the following lines adjust system-level domains rather than
performing any bundle-specific verification.
- Around line 30-64: Replace the execSync calls in the macOS branch
(process.platform === 'darwin') that interpolate bundleID into command strings
with execFileSync using argument arrays: locate the loop over bundleIDs and the
standalone execSync calls (symbols: bundleIDs, execSync) and call
execFileSync('defaults', ['write', bundleID, 'NSQuitAlwaysKeepsWindows',
'-bool', 'false'], {stdio:'pipe'}) and similarly for the other keys
(ApplePersistenceIgnoreState, LSQuarantine, DialogType) to avoid shell
interpolation; preserve the existing try/catch non-fatal behavior and same stdio
option.
ℹ️ Review info⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 70033d38-66b6-4678-a46a-f5d9fc89d10a
📥 Commits
Reviewing files that changed from the base of the PR and between e2e49b9 and cd36811.
- Replace execSync with execFileSync using argument arrays to avoid shell
interpolation of bundleID values
- Fix misleading comment that claimed "verify at least one bundle ID"
when the block actually applies system-level LaunchServices/CrashReporter
settings, not per-bundle verification
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 7
🧹 Nitpick comments (2)
e2e/specs/startup/window.test.ts (1)
149-164: ⚡ Quick win
Use polling for the on-screen assertion to reduce startup flakiness.
onScreen is checked once, but restored bounds/display state can settle slightly after app readiness on CI. Wrap this in expect.poll() so both tests wait for convergence instead of failing transiently.
As per coding guidelines: "Prefer polling with expect.poll() or toContainText({timeout}) over one-shot assertions for async state changes".
Also applies to: 200-211
🤖 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/window.test.ts` around lines 149 - 164, Replace the
one-shot on-screen check with a polled assertion: repeatedly evaluate displays
via app2.evaluate (screen.getAllDisplays().map(d => d.workArea)), recompute
midX/midY from bounds and test the onScreen condition until it becomes true or
times out by using expect.poll() around the evaluation of onScreen; update the
assertion that currently uses the onScreen variable so it waits for convergence
and fails only after timeout, and apply the same polling change to the similar
check later that uses the same displays/midX/midY/onScreen pattern.
src/main/app/intercom.test.js (1)
53-57: ⚡ Quick win
Consider using __esModule pattern for singleton mock.
The mock adds once and on methods but uses a flat object structure. Since MainWindow is a singleton imported as a default export, consider wrapping the mock in the __esModule pattern:
This structure better reflects ES module semantics and enables type-safe access with jest.mocked(). As per coding guidelines, "Mock singletons in Jest using __esModule: true + default property."
🤖 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.test.js` around lines 53 - 57, The current Jest mock
for the MainWindow singleton uses a flat object; update it to follow the
__esModule pattern so the default export is a mocked object—wrap the mock in {
__esModule: true, default: { get: jest.fn(), once: jest.fn(), on: jest.fn() } }
so imports of mainWindow default behave like the real ES module and work with
jest.mocked() and type-safe access to get/once/on.
🤖 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 @.github/workflows/e2e-functional-template.yml:
- Around line 345-358: Replace the hardcoded AWS_S3_BUCKET and direct S3 URL
with the repo-standard secret-backed bucket and CDN-mapped public URL: use the
secret variable (MM_DESKTOP_RELEASE_BUCKET) for AWS_S3_BUCKET instead of
"mattermost-cypress-report", keep constructing S3_PREFIX from RUN_ID and
RUN_ATTEMPT as before, sync to that bucket with aws s3 sync, and set REPORT_URL
to the CDN path (e.g.
https://releases.mattermost.com/desktop/${S3_PREFIX}/index.html) so consumers
use the releases.mattermost.com/desktop public URL rather than the direct S3
URL.
- Around line 353-356: The workflow currently forces --content-type text/html on
the aws s3 sync call (the line starting with aws s3 sync e2e/playwright-report/
"s3://${AWS_S3_BUCKET}/${S3_PREFIX}/" --content-type text/html), which
incorrectly sets every asset to text/html; remove the --content-type text/html
flag from the sync command so S3 will infer correct MIME types, and if you need
HTML files to have specific headers, add a separate step that uploads only HTML
(or uses aws s3 cp with --content-type text/html) for index.html/other .html
files after the sync.
In `@e2e/global-setup.ts`:
- Around line 57-58: Update the explanatory comment above the macOS crash dialog
suppression block: replace the phrase "killed by SIGKILL in global-teardown"
with wording that explains the suppression prevents macOS crash dialogs for
SIGTERM or other unexpected quits (since global-teardown now skips SIGKILL on
macOS). Edit the comment near the macOS suppression code so it accurately
describes current behavior (mentioning SIGTERM/other unexpected quits instead of
SIGKILL) to avoid confusion.
In `@e2e/specs/notification_trigger/notification_badge_windows_linux.test.ts`:
- Around line 23-31: The readiness polling currently swallows all evaluate
errors; modify the loop in beforeEach()/triggerBadge() to only retry when the
caught error message indicates a transient Electron execution-context problem
(e.g., contains "Execution context was destroyed" or "Unable to find context"),
otherwise rethrow so non-transient failures fail fast; after the polling loop
assert that the hook was found (throw/expect if !isReady) instead of silently
proceeding, and remove the optional chaining on
__testTriggerSetUnreadBadgeSetting (call it directly and throw if missing) so
the beforeEach() fails immediately when test hooks are not present and the
showUnreadBadgeSetting is reliably reset.
- Around line 66-84: The loop in the beforeEach uses (global as
any).__testTriggerSetUnreadBadgeSetting?.(false) which is a no-op if the hook
isn't registered, so the loop can break prematurely; change the logic to poll
for the hook's existence using electronApp.evaluate to check typeof (global as
any).__testTriggerSetUnreadBadgeSetting === 'function' and only call
__testTriggerSetUnreadBadgeSetting(false) once that check returns true; if the
deadline passes without the hook becoming available, throw an Error (e.g., "test
hook __testTriggerSetUnreadBadgeSetting not registered in time") so beforeEach
fails instead of exiting silently.
In `@e2e/specs/server_management/popout_windows.test.ts`:
- Around line 115-136: The test currently snapshots existing windows by URL in
the before Set, which is ambiguous for multiple popouts; instead capture
identities of existing windows (use electronApp.windows() objects) into before,
then when polling (the expect.poll that assigns popout) find a window w that is
not in before AND whose w.url() includes 'popout.html'; update the before
creation and the poll comparison logic (references: electronApp.windows(),
before, clickFileMenuItem, popout, and the expect.poll block) so you compare
window identity/object membership rather than URL strings.
In `@e2e/utils/analyze-flaky-test.js`:
- Around line 202-209: The returned totalCount can be lower than the
authoritative failure count; update the return to reconcile totalCount as well
(use reconciledFailed and reconciledPassed plus outcomes.skipped) rather than
blindly returning outcomes.total so TOTAL_* is always >= NEW_FAILURES_*;
specifically, compute totalCount = reconciledFailed + reconciledPassed +
outcomes.skipped (or set totalCount = Math.max(outcomes.total, reconciledFailed
+ reconciledPassed + outcomes.skipped)) and return that instead of
outcomes.total, referencing reconciledFailed, reconciledPassed, failureCount and
outcomes.skipped/outcomes.total in the function.
---
Nitpick comments:
In `@e2e/specs/startup/window.test.ts`:
- Around line 149-164: Replace the one-shot on-screen check with a polled
assertion: repeatedly evaluate displays via app2.evaluate
(screen.getAllDisplays().map(d => d.workArea)), recompute midX/midY from bounds
and test the onScreen condition until it becomes true or times out by using
expect.poll() around the evaluation of onScreen; update the assertion that
currently uses the onScreen variable so it waits for convergence and fails only
after timeout, and apply the same polling change to the similar check later that
uses the same displays/midX/midY/onScreen pattern.
In `@src/main/app/intercom.test.js`:
- Around line 53-57: The current Jest mock for the MainWindow singleton uses a
flat object; update it to follow the __esModule pattern so the default export is
a mocked object—wrap the mock in { __esModule: true, default: { get: jest.fn(),
once: jest.fn(), on: jest.fn() } } so imports of mainWindow default behave like
the real ES module and work with jest.mocked() and type-safe access to
get/once/on.
🪄 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: 675d0cf1-4ce2-404e-be21-0c5da7528de1
📥 Commits
Reviewing files that changed from the base of the PR and between e2e49b9 and 45c8a99.
- analyze-flaky-test.js: reconcile totalCount against authoritative
reconciledFailed/reconciledPassed/skipped rather than raw outcomes.total
to keep the three values internally consistent
- e2e-functional-template.yml: use MM_DESKTOP_RELEASE_BUCKET secret for
S3 bucket name, serve report via CDN URL, drop --content-type text/html
from aws s3 sync (S3 infers MIME type from file extension)
- global-setup.ts: update stale comment; teardown no longer uses SIGKILL
- notification_badge_windows_linux.test.ts: rethrow non-transient errors
in triggerBadge catch block; poll for __testTriggerSetUnreadBadgeSetting
existence in beforeEach instead of relying on optional chaining (?.)
which silently no-ops when the hook is not yet registered
- popout_windows.test.ts: snapshot window object identity in `before` Set
rather than URL strings to avoid false matches on URL changes/duplicates
- window.test.ts: wrap one-shot onScreen assertion in expect.poll() so the
window position has time to settle before the check fires
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Fail fast when setup hook never becomes ready before deadline.
Line 75–99 still allows beforeEach to complete without initialization if the deadline is reached, which can leak state into subsequent tests. Add an explicit post-loop failure when readiness is not achieved.
Proposed 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 initialized = false;
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;
});
+ initialized = 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 (!initialized) {+ throw new Error('test hook __testTriggerSetUnreadBadgeSetting not registered in time');+ }
});
🤖 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 70 - 99, The beforeEach loop can exit without initializing if the
deadline is reached; update the test.beforeEach to throw an explicit error after
the polling loop if the readiness flag wasn't set so tests fail fast. After the
while loop that polls electronApp.evaluate for (global as
any).__testTriggerSetUnreadBadgeSetting and before returning from beforeEach,
check the readiness state (e.g., re-evaluate or track a local boolean) and throw
a descriptive Error like "badge setup hook not ready before deadline" when not
ready; reference the existing electronApp.evaluate calls and the
__testTriggerSetUnreadBadgeSetting / __testBadgeState symbols to locate where to
add the check and throw.
🧹 Nitpick comments (1)
e2e/utils/analyze-flaky-test.js (1)
61-66: 💤 Low value
Minor: Missing skipped check for consistency with getOutcomeCounts.
The hasPassingRetry check (line 62) doesn't verify c.skipped === undefined, so a skipped test would be treated as "passing". This could cause a failed retry to be excluded if its base test was skipped.
getOutcomeCounts (lines 153-155) handles this correctly by including the skipped check. Consider aligning for consistency, though the practical impact is likely minimal.
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 61 - 66, The hasPassingRetry
predicate in analyze-flaky-test.js treats a retry with c.skipped as passing;
update the predicate used to compute hasPassingRetry to also check that
c.skipped === undefined (i.e., change the arrow function in the cases.some call
to require c.name === baseName && c.failure === undefined && c.error ===
undefined && c.skipped === undefined) so it matches the behavior of
getOutcomeCounts and avoids treating skipped retries as passes.
🤖 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.
Duplicate comments:
In `@e2e/specs/notification_trigger/notification_badge_windows_linux.test.ts`:
- Around line 70-99: The beforeEach loop can exit without initializing if the
deadline is reached; update the test.beforeEach to throw an explicit error after
the polling loop if the readiness flag wasn't set so tests fail fast. After the
while loop that polls electronApp.evaluate for (global as
any).__testTriggerSetUnreadBadgeSetting and before returning from beforeEach,
check the readiness state (e.g., re-evaluate or track a local boolean) and throw
a descriptive Error like "badge setup hook not ready before deadline" when not
ready; reference the existing electronApp.evaluate calls and the
__testTriggerSetUnreadBadgeSetting / __testBadgeState symbols to locate where to
add the check and throw.
---
Nitpick comments:
In `@e2e/utils/analyze-flaky-test.js`:
- Around line 61-66: The hasPassingRetry predicate in analyze-flaky-test.js
treats a retry with c.skipped as passing; update the predicate used to compute
hasPassingRetry to also check that c.skipped === undefined (i.e., change the
arrow function in the cases.some call to require c.name === baseName &&
c.failure === undefined && c.error === undefined && c.skipped === undefined) so
it matches the behavior of getOutcomeCounts and avoids treating skipped retries
as passes.
ℹ️ Review info⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e9b161f9-29fd-426d-98e3-c02c3b107417
📥 Commits
Reviewing files that changed from the base of the PR and between 45c8a99 and 4d8521e.
The MM_DESKTOP_RELEASE_BUCKET secret points to releases-mattermost which
the CI IAM user (mattermost-cypress-report) has no access to. Revert to
the original hardcoded bucket name and matching S3-hosted report URL to
restore upload access.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…r and closeAllPopouts
Cover the two new methods added in the popout-cleanup feature:
registerMainWindowCloseHandler:
- window exists → win.once('closed', ...) is registered
- window missing → early return, no throw
closeAllPopouts:
- calls each cleanup listener, destroys browser windows, empties both Maps
- skips destroy() for already-destroyed browser windows
- swallows per-entry errors and continues to remaining windows
- handles empty Map without error
MAIN_WINDOW_CREATED wiring:
- constructor registers MainWindow.on(MAIN_WINDOW_CREATED, ...)
- full chain: MAIN_WINDOW_CREATED → registerMainWindowCloseHandler → 'closed'
→ closeAllPopouts destroys all tracked popouts
Also imports MAIN_WINDOW_CREATED from common/communication.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The reason will be displayed to describe this comment to others. Learn more.
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 @.github/workflows/e2e-functional.yml:
- Line 44: The top-level workflow permission "pull-requests: write" is too
broad; remove that entry from the global permissions block and instead add
"pull-requests: write" only to the specific job(s) that need it (the job(s)
which update PR metadata/body) by adding a permissions: block inside those job
definitions and setting "pull-requests: write" there while keeping default
least-privilege permissions at the workflow level.
🪄 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: 0e0840fc-2229-4c4d-b1b8-f6fae917c7b3
📥 Commits
Reviewing files that changed from the base of the PR and between 8b865eb and 9d891fb.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Scope pull-requests: write to only the job that needs it.
Granting PR write at workflow level expands write access for every job token in this workflow. Move this permission to the specific job(s) that update PR metadata/body.
‼️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
pull-requests: write
permissions:
contents: read
statuses: write
jobs:
e2e-tests:
permissions:
pull-requests: write
needs:
- prepare-matrix
- update-initial-status
🧰 Tools🪛 zizmor (1.25.2)
[error] 44-44: overly broad permissions (excessive-permissions): pull-requests: write is overly broad at the workflow level
(excessive-permissions)
🤖 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/e2e-functional.yml at line 44, The top-level workflow
permission "pull-requests: write" is too broad; remove that entry from the
global permissions block and instead add "pull-requests: write" only to the
specific job(s) that need it (the job(s) which update PR metadata/body) by
adding a permissions: block inside those job definitions and setting
"pull-requests: write" there while keeping default least-privilege permissions
at the workflow level.
ci run 26487516636 fails on all platforms in src/main/app/intercom.test.js: the Jest electron mock has no app.once, but this PR calls app.once('before-quit', …) in handleMainWindowIsShown (intercom.ts:145). Error: TypeError: _electron.app.once is not a function.
report-test-results failed only because macos-test-results artifact was missing after the mac build job failed (cascade).
Electron Playwright Tests run 26487521780 succeeded on the same branch — E2E can pass while ci unit jobs stay red.
Product (interactive Tier A)
No PR-introduced product regression confirmed with traces. Server-backed popout/login scenarios were Blocked (Mattermost test server: login form/shell not ready after login).
Adversarial: cold start with bounds-info.jsonx=-9999 never sets __e2eAppReady within 20–45s on both PR head and master — pre-existing bounds/readiness edge, not PR-specific.
MM-T4403_2 failed once on __e2eAppReady timeout then passed on immediate rerun (flake; not promoted).
Operator: fix intercom.test.js mock (app.once) before merge; verify PR Mattermost server is ready for login-backed QA.
The reason will be displayed to describe this comment to others. Learn more.
CI regression (confirmed):handleMainWindowIsShown now calls app.once('before-quit', onBeforeQuit), but intercom.test.js mocks electron.app without once. Linux/mac/win ci jobs fail with TypeError: app.once is not a function. Extend the Jest mock with once/removeListener (or stub before-quit cleanup) so unit tests match production Electron APIs.
Failure before: TypeError: _electron.app.once is not a function at intercom.ts:145
Fix: add app.once and app.removeListener to jest electron mock; add removeListener to window mock
Verification: npm run test:unit -- src/main/app/intercom.test.js passes; master lacks new handleMainWindowIsShown cases
Risk: R2 from QA report
Co-authored-by: yasser khan <attitude3cena.yf@gmail.com>
All three platform jobs failed in ci/test, not in packaging: TypeError: _electron.app.once is not a function in intercom.test.js after app.once('before-quit', …) was added in intercom.ts:145. Pushed fix 4d077d05 — adds app.once / removeListener to the Jest electron mock. report-test-results failed only because macos-test-results was never uploaded when the mac job failed.
Product / E2E
Pass (test build): welcome screen + window bounds specs; unit suites intercom.test.js (10) and popoutManager.test.js (61).
Pass (prod build):dist/ launch shows main index window.
Blocked (server-backed): Popout E2E could not log in — server view stayed on team root URL; no #input_loginId and no app shell within helper timeouts. API ping/login succeeded. Re-check MM_TEST_USER_NAME / MM_TEST_PASSWORD for the PR automation server before treating popout behavior as verified.
Merge note (from PR description, not re-tested here)
CMT workflow still depends on Matterwick /cmt_dispatch deployment and repo secrets per author rollout order.
Full report: qa-report.md on the QA branch run.
Sent by Cursor Automation: Desktop QA Agent | Linux
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick comments (12)
qa-report.md (12)
137-150: ⚡ Quick win
Add language identifier to fenced code block.
Add yaml or text to enable syntax highlighting.
📝 Proposed fix
-```+```yaml
id: S5
🤖 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 `@qa-report.md` around lines 137 - 150, The fenced code block at the top of the
report is missing a language identifier, so add one (for example `yaml` or
`text`) after the opening triple backticks to enable syntax highlighting; update
the block that begins with the lines "id: S5" and "tier: A" so the opening fence
becomes "```yaml" (or "```text") to mark the block language.
92-108: ⚡ Quick win
Add language identifier to fenced code block.
Add yaml or text to enable syntax highlighting.
📝 Proposed fix
-```+```yaml
id: S2
🤖 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 `@qa-report.md` around lines 92 - 108, The fenced code block containing the
report front-matter (starting with ``` and lines like "id: S2", "tier: A",
"verdict: Pass") needs a language identifier for syntax highlighting; change the
opening fence from ``` to ```yaml (or ```text) so the block is recognized as
YAML, ensuring keys such as id, tier, verdict, start_utc, end_utc, etc., are
highlighted correctly.
110-126: ⚡ Quick win
Add language identifier to fenced code block.
Add yaml or text to enable syntax highlighting.
📝 Proposed fix
-```+```yaml
id: S3
🤖 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 `@qa-report.md` around lines 110 - 126, The fenced code block in qa-report.md
(the block containing the S3 report starting with the lines "id: S3" and "tier:
A") lacks a language identifier; update the opening fence from ``` to ```yaml
(or ```text) so the block becomes a labeled YAML code block, ensuring proper
syntax highlighting for the entire block that includes "id: S3", "verdict:
Blocked", "trace:", "screenshot:", etc.
6-6: 💤 Low value
Consider breaking the summary paragraph into shorter sentences.
The 90+ word paragraph packs CI failure root cause, fix commit, blocked scenarios, and operator guidance into a single sentence. While technically correct, breaking it into 2-3 sentences would improve scannability for operators triaging the report.
✏️ Proposed rewrite for readability
-CI was failing on all platforms because `src/main/app/intercom.test.js` mocked `electron.app` without `app.once`, while the PR added `app.once('before-quit', …)` in `handleMainWindowIsShown`. That gap is fixed in commit `4d077d05` on the PR branch. Tier A runs without a live Mattermost session passed: the PR automation server answers ping and API login, but the embedded server view never reached the login form or post-login app shell within the harness timeouts, so popout and other server-backed scenarios are **Blocked** here—not failed as product regressions. Unit coverage for popout cleanup and intercom readiness passed. Operator should confirm `MM_TEST_USER_NAME` / `MM_TEST_PASSWORD` match the PR test server and re-run Linux E2E after CI is green.+CI was failing on all platforms because `src/main/app/intercom.test.js` mocked `electron.app` without `app.once`, while the PR added `app.once('before-quit', …)` in `handleMainWindowIsShown`. That gap is fixed in commit `4d077d05` on the PR branch.++Tier A runs without a live Mattermost session passed. The PR automation server answers ping and API login, but the embedded server view never reached the login form or post-login app shell within the harness timeouts, so popout and other server-backed scenarios are **Blocked** here—not failed as product regressions. Unit coverage for popout cleanup and intercom readiness passed.++Operator should confirm `MM_TEST_USER_NAME` / `MM_TEST_PASSWORD` match the PR test server and re-run Linux E2E after CI is green.
🤖 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 `@qa-report.md` at line 6, Split the long summary into 2–3 shorter sentences
for readability: first state the CI failure root cause (intercom.test.js mocked
electron.app without app.once while handleMainWindowIsShown now uses
app.once('before-quit', …)) and mention the fix commit 4d077d05; second note the
current test status — Tier A runs without a live Mattermost session, the PR
automation server responds but the embedded server view never reached login/app
shell so popout and server-backed scenarios are Blocked (not Failed); finally
add a short operator action sentence to confirm MM_TEST_USER_NAME /
MM_TEST_PASSWORD match the PR test server and re-run Linux E2E, and briefly
mention unit coverage for popout cleanup and intercom readiness passed.
38-47: ⚡ Quick win
Add blank line before table.
Markdown best practice requires blank lines surrounding tables for consistent rendering across parsers.
📝 Proposed fix
### Risks
+
| ID | Risk | Hunk |
🤖 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 `@qa-report.md` around lines 38 - 47, Add a blank line before the Markdown
table that starts under the "### Risks" heading so the table is separated from
the preceding content; locate the "### Risks" heading and the table row
beginning with "| ID | Risk | Hunk |" in qa-report.md and insert a single empty
line between the heading (or prior paragraph) and that table to ensure
consistent rendering across Markdown parsers.
194-197: 💤 Low value
Consider clarifying the baseline phrasing.
The phrase "new tests did not exist on master (failure PR-specific mock gap)" is slightly ambiguous. Consider: "Baseline: New tests introduced by this PR; failure due to mock gap specific to this PR's changes."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@qa-report.md` around lines 194 - 197, Update the baseline phrasing in the QA
table row for SHA `4d077d05` referencing `src/main/app/intercom.test.js` so it
reads explicitly that the tests are new in this PR and the failure was due to a
PR-specific mock gap; replace "new tests did not exist on master (failure
PR-specific mock gap)" with a clearer sentence such as "Baseline: New tests
introduced by this PR; failure due to mock gap specific to this PR's changes."
to remove ambiguity.
163-169: ⚡ Quick win
Add language identifier to fenced code block.
Add yaml or text to enable syntax highlighting.
📝 Proposed fix
-```+```yaml
id: S7
🤖 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 `@qa-report.md` around lines 163 - 169, The fenced code block containing the
YAML-like key-value lines (starting with "id: S7", "tier: B", etc.) should
include a language identifier for syntax highlighting; update the opening
triple-backtick fence to use a language like "yaml" (or "text") so the block
becomes ```yaml and retains the same inner content; ensure you only modify the
opening fence and not the block contents or the adjacent npm test result line.
171-177: ⚡ Quick win
Add language identifier to fenced code block.
Add yaml or text to enable syntax highlighting.
📝 Proposed fix
-```+```yaml
id: S8
🤖 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 `@qa-report.md` around lines 171 - 177, The fenced code block containing "id:
S8\ntier: B\nrisks: R2,R3\nverdict: Pass" needs a language identifier for syntax
highlighting; update the opening fence in qa-report.md (the block that starts
with ``` and contains id: S8) to use a language tag such as yaml (```yaml) or
text (```text) so the block is highlighted correctly.
73-89: ⚡ Quick win
Add language identifier to fenced code block.
The structured execution log resembles YAML. Adding a language identifier (yaml or text) improves syntax highlighting and maintainability.
📝 Proposed fix
-```+```yaml
id: S1
🤖 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 `@qa-report.md` around lines 73 - 89, The fenced code block containing the
structured execution log (keys like id: S1, verdict: Pass, start_utc, end_utc)
should include a language identifier to enable proper syntax highlighting;
update the opening fence from ``` to ```yaml (or ```text) so the block is marked
as YAML, leaving the block contents unchanged.
128-135: ⚡ Quick win
Add language identifier to fenced code block.
Add yaml or text to enable syntax highlighting.
📝 Proposed fix
-```+```yaml
id: S4
🤖 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 `@qa-report.md` around lines 128 - 135, The fenced code block in qa-report.md
lacks a language identifier; update the opening ``` to ```yaml (or ```text) so
the block containing "id: S4", "tier: A", "risks: R1", "verdict: Blocked",
"build: test", "blocker: Same as S3 (requires logged-in Mattermost server
view)." is annotated for syntax highlighting by changing the first line from ```
to ```yaml (or ```text).
19-27: ⚡ Quick win
Add blank line before table.
Markdown best practice requires blank lines surrounding tables for consistent rendering across parsers.
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@qa-report.md` around lines 19 - 27, The markdown under the heading "### Files
changed by surface" is missing a blank line before the table which can break
rendering in some parsers; open the qa-report.md content around the "### Files
changed by surface" heading and insert a single blank line immediately before
the table row starting with "| Surface | Paths |" so the table is separated from
the heading and follows Markdown best practices.
155-161: ⚡ Quick win
Add language identifier to fenced code block.
Add yaml or text to enable syntax highlighting.
📝 Proposed fix
-```+```yaml
id: S6
🤖 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 `@qa-report.md` around lines 155 - 161, The fenced code block containing the
test metadata (starting with the line "id: S6") lacks a language identifier;
update the opening fence to include "yaml" (e.g., change "```" to "```yaml") so
the block is syntax-highlighted; target the fenced block that contains "id: S6",
"tier: B", "risks: R4", and "verdict: Pass" and leave the block contents
unchanged.
🤖 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 `@qa-report.md`:
- Around line 137-150: The fenced code block at the top of the report is missing
a language identifier, so add one (for example `yaml` or `text`) after the
opening triple backticks to enable syntax highlighting; update the block that
begins with the lines "id: S5" and "tier: A" so the opening fence becomes
"```yaml" (or "```text") to mark the block language.
- Around line 92-108: The fenced code block containing the report front-matter
(starting with ``` and lines like "id: S2", "tier: A", "verdict: Pass") needs a
language identifier for syntax highlighting; change the opening fence from ```
to ```yaml (or ```text) so the block is recognized as YAML, ensuring keys such
as id, tier, verdict, start_utc, end_utc, etc., are highlighted correctly.
- Around line 110-126: The fenced code block in qa-report.md (the block
containing the S3 report starting with the lines "id: S3" and "tier: A") lacks a
language identifier; update the opening fence from ``` to ```yaml (or ```text)
so the block becomes a labeled YAML code block, ensuring proper syntax
highlighting for the entire block that includes "id: S3", "verdict: Blocked",
"trace:", "screenshot:", etc.
- Line 6: Split the long summary into 2–3 shorter sentences for readability:
first state the CI failure root cause (intercom.test.js mocked electron.app
without app.once while handleMainWindowIsShown now uses app.once('before-quit',
…)) and mention the fix commit 4d077d05; second note the current test status —
Tier A runs without a live Mattermost session, the PR automation server responds
but the embedded server view never reached login/app shell so popout and
server-backed scenarios are Blocked (not Failed); finally add a short operator
action sentence to confirm MM_TEST_USER_NAME / MM_TEST_PASSWORD match the PR
test server and re-run Linux E2E, and briefly mention unit coverage for popout
cleanup and intercom readiness passed.
- Around line 38-47: Add a blank line before the Markdown table that starts
under the "### Risks" heading so the table is separated from the preceding
content; locate the "### Risks" heading and the table row beginning with "| ID |
Risk | Hunk |" in qa-report.md and insert a single empty line between the
heading (or prior paragraph) and that table to ensure consistent rendering
across Markdown parsers.
- Around line 194-197: Update the baseline phrasing in the QA table row for SHA
`4d077d05` referencing `src/main/app/intercom.test.js` so it reads explicitly
that the tests are new in this PR and the failure was due to a PR-specific mock
gap; replace "new tests did not exist on master (failure PR-specific mock gap)"
with a clearer sentence such as "Baseline: New tests introduced by this PR;
failure due to mock gap specific to this PR's changes." to remove ambiguity.
- Around line 163-169: The fenced code block containing the YAML-like key-value
lines (starting with "id: S7", "tier: B", etc.) should include a language
identifier for syntax highlighting; update the opening triple-backtick fence to
use a language like "yaml" (or "text") so the block becomes ```yaml and retains
the same inner content; ensure you only modify the opening fence and not the
block contents or the adjacent npm test result line.
- Around line 171-177: The fenced code block containing "id: S8\ntier: B\nrisks:
R2,R3\nverdict: Pass" needs a language identifier for syntax highlighting;
update the opening fence in qa-report.md (the block that starts with ``` and
contains id: S8) to use a language tag such as yaml (```yaml) or text (```text)
so the block is highlighted correctly.
- Around line 73-89: The fenced code block containing the structured execution
log (keys like id: S1, verdict: Pass, start_utc, end_utc) should include a
language identifier to enable proper syntax highlighting; update the opening
fence from ``` to ```yaml (or ```text) so the block is marked as YAML, leaving
the block contents unchanged.
- Around line 128-135: The fenced code block in qa-report.md lacks a language
identifier; update the opening ``` to ```yaml (or ```text) so the block
containing "id: S4", "tier: A", "risks: R1", "verdict: Blocked", "build: test",
"blocker: Same as S3 (requires logged-in Mattermost server view)." is annotated
for syntax highlighting by changing the first line from ``` to ```yaml (or
```text).
- Around line 19-27: The markdown under the heading "### Files changed by
surface" is missing a blank line before the table which can break rendering in
some parsers; open the qa-report.md content around the "### Files changed by
surface" heading and insert a single blank line immediately before the table row
starting with "| Surface | Paths |" so the table is separated from the heading
and follows Markdown best practices.
- Around line 155-161: The fenced code block containing the test metadata
(starting with the line "id: S6") lacks a language identifier; update the
opening fence to include "yaml" (e.g., change "```" to "```yaml") so the block
is syntax-highlighted; target the fenced block that contains "id: S6", "tier:
B", "risks: R4", and "verdict: Pass" and leave the block contents unchanged.
ℹ️ Review info⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bb479502-e144-43bb-9e88-1f8f16f6a91f
📥 Commits
Reviewing files that changed from the base of the PR and between 9d891fb and 4bfe9d0.
📒 Files selected for processing (2)
qa-report.md
src/main/app/intercom.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
src/main/app/intercom.test.js
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
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
github-actions
Bot
added
E2E/Run
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Problem
CMT Provisioner runs on desktop succeed in GitHub Actions but never trigger the follow-up
compatibility-matrix-testing.ymlworkflow. The two most recent manual CMT runs both showed this:Matterwick was receiving the
workflow_run.requestedwebhook (verified, 202 OK), but it was never dispatching the test workflow.Root cause
The previous design relied on matterwick reading the
server_versionsworkflow_dispatch input out of theworkflow_run.requestedwebhook payload. GitHub does not include workflow_dispatch inputs on theworkflow_runobject in webhook payloads — verified against the Octokit webhook schema (35 fields, noinputs), the actual REST API for run 26026866029, and the official GitHub docs.So matterwick received an empty input map, bailed out at
if !ok || serverVersionsStr == "", and never calleddispatchCMTWorkflow.Fix (workflow side)
Replace the echo-only stub with a direct
curlPOST to matterwick's new/cmt_dispatchendpoint, sending the full context the webhook payload cannot carry:{ "owner": "mattermost", "repo": "desktop", "sha": "...", "ref": "master", "run_id": <github.run_id>, "server_versions": "v11.1.0, v11.2.0" }Matterwick returns
202 Acceptedimmediately and provisions instances + dispatchescompatibility-matrix-testing.ymlasynchronously in a goroutine (~30 min provisioning window, same as before).The workflow also fails fast with a clear error if
vars.MATTERWICK_URLorsecrets.MATTERWICK_CMT_TRIGGER_SECRETis missing on the repo.Companion PR
/cmt_dispatchand/cleanup_e2eendpoints. Must be deployed before this PR is merged, otherwise the curl will 404.CMTTriggerSecretto matterwick config)Rollout order
This PR depends on:
mattermost/matterwick#90merged + deployedgitops-platformPR merged with real (non-placeholder) tokensMATTERWICK_CMT_TRIGGER_SECRETadded to this repo's GitHub Actions secrets (the value matches theCMTTriggerSecretset in step 2)MATTERWICK_CLEANUP_SECRET(already present) rotated to match the newCleanupSecretin gitops, if rotatedOnly after all four are in place is this PR safe to merge. Until then, dispatching CMT Provisioner will fail loudly with the explicit pre-flight error.
What this does NOT touch
Testing
yaml.safe_loadMATTERWICK_URLandMATTERWICK_CMT_TRIGGER_SECRET(fail with explicit error message)--fail-with-body --retry 3ensures the workflow step fails loudly if matterwick is unreachable or returns non-2xxv11.1.0, v11.2.0→ confirm matterwick log showsendpoint=/cmt_dispatch Accepted CMT dispatch request→ wait ~30 min → confirmcompatibility-matrix-testing.ymldispatchedTest plan
.github/workflows/cmt-provisioner.ymlvars.MATTERWICK_URLis set on repo (already used by existing cleanup curl)MATTERWICK_CMT_TRIGGER_SECRETto repo secrets (synced with gitopsCMTTriggerSecret)compatibility-matrix-testing.ymlfires ~30 min later with correctCMT_MATRIX🤖 Generated with Claude Code
Change Impact: 🟡 Medium
Regression Risk:
The largest risk is CI/dispatch reliability: replacing a webhook-driven stub with a direct POST to Matterwick adds a runtime dependency (MATTERWICK_URL, MATTERWICK_CMT_TRIGGER_SECRET) and requires Matterwick deployment (mattermost/matterwick#90). Although pre-flight variable/secret checks and curl retry/fail flags reduce silent failures, network issues, misconfiguration, or deployment sequencing can cause workflow failures or missed dispatches. The changes also modify multiple CI workflows and E2E test infrastructure (moderate blast radius). Application code changes are limited and covered by added unit tests (popout cleanup, intercom), lowering regression risk for runtime behavior.
QA Recommendation:
Perform targeted manual validation prior to merge: ensure matterwick endpoint is deployed and required secrets/vars are configured, run an end-to-end provisioner execution to confirm Matterwick returns 202 and dispatches compatibility-matrix-testing.yml (~30 minutes). Verify pre-flight checks and that curl retry/fail behavior surfaces clear diagnostics on failures. Run affected E2E suites and new/updated unit tests to validate test infra, popout cleanup, and related behaviors.
Generated by CodeRabbitAI
Server for Cursor Automation: https://desktop-pr-3829-linux-j1snsewh.test.mattermost.cloud