Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Avoid creating main window before app initialization finishes #335

Merged
merged 2 commits into from
Jul 15, 2024
Merged

Avoid creating main window before app initialization finishes #335

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
da2b50b
Avoid creating main window before app initializes
fluiddot Jul 3, 2024
91201f0
Update app unit tests
fluiddot Jul 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 20 additions & 6 deletions src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import {
migrateFromWpNowFolder,
needsToMigrateFromWpNowFolder,
} from './migrations/migrate-from-wp-now-folder';
import setupWPServerFiles from './setup-wp-server-files';
import { setupWPServerFiles, updateWPServerFiles } from './setup-wp-server-files';
import { stopAllServersOnQuit } from './site-server';
import { loadUserData } from './storage/user-data'; // eslint-disable-next-line import/order
import { setupUpdates } from './updates';
Expand All @@ -54,6 +54,8 @@ const isInInstaller = require( 'electron-squirrel-startup' );
// Ensure we're the only instance of the app running
const gotTheLock = app.requestSingleInstanceLock( getCLIDataForMainInstance() );

let finishedInitialization = false;

if ( gotTheLock && ! isInInstaller ) {
if ( isCLI() ) {
processCLICommand( { mainInstance: true, appBoot } );
Expand Down Expand Up @@ -171,6 +173,10 @@ async function appBoot() {
} else {
// Handle custom protocol links on Windows and Linux
app.on( 'second-instance', ( _event, argv ): void => {
if ( ! finishedInitialization ) {
return;
}

withMainWindow( ( mainWindow ) => {
// CLI commands are likely invoked from other apps, so we need to avoid changing app focus.
const isCLI = argv?.find( ( arg ) => arg.startsWith( '--cli=' ) );
Expand Down Expand Up @@ -246,14 +252,16 @@ async function appBoot() {
} );
} );

setupIpc();

await setupWPServerFiles().catch( Sentry.captureException );
// WordPress server files are updated asynchronously to avoid delaying app initialization
updateWPServerFiles().catch( Sentry.captureException );

if ( await needsToMigrateFromWpNowFolder() ) {
await migrateFromWpNowFolder();
}

setupIpc();

createMainWindow();

// Handle CLI commands
Expand All @@ -270,6 +278,8 @@ async function appBoot() {
bumpStat( 'studio-app-launch-total', process.platform );
// Bump stat for unique weekly app launch, approximates weekly active users
bumpAggregatedUniqueStat( 'local-environment-launch-uniques', process.platform, 'weekly' );

finishedInitialization = true;
} );

// Quit when all windows are closed, except on macOS. There, it's common
Expand All @@ -290,10 +300,14 @@ async function appBoot() {
} );

app.on( 'activate', () => {
// On OS X it's common to re-create a window in the app when the
// dock icon is clicked and there are no other windows open.
if ( ! finishedInitialization ) {
return;
}

if ( BrowserWindow.getAllWindows().length === 0 ) {
app.whenReady().then( createMainWindow ).catch( Sentry.captureException );
// On OS X it's common to re-create a window in the app when the
// dock icon is clicked and there are no other windows open.
createMainWindow();
}
} );
}
5 changes: 4 additions & 1 deletion src/setup-wp-server-files.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,13 @@ async function copyBundledWPCLI() {
const bundledWPCLIPath = path.join( getResourcesPath(), 'wp-files', 'wp-cli', 'wp-cli.phar' );
await fs.copyFile( bundledWPCLIPath, getWpCliPath() );
}
export default async function setupWPServerFiles() {
export async function setupWPServerFiles() {
await copyBundledLatestWPVersion();
await copyBundledSqlite();
await copyBundledWPCLI();
}

export async function updateWPServerFiles() {
await updateLatestWordPressVersion();
await updateLatestSqliteVersion();
await updateLatestWPCliVersion();
Expand Down
194 changes: 96 additions & 98 deletions src/tests/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,19 @@
* @jest-environment node
*/
import fs from 'fs';
import { createMainWindow } from '../main-window';
import setupWPServerFiles from '../setup-wp-server-files';
import { createMainWindow, withMainWindow } from '../main-window';
import { setupWPServerFiles } from '../setup-wp-server-files';

jest.mock( 'fs' );
jest.mock( 'file-stream-rotator' );
jest.mock( '../main-window' );
jest.mock( '../updates' );
jest.mock( '../lib/bump-stats' );
jest.mock( '../setup-wp-server-files', () =>
jest.fn( () => new Promise< void >( ( resolve ) => resolve() ) )
);
jest.mock( '../lib/cli' );
jest.mock( '../setup-wp-server-files', () => ( {
setupWPServerFiles: jest.fn( () => Promise.resolve() ),
updateWPServerFiles: jest.fn( () => Promise.resolve() ),
} ) );

const mockUserData = {
sites: [],
Expand All @@ -23,6 +25,46 @@ const mockUserData = {
);
( fs as MockedFs ).__setFileContents( '/path/to/app/temp/com.wordpress.studio/', '' );

function mockElectron(
{
appEvents,
appMocks,
electronMocks,
}: {
appEvents: string[];
electronMocks?: Partial< typeof import('electron') >;
appMocks?: Partial< typeof import('electron').app >;
} = {
appEvents: [],
}
) {
const mockedEvents = appEvents.reduce< Record< string, ( ...args: any[] ) => Promise< void > > >(
( accum, event ) => {
// eslint-disable-next-line @typescript-eslint/no-empty-function
return { ...accum, [ event ]: async () => {} };
},
{}
);
jest.doMock( 'electron', () => {
const electron = jest.genMockFromModule( 'electron' ) as typeof import('electron');
return {
...electron,
...electronMocks,
app: {
...electron.app,
...appMocks,
on: jest.fn( ( event, callback ) => {
const mockedEventName = Object.keys( mockedEvents ).find( ( key ) => key === event );
if ( mockedEventName ) {
mockedEvents[ mockedEventName ] = callback;
}
} ),
},
};
} );
return { mockedEvents };
}

afterEach( () => {
jest.clearAllMocks();
} );
Expand All @@ -37,26 +79,16 @@ it( 'should handle authentication deep links', () => {
jest.isolateModules( async () => {
const originalProcessPlatform = process.platform;
Object.defineProperty( process, 'platform', { value: 'darwin' } );
// eslint-disable-next-line @typescript-eslint/no-empty-function
let openUrl: ( ...args: any[] ) => void = () => {};
const mockIpcMainEmit = jest.fn();
jest.doMock( 'electron', () => {
const electron = jest.genMockFromModule( 'electron' ) as typeof import('electron');
return {
...electron,
const electron = jest.genMockFromModule( 'electron' ) as typeof import('electron');
const { mockedEvents } = mockElectron( {
appEvents: [ 'open-url' ],
electronMocks: {
ipcMain: {
...electron.ipcMain,
emit: mockIpcMainEmit,
},
app: {
...electron.app,
on: jest.fn( ( event, callback ) => {
if ( event === 'open-url' ) {
openUrl = callback;
}
} ),
},
};
},
} );
const mockAuthResult = { email: 'mock-email', displayName: 'mock-display-name' };
const mockResolvedValue = Promise.resolve( mockAuthResult );
Expand All @@ -66,6 +98,7 @@ it( 'should handle authentication deep links', () => {
handleAuthCallback: mockHandleAuthCallback,
} ) );
require( '../index' );
const { 'open-url': openUrl } = mockedEvents;

const mockHash = '#access_token=1234&expires_in=1';
openUrl( {}, `wpcom-local-dev://auth${ mockHash }` );
Expand All @@ -80,104 +113,69 @@ it( 'should handle authentication deep links', () => {
} );
} );

it( 'should await the app ready state before creating a window for activate events', async () => {
it( 'should setup server files before creating main window', async () => {
await jest.isolateModulesAsync( async () => {
// eslint-disable-next-line @typescript-eslint/no-empty-function
let activate: ( ...args: any[] ) => void = () => {};
jest.doMock( 'electron', () => {
const electron = jest.genMockFromModule( 'electron' ) as typeof import('electron');
return {
...electron,
app: {
...electron.app,
whenReady: jest.fn( () => Promise.resolve() ),
on: jest.fn( ( event, callback ) => {
if ( event === 'activate' ) {
activate = callback;
}
} ),
},
};
} );
const { mockedEvents } = mockElectron( { appEvents: [ 'ready' ] } );
require( '../index' );
const { ready } = mockedEvents;

activate();
// Add a mock function to check that `setupWPServerFiles` is resolved before
// creating the main window.
const resolveFn = jest.fn();
( setupWPServerFiles as jest.Mock ).mockImplementation( async () => {
await new Promise( process.nextTick );
resolveFn();
} );

await ready();

expect( createMainWindow ).not.toHaveBeenCalled();
// Await the mocked `whenReady` promise resolution
await new Promise( process.nextTick );
expect( createMainWindow ).toHaveBeenCalled();
expect( resolveFn ).toHaveBeenCalled();
const setupWPServerFilesResolvedOrder = resolveFn.mock.invocationCallOrder[ 0 ];
const createMainWindowOrder = ( createMainWindow as jest.Mock ).mock.invocationCallOrder[ 0 ];

expect( setupWPServerFilesResolvedOrder ).toBeLessThan( createMainWindowOrder );
} );
} );

it( 'should gracefully handle app ready failures when creating a window on activate', async () => {
it( 'should wait app initialization before creating main window via activate event', async () => {
await jest.isolateModulesAsync( async () => {
// eslint-disable-next-line @typescript-eslint/no-empty-function
let activate: ( ...args: any[] ) => void = () => {};
jest.doMock( 'electron', () => {
const electron = jest.genMockFromModule( 'electron' ) as typeof import('electron');
return {
...electron,
app: {
...electron.app,
whenReady: jest.fn( () => Promise.reject() ),
on: jest.fn( ( event, callback ) => {
if ( event === 'activate' ) {
activate = callback;
}
} ),
},
};
} );
const captureExceptionMock = jest.fn();
jest.doMock( '@sentry/electron/main', () => ( {
init: jest.fn(),
captureException: captureExceptionMock,
} ) );
const { mockedEvents } = mockElectron( { appEvents: [ 'ready', 'activate' ] } );

require( '../index' );
const { ready, activate } = mockedEvents;

await activate();
expect( createMainWindow as jest.Mock ).not.toHaveBeenCalled();

activate();
await ready();

await new Promise( process.nextTick );
expect( createMainWindow ).not.toHaveBeenCalled();
expect( captureExceptionMock ).toHaveBeenCalled();
await activate();
expect( createMainWindow as jest.Mock ).toHaveBeenCalled();
} );
} );

it( 'should setup server files before creating main window', async () => {
it( 'should wait app initialization before creating main window via second-instance event', async () => {
await jest.isolateModulesAsync( async () => {
// eslint-disable-next-line @typescript-eslint/no-empty-function
let ready: ( ...args: any[] ) => Promise< void > = async () => {};
jest.doMock( 'electron', () => {
const electron = jest.genMockFromModule( 'electron' ) as typeof import('electron');
return {
...electron,
app: {
...electron.app,
on: jest.fn( ( event, callback ) => {
if ( event === 'ready' ) {
ready = callback;
}
} ),
},
};
} );
const { mockedEvents } = mockElectron( { appEvents: [ 'ready', 'second-instance' ] } );

// The "second-instance" event is only invoked on Windows/Linux platforms.
// Therefore, we ensure the initialization is performed on one of those
// platforms.
const originalProcessPlatform = process.platform;
Object.defineProperty( process, 'platform', { value: 'win32' } );

require( '../index' );
const { ready, 'second-instance': secondInstance } = mockedEvents;

// Add a mock function to check that `setupWPServerFiles` is resolved before
// creating the main window.
const resolveFn = jest.fn();
( setupWPServerFiles as jest.Mock ).mockImplementation( async () => {
await new Promise( process.nextTick );
resolveFn();
} );
await secondInstance();
// "withMainWindow" creates the main window if it doesn't exist
expect( withMainWindow as jest.Mock ).not.toHaveBeenCalled();

await ready();

expect( resolveFn ).toHaveBeenCalled();
const setupWPServerFilesResolvedOrder = resolveFn.mock.invocationCallOrder[ 0 ];
const createMainWindowOrder = ( createMainWindow as jest.Mock ).mock.invocationCallOrder[ 0 ];
await secondInstance();
expect( withMainWindow as jest.Mock ).toHaveBeenCalled();

expect( setupWPServerFilesResolvedOrder ).toBeLessThan( createMainWindowOrder );
Object.defineProperty( process, 'platform', { value: originalProcessPlatform } );
} );
} );
Loading