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.

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

Avoid creating main window before app initialization finishes #335

Merged
merged 2 commits into from
Jul 15, 2024

Conversation

fluiddot
Copy link
Contributor

@fluiddot fluiddot commented Jul 3, 2024

Related to 8016-gh-Automattic/dotcom-forge.

Proposed Changes

  • Add a variable to determine when the app finished initialization. This is used in app event handlers activate and second-instance to avoid creating the main window before the app initialization finishes.
  • Split the logic of setupWPServerFiles into two functions to avoid delaying the app initialization. One will handle the preparation of WordPress files by copying the bundled files. Since this is needed for creating sites, it will be run synchronously during the app initialization. The other function will try to update the server files (e.g. WordPress version). Depending on the internet connection, it might incur negatively in the bootup time so it will be executed asynchronously. This will speed up app initialization and create the main window as soon as possible.
  • Update unit tests that cover app initialization logic. Note that due to the PR changes, some unit tests have been converged into new ones.

Testing Instructions

macOS

  • Apply the following patch:
diff --git forkSrcPrefix/src/setup-wp-server-files.ts forkDstPrefix/src/setup-wp-server-files.ts
index 34d3e030f9a43566161f571cce4f0180aba2e1b5..07c174c22ea44daa182557de55a965d428087a0f 100644
--- forkSrcPrefix/src/setup-wp-server-files.ts
+++ forkDstPrefix/src/setup-wp-server-files.ts
@@ -62,6 +62,7 @@ export async function setupWPServerFiles() {
 	await copyBundledLatestWPVersion();
 	await copyBundledSqlite();
 	await copyBundledWPCLI();
+	await new Promise( ( resolve ) => setTimeout( resolve, 10_000 ) );
 }
 
 export async function updateWPServerFiles() {
  • Run the app with the command npm start.
  • Observe the main window is not created.
  • Click on the Studio app icon located in the docker.
  • Observe the main window is not created and no exceptions are produced.
  • Wait 10 seconds and observe the main window is created.

Windows

  • Apply the following patch:
diff --git forkSrcPrefix/src/setup-wp-server-files.ts forkDstPrefix/src/setup-wp-server-files.ts
index 34d3e030f9a43566161f571cce4f0180aba2e1b5..07c174c22ea44daa182557de55a965d428087a0f 100644
--- forkSrcPrefix/src/setup-wp-server-files.ts
+++ forkDstPrefix/src/setup-wp-server-files.ts
@@ -62,6 +62,7 @@ export async function setupWPServerFiles() {
 	await copyBundledLatestWPVersion();
 	await copyBundledSqlite();
 	await copyBundledWPCLI();
+	await new Promise( ( resolve ) => setTimeout( resolve, 10_000 ) );
 }
 
 export async function updateWPServerFiles() {
  • Generate the app binary by running the command npm run make.
  • Open the executable located at out/Studio-win32-x64/Studio.exe.
  • Observe the main window is not created.
  • Open a second time the executable.
  • Observe the main window is not created and no exceptions are produced.
  • Wait 10 seconds and observe the main window is created.
  • Check logs located in %APPDATA%/Studio/logs and observe the exception Error invoking remote method 'getAppGlobals' is not logged.

Pre-merge Checklist

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

@fluiddot fluiddot requested a review from a team July 3, 2024 11:33
@fluiddot fluiddot self-assigned this Jul 3, 2024
Copy link
Contributor

@kozer kozer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested it on Linux:

  1. Applied the diff
  2. Build the app
  3. Ran the app ( The main window don't appear )
  4. While waiting, ran a second instance
  5. Checked the logs. No errors!
    Nice work @fluiddot !

@fluiddot
Copy link
Contributor Author

fluiddot commented Jul 4, 2024

Thanks @kozer for testing the PR 🙇 ! The issue is mainly affecting macOS and Windows, so it would be great if someone else could check the changes in those platforms.

@fluiddot fluiddot requested a review from a team July 4, 2024 15:28
@kozer
Copy link
Contributor

kozer commented Jul 4, 2024

Thanks @kozer for testing the PR 🙇 ! The issue is mainly affecting macOS and Windows, so it would be great if someone else could check the changes in those platforms.

Yes sure! I am not expecting to merge it, as not all cases haven't been tested. I approved though, as for me it works! Thanks for mentioning this.

Copy link
Contributor

@wojtekn wojtekn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works as expected on macOS.

@fluiddot fluiddot merged commit db8841c into trunk Jul 15, 2024
11 checks passed
@fluiddot fluiddot deleted the fix/app-globals-not-defined branch July 15, 2024 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants