[MM-68738][MM-68739][MM-68741][MM-68742][MM-68747][MM-68748][MM-68750][MM-68814][MM-68880][MM-68908][MM-69030][MM-69783] Guard against destroyed Electron objects and premature window creation

[devinbinnie]

Summary

Sentry surfaced a cluster of crashes where the main window, a server's WebContents, or the tray were accessed after being destroyed, or where a BrowserWindow was constructed before Electron's ready event fired. In most cases the offending code either had no isDestroyed() check at all, or checked one object (e.g. a view's own webContents) while dereferencing another (e.g. its parent window) unchecked.

This PR adds the missing guards at each crash site, and in the "Object has been destroyed" category, fixes the shared root cause: MainWindow.get()/getBounds() returned a destroyed BrowserWindow instead of undefined, so the many existing if (!mainWindow) checks throughout the codebase didn't catch it.

TypeError: Object has been destroyed

• MM-68738 — MattermostWebContentsView's load-retry, cert-error, and load-success/incompatible-server paths sent IPC messages to parentWindow.webContents after the parent window was destroyed. Added a sendToParentWindow helper that checks isDestroyed() before sending.
• MM-68739 — AppState.emitStatusForServer()/updateUnreadsAndMentionsPerServer() fan out to listeners (e.g. the badge) that called MainWindow.get() and used the result without checking for destruction. Fixed by hardening MainWindow.get().
• MM-68741 — ServerManager.setLoggedIn() drives TabManager.setActiveTab(), which sent to MainWindow.get() with the same gap. Fixed by hardening MainWindow.get().
• MM-68742 — ViewManager.updateViewTitle() (from handlePageTitleUpdated) drives TabManager.handleViewUpdated(), same MainWindow.get() gap. Fixed by hardening MainWindow.get().
• MM-68747 — PermissionsManager.doPermissionRequest() read mainWindow.webContents.id off a possibly-destroyed MainWindow.get() result. Fixed by hardening MainWindow.get().
• MM-68750 — downloadsDropdownView.updateDownloadsDropdown() called MainWindow.getBounds() during downloadsManager.saveAll(), which called getContentBounds() on a destroyed window. Fixed by routing getBounds() through the hardened get().
• MM-68908 — MainWindow.saveWindowState() read bounds/maximized/fullscreen state off the BrowserWindow on blur/close without an isDestroyed() check. Added the guard directly.
• MM-69783 — AppState.emitStatus() had the same MainWindow.get() gap as MM-68739. Fixed by hardening MainWindow.get().

Error: Tray is destroyed

• MM-68814 — MenuManager's IPC-driven menu refresh called Tray.setMenu() during shutdown, hitting a destroyed native Tray. setMenu() now checks isDestroyed() before calling setContextMenu(), matching the existing guard in update().
• MM-69030 — Same setMenu() call, triggered by refreshMenu() instead. Covered by the same guard.

Error: Cannot create BrowserWindow before app is ready

• MM-68748 — activate could fire before Electron's ready event, so MainWindow.show() called init() and constructed a BrowserWindow too early. handleAppActivate() now checks app.isReady() and defers to app.once('ready', ...) if it isn't.
• MM-68880 — Same race condition as MM-68748. Covered by the same fix.

Ticket Link

• MM-68738
• MM-68739
• MM-68741
• MM-68742
• MM-68747
• MM-68748
• MM-68750
• MM-68814
• MM-68880
• MM-68908
• MM-69030
• MM-69783

Release Note

Fixed several crashes caused by accessing the main window, a server view, or the tray icon after it was destroyed, or by creating the main window before the app finished launching.

Change Impact: 🟡 Medium

Regression Risk: Lifecycle/window-management hardening across startup/activation, IPC, tray menu updates, and multiple Electron view paths; while changes are defensive (destroyed-object guards) and tests cover handleAppActivate/some window behaviors, key destroyed-object guard cases are not fully exercised, leaving moderate risk around startup/activate and window/tray state transitions.
QA Recommendation: Perform brief manual smoke testing: app startup, macOS dock “activate” before/at ready, main window open/close, tray menu interactions, and a scenario that triggers Mattermost view load/retry (including teardown) to ensure no crashes/regressions.
Generated by CodeRabbitAI

[coderabbitai]

📝 Walkthrough

Walkthrough

The changes add destruction checks for Electron windows, trays, and parent-window IPC, update related mocks, and introduce readiness-aware handling for the app activation event.

Changes

Electron teardown safety

Layer / File(s)	Summary
Window and tray destruction guards src/app/mainWindow/mainWindow.ts, src/app/mainWindow/mainWindow.test.js, src/app/system/tray/tray.ts	Main window access, bounds retrieval, state persistence, and tray menu updates now stop when Electron objects are missing or destroyed.
Parent-window IPC guard src/app/views/MattermostWebContentsView.ts, src/app/views/MattermostWebContentsView.test.js	Load and retry notifications use guarded parent-window IPC, and affected window mocks now provide isDestroyed().

App activation readiness

Layer / File(s)	Summary
Activation handler and wiring src/main/app/app.ts, src/main/app/initialize.ts, src/main/app/app.test.js	handleAppActivate shows the main window immediately when the app is ready or after the one-time ready event, with tests for both paths.

Estimated code review effort: 2 (Simple) | ~15 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ElectronApp
  participant handleAppActivate
  participant MainWindow
  ElectronApp->>handleAppActivate: activate event
  handleAppActivate->>ElectronApp: check isReady()
  alt app is ready
    handleAppActivate->>MainWindow: show()
  else app is not ready
    handleAppActivate->>ElectronApp: once('ready')
    ElectronApp-->>handleAppActivate: ready event
    handleAppActivate->>MainWindow: show()
  end

Loading

Suggested reviewers: svelle

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.00% 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 matches the main change: protecting Electron objects from destruction and avoiding premature window creation.
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.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch more_crash_guards

---

_{Comment @coderabbitai help to get the list of available commands.}

[mm-cloud-bot]

❌ E2E Test Setup Failed

Failed to create E2E test instances: failed to create installation: failed with status code 409

[mattermost-build]

Test check c57a656 — action needed

Tests are present but only update mocks for compatibility; the core isDestroyed() guard branches (the actual crash fixes) are never exercised in any test.
Add unit tests that set isDestroyed() to return true and verify each guard suppresses the call — specifically for MainWindow.get(), saveWindowState(), TrayIcon.setMenu(), and sendToParentWindow().

More details (truncated)

Test Files Detected

Category	Count	Files
Unit	3	src/app/mainWindow/mainWindow.test.js, src/app/views/MattermostWebContentsView.test.js, src/main/app/app.test.js
Integration	0	—
E2E	0	—

Analysis

The PR fixes multiple crash-on-destruction bugs by adding isDestroyed() guards across four production files. The test changes fall into two categories:

1. Mock compatibility updates (not new coverage):

• MattermostWebContentsView.test.js: Adds isDestroyed: jest.fn(() => false) to ~12 window mocks across all describe blocks. These updates only ensure existing tests keep passing after the new sendToParentWindow helper was introduced — they do NOT test the guard itself (the isDestroyed() === true path).
• mainWindow.test.js: Adds isDestroyed: jest.fn(() => false) to one window mock for similar reasons.

2. Genuine new test coverage:

• app.test.js: Adds two tests for the new handleAppActivate() function — one for when app.isReady() is true (calls MainWindow.show() immediately) and one for when it is false (defers via app.once('ready', ...)). These adequately cover the new logic.

Coverage gaps for the core crash-prevention logic:

• MainWindow.get() — The hardened behavior (returning undefined when browserWindow.isDestroyed() is true) has no test. All existing tests mock isDestroyed returning false, meaning the new undefined-return branch is never exercised.
• MainWindow.saveWindowState() — The new early-return guard when window.isDestroyed() is true has no test.
• TrayIcon.setMenu() — There is no test file for tray.ts at all. The new isDestroyed() check in setMenu() is completely untested.
• MattermostWebContentsView.sendToParentWindow() — The new private helper suppresses IPC when parentWindow.isDestroyed() is true, but no test verifies that behavior. Tests only confirm the happy path (not destroyed).

The most safety-critical fixes — the isDestroyed() guards that prevent the crashes — are the ones without test coverage.

Files Analyzed

Production files (5/5):

• src/app/mainWindow/mainWindow.ts
• src/app/system/tray/tray.ts
• src/app/views/MattermostWebContentsView.ts
• src/main/app/app.ts
• src/main/app/initialize.ts

Test files (3/3):

• src/app/mainWindow/mainWindow.test.js
• src/app/views/MattermostWebContentsView.t

...truncated. View full analysis details

Suggestions

1. MainWindow.get() destroyed path — Add a unit test in mainWindow.test.js that sets win.browserWindow.isDestroyed to return true and asserts mainWindow.get() returns undefined.
2. MainWindow.saveWindowState() guard — Add a test that calls saveWindowState with a mock window whose isDestroyed() returns true and asserts that getBounds/isMaximized are never called.
3. TrayIcon.setMenu() destroyed path — Add a test file (or extend an existing one) for tray.ts that calls setMenu() on a destroyed tray and asserts setContextMenu is not called.
4. MattermostWebContentsView.sendToParentWindow() — Add a test scenario (e.g., within the existing load/retry/loadSuccess describe blocks) where the parent window mock returns isDestroyed: jest.fn(() => true) and asserts that webContents.send is NOT called.

To override, comment /test-analysis-override <reason> after verifying tests are adequate or not required.

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (2)

src/app/views/MattermostWebContentsView.test.js (1)

127-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add coverage for destroyed parent windows.

All updated mocks return false from isDestroyed(), so the new early-return behavior is untested. Add cases with isDestroyed() returning true and assert that parent webContents.send() is not called.

Also applies to: 193-193, 257-257, 275-275, 305-305, 334-334, 466-466, 498-498, 559-559, 635-635, 642-642, 649-649, 658-658, 668-668

🤖 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/views/MattermostWebContentsView.test.js` at line 127, Add test cases
in MattermostWebContentsView.test.js for each affected parent-window mock,
configuring isDestroyed() to return true and asserting parent webContents.send()
is not called. Cover the early-return behavior while preserving the existing
assertions for non-destroyed windows.

src/main/app/app.test.js (1)

69-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use jest.mocked() for mock-function access.

The new tests call .mockReturnValue(), .mockImplementation(), and assertions directly on mocked functions. Wrap app.isReady, app.once, and MainWindow.show with jest.mocked() to follow the repository test convention.

Proposed adjustment

-            app.isReady.mockReturnValue(true);
+            jest.mocked(app.isReady).mockReturnValue(true);
...
-            expect(MainWindow.show).toHaveBeenCalled();
+            expect(jest.mocked(MainWindow.show)).toHaveBeenCalled();
...
-            app.once.mockImplementation((event, cb) => {
+            jest.mocked(app.once).mockImplementation((event, cb) => {

🤖 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/app.test.js` around lines 69 - 85, Update the tests for
handleAppActivate to wrap app.isReady, app.once, and MainWindow.show with
jest.mocked() before calling mockReturnValue, mockImplementation, or assertion
methods, preserving the existing test behavior and expectations.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/app/mainWindow/mainWindow.ts`:
- Around line 133-137: Update show() to obtain the window through the guarded
get() accessor instead of directly using this.win.browserWindow. Reuse that
guarded result for the branch and all subsequent show/focus operations,
preserving the existing behavior while preventing operations on a destroyed
window.
- Around line 222-226: Update the onBlur() flow to check whether the target
window is destroyed before calling getContentBounds() or emitting bounds. Return
immediately for a destroyed window, then pass the validated window to
saveWindowState() so its guard remains effective.

---

Nitpick comments:
In `@src/app/views/MattermostWebContentsView.test.js`:
- Line 127: Add test cases in MattermostWebContentsView.test.js for each
affected parent-window mock, configuring isDestroyed() to return true and
asserting parent webContents.send() is not called. Cover the early-return
behavior while preserving the existing assertions for non-destroyed windows.

In `@src/main/app/app.test.js`:
- Around line 69-85: Update the tests for handleAppActivate to wrap app.isReady,
app.once, and MainWindow.show with jest.mocked() before calling mockReturnValue,
mockImplementation, or assertion methods, preserving the existing test behavior
and expectations.

🪄 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: 526855ce-195e-490c-89f7-9290266117b5

📥 Commits

Reviewing files that changed from the base of the PR and between dd4f35b and b6bc84b.

📒 Files selected for processing (7)

• src/app/mainWindow/mainWindow.ts
• src/app/system/tray/tray.ts
• src/app/views/MattermostWebContentsView.test.js
• src/app/views/MattermostWebContentsView.ts
• src/main/app/app.test.js
• src/main/app/app.ts
• src/main/app/initialize.ts

[coderabbitai]

🧹 Nitpick comments (1)

src/app/mainWindow/mainWindow.test.js (1)

589-590: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover the destroyed-window paths with a stable fixture.

jest.resetAllMocks() resets the isDestroyed implementation after each test, so later calls return undefined instead of modeling Electron’s boolean API. Reinitialize it in beforeEach() (or deliberately use jest.clearAllMocks()), add true-state tests for the new guards, and provide getContentBounds on baseWindow before testing getBounds().

Based on learnings, reset mock state deliberately when assertions depend on repeated test invocations.

🤖 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/mainWindow/mainWindow.test.js` around lines 589 - 590, Update the
mainWindow test setup so beforeEach reinitializes baseWindow.isDestroyed with a
boolean-returning implementation, or replace resetAllMocks with clearAllMocks
where appropriate. Add coverage for both destroyed-window guard paths, and
define baseWindow.getContentBounds before exercising getBounds().

Source: Learnings

🤖 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 `@src/app/mainWindow/mainWindow.test.js`:
- Around line 589-590: Update the mainWindow test setup so beforeEach
reinitializes baseWindow.isDestroyed with a boolean-returning implementation, or
replace resetAllMocks with clearAllMocks where appropriate. Add coverage for
both destroyed-window guard paths, and define baseWindow.getContentBounds before
exercising getBounds().

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: e6a50353-e092-4478-82c8-165f6f1436d9

📥 Commits

Reviewing files that changed from the base of the PR and between b6bc84b and c57a656.

📒 Files selected for processing (2)

• src/app/mainWindow/mainWindow.test.js
• src/app/mainWindow/mainWindow.ts