fix: Ensure non-destroyed window exists for authentication event handler by dcalhoun · Pull Request #86 · Automattic/studio

dcalhoun · 2024-05-03T15:36:18Z

Fixes https://github.com/Automattic/dotcom-forge/issues/6906.
Fixes #14.

Proposed Changes

Replace a cached main window reference with a more robust implementation of the
pre-existing withMainWindow utility used by the menu creation logic. The new
approach ensures a non-destroyed window is available whenever an event handler
attempts to reference it by accessing or creating the window within the event
handler, rather than using a main window reference that was passed when the
event handler was registered.

Testing Instructions

1. Authentication flow succeeds after closing and re-opening Studio's window

Launch Studio.
Close the Studio window.
Open a new Studio window (e.g., click the dock icon).
Complete the WPCOM authentication flow, allowing the browser to open the Studio app.
Verify the Studio app becomes focused and the correct authenticated state is displayed.

2. Authentication flow succeeds after quitting Studio

Important

This test case requires uninstalling all other Studio app installs before testing and using a build from npm run make.

Launch Studio.
Begin the WPCOM authentication flow, then quit the Studio app before continuing.
Complete the WPCOM authentication flow, allowing the browser to open the Studio app.
Verify the Studio app launches, becomes focused, and the correct authenticated state is displayed.

3. Studio menu items have not regressed

Verify the various menu items — Add Site..., Settings..., Close Window, etc — continue to function as expected both with and without an active Studio window open.

Pre-merge Checklist

Have you checked for TypeScript, React or other console errors?

Providing the cached main window reference at the time of event listener creation could result in the callback attempting to emit events on a window that had since been destroyed.

Currying is unnecessary as (1) we have no use for it currently and (2) this higher-order function will most likely always want to be invoked immediately to ensure the window reference provided is current and available.

This same logic is run in an event handler for the window `close` event, which makes this invocation unnecessary.

Registering the `open-url` event handler _before_ the app `ready` event, which is explicitly directed in the Electron, results in the URL handler correctly manages authentication on app start. https://www.electronjs.org/docs/latest/api/app#event-open-url-macos

The final scenario of `did-finish-load` did not return the actual window, so never returning the window is more consistent.

Use "set up" verb rather than "setup" noun.

There appears to be no reason to await the app `ready` event.

Intended to await the mock promise, not the mock function.

The CI server does not leverage the `darwin` platform, so we must mock the platform to verify the deep link handler.

dcalhoun · 2024-05-04T12:26:37Z

+		MAIN_WINDOW_WEBPACK_ENTRY: 'main-window-webpack-entry',
+		MAIN_WINDOW_PRELOAD_WEBPACK_ENTRY: 'main-window-preload-webpack-entry',


These additions mirror globals assigned by electron-forge's webpack build configuration.

dcalhoun · 2024-05-04T12:28:53Z


 	Menu.setApplicationMenu( null );

+	setupCustomProtocolHandler();


This was relocated out of the app ready event callback as it registers an event handler for the open-url event, which Electron's documentation explicitly directs not to register after the ready event.

dcalhoun · 2024-05-04T12:29:43Z


-		mainWindow = createMainWindow();
-		setupAuthCallbackHandler( mainWindow );
-		handleAuthOnStartup();


This manual authentication handling become unnecessary now that the open-url event handler is registered before the app ready event.

dcalhoun · 2024-05-04T12:32:14Z

+	if ( mainWindow && ! mainWindow.isDestroyed() ) {
+		return mainWindow;
+	}


This guard prevents reassigning mainWindow when it is already populated with a non-destroyed window to avoid creating lingering windows. If that is confusing for a method named createMainWindow, we might instead destroy the existing window.

dcalhoun · 2024-05-04T12:34:37Z

+/**
+ * Reset the main window reference. Exported for testing as resetting modules
+ * with Jest while preserving manual Electron mocks proved quite difficult.
+ */
+export function __resetMainWindow() {
+	mainWindow = null;
+}


This internal, testing-only function is less than ideal, but I tried a few different approaches (e.g, jest.doMock, jest.resetModules, jest.isolatedModules) all of which resulted in a shortcoming — mocking too late or breaking other manual package mocks.

dcalhoun · 2024-05-05T12:04:45Z


+	setupCustomProtocolHandler();
+
+	setUpAuthCallbackHandler();


This was relocated out of the app ready event callback as there is no longer a reason to await that event now that we do not reference mainWindow until within the event callback.

dcalhoun · 2024-05-06T12:50:55Z

+( fs as MockedFs ).__setFileContents( '/path/to/app/temp/com.wordpress.studio/', '' );
+
+it( 'should boot successfully', () => {
+	jest.isolateModules( () => {


The entry module includes side effects, so we must leverage isolateModules to reset/clear those side effects for each test case.

wojtekn

All cases work correctly. I didn't spot any regression issues.

sentry · 2024-05-14T14:38:12Z

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

‼️ Error: Cannot create BrowserWindow before app is ready createMainWindow(main/studio/./src/main-window) View Issue

_{Did you find this useful? React with a 👍 or 👎}

## Related issues - Fixes [STU-1750](https://linear.app/a8c/issue/STU-1750/studio-reopens-multiple-times-when-quitting-during-site-startup) ## How AI was used in this PR Claude Code investigated the bug, identified the root cause (create-if-missing branch in `getMainWindow()` racing with async IPC fan-out during the `will-quit` shutdown window), proposed the minimal gate at `sendIpcEventToRenderer`, and applied the change. I reviewed the diff and confirmed both the fix and its placement before committing. ## Proposed Changes - Add an `isAppQuitting` flag in `apps/studio/src/ipc-utils.ts` with an exported `markAppQuitting()` setter. While set, `sendIpcEventToRenderer` returns early without calling `getMainWindow()`. - Call `markAppQuitting()` at the start of the `will-quit` handler in `apps/studio/src/index.ts`, before any other cleanup. ### Why this works When the user confirms "Stop sites" on quit, `will-quit` calls `event.preventDefault()` and waits up to 6 s for `stopAllServers()` to drain. During that window, in-flight async callbacks (site-startup events, snapshot events, sync progress, etc.) keep firing through `sendIpcEventToRenderer()`. Each call routes through `getMainWindow()` — and because the original window has typically already been closed by Electron's shutdown sequence, `getMainWindow()` falls through to `createMainWindow()` and spawns a fresh `BrowserWindow`. One pending event = one ghost window. With ~6 sites starting simultaneously on a Linux VM, you reliably see ~6 windows reopen. Blocking the fan-out at the `sendIpcEventToRenderer` chokepoint is the smallest fix: it catches every async-IPC path without changing `getMainWindow`'s signature or touching its 15+ other call sites (menus, deeplinks, update dialog, etc.), which are user-triggered and can't fire post-`will-quit` anyway. `will-quit` (not `before-quit`) is the right gate point: `before-quit` can still be cancelled by the sync-in-progress dialog or the "Cancel" button on the running-sites dialog. Once `will-quit` fires, the app is committed. ### Regression origin The mechanic was introduced in #86 (May 2024) for OAuth deep-link handling. It became today's user-visible bug in #849 (Jan 2025), which routed `sendIpcEventToRenderer` through `getMainWindow()`. The bug got more reproducible recently due to Linux VM testing (slower shutdown widens the race window) plus PR #3350 removing an accidental escape hatch that bypassed the quit dialog when the CLI wasn't installed. ## Testing Instructions Reproduce on `trunk` first, then verify the fix on this branch. **Repro on trunk (Linux/Windows easier; macOS rarer):** 1. Have several Studio sites created (~6 makes it obvious). 2. Quit Studio so all sites stop. 3. Re-launch Studio. 4. Start all sites at once. 5. **Before** they finish booting, quit Studio. 6. Observe: Studio reopens 4–6 times in quick succession. **Verify on this branch:** 1. Repeat steps 1–5 above. 2. Studio should quit cleanly with no ghost windows. ## Pre-merge Checklist - [x] Have you checked for TypeScript, React or other console errors? - [x] Manual repro on Linux (with several sites) - [x] Manual repro on Windows (with several sites)

dcalhoun added the [Type] Bug label May 3, 2024

dcalhoun self-assigned this May 3, 2024

dcalhoun force-pushed the fix/ensure-non-destroyed-window-exists-for-authentication-event-handler branch 4 times, most recently from 988b450 to 44078d9 Compare May 5, 2024 12:03

dcalhoun added 11 commits May 6, 2024 07:06

fix: Ensure non-destroyed window exists for authentication event handler

6394285

Providing the cached main window reference at the time of event listener creation could result in the callback attempting to emit events on a window that had since been destroyed.

refactor: Remove unnecessary currying

47f4429

Currying is unnecessary as (1) we have no use for it currently and (2) this higher-order function will most likely always want to be invoked immediately to ensure the window reference provided is current and available.

refactor: Remove duplicative menu nullifying

21a9245

This same logic is run in an event handler for the window `close` event, which makes this invocation unnecessary.

refactor: Return void from withMainWindow

a4dfb4d

The final scenario of `did-finish-load` did not return the actual window, so never returning the window is more consistent.

test: Assert withMainWindow utility

69dd5ef

refactor: Fix setupAuthCallbackHandler name typo

9d7a4e4

Use "set up" verb rather than "setup" noun.

refactor: Hoist authentication event handler setup

0a17054

There appears to be no reason to await the app `ready` event.

test: Assert entry module

7bbf770

test: Assert oauth utility

4ed878b

test: Fix typo

3361577

Intended to await the mock promise, not the mock function.

dcalhoun force-pushed the fix/ensure-non-destroyed-window-exists-for-authentication-event-handler branch from f00af0a to 3361577 Compare May 6, 2024 11:06

test: Mock process platform

13db766

The CI server does not leverage the `darwin` platform, so we must mock the platform to verify the deep link handler.

dcalhoun force-pushed the fix/ensure-non-destroyed-window-exists-for-authentication-event-handler branch from 31af4e0 to 13db766 Compare May 6, 2024 12:48

dcalhoun commented May 6, 2024

View reviewed changes

dcalhoun marked this pull request as ready for review May 6, 2024 12:55

dcalhoun requested a review from a team May 6, 2024 12:56

wojtekn approved these changes May 7, 2024

View reviewed changes

dcalhoun merged commit a342b0c into trunk May 7, 2024

dcalhoun deleted the fix/ensure-non-destroyed-window-exists-for-authentication-event-handler branch May 7, 2024 11:51

This was referenced May 9, 2024

test: Simplify mocked oauth promise await #107

Merged

Redirect Studio app users to magic login flow when signing up Automattic/wp-calypso#90204

Merged

ivan-ottinger mentioned this pull request May 25, 2026

Prevent ghost windows when quitting during site startup #3608

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: Ensure non-destroyed window exists for authentication event handler - #86

fix: Ensure non-destroyed window exists for authentication event handler#86
dcalhoun merged 12 commits into
trunkfrom
fix/ensure-non-destroyed-window-exists-for-authentication-event-handler

dcalhoun commented May 3, 2024 •

edited

Loading

dcalhoun May 4, 2024

dcalhoun May 4, 2024

dcalhoun May 4, 2024

dcalhoun May 4, 2024

dcalhoun May 4, 2024

dcalhoun May 5, 2024

Uh oh!

dcalhoun May 6, 2024

wojtekn left a comment

sentry Bot commented May 14, 2024

Labels

3 participants

		MAIN_WINDOW_WEBPACK_ENTRY: 'main-window-webpack-entry',
		MAIN_WINDOW_PRELOAD_WEBPACK_ENTRY: 'main-window-preload-webpack-entry',


		Menu.setApplicationMenu( null );

		setupCustomProtocolHandler();

Uh oh!

Conversation

dcalhoun commented May 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Testing Instructions

Pre-merge Checklist

dcalhoun May 4, 2024

Choose a reason for hiding this comment

dcalhoun May 4, 2024

Choose a reason for hiding this comment

dcalhoun May 4, 2024

Choose a reason for hiding this comment

dcalhoun May 4, 2024

Choose a reason for hiding this comment

dcalhoun May 4, 2024

Choose a reason for hiding this comment

dcalhoun May 5, 2024

Choose a reason for hiding this comment

Uh oh!

dcalhoun May 6, 2024

Choose a reason for hiding this comment

wojtekn left a comment

Choose a reason for hiding this comment

sentry Bot commented May 14, 2024

Suspect Issues

Labels

3 participants

dcalhoun commented May 3, 2024 •

edited

Loading