Handle errors of failed opening browser on Windows #840

nightnei · 2025-01-23T20:21:20Z

Related issues

Fixes https://github.com/Automattic/dotcom-forge/issues/9702

Proposed Changes

We had case when users could not open browser and they didn't see any errors.
With this PR if opening browser of IDE is failed - we render popup with error.

Testing Instructions

Run this branch on Windows
Install Chrome
Open any link from Studio and select Chrome as a default browser
Find Chrome.exe in "Program files" and rename it, so that Windows could not find it as an entry point for your default browser
Open Studio
Assert that click on the next links lead to popup with the error (screenshot below)
6.1 authenticate: "Sync" -> "Log in to WordPress.com" button
6.2 openSiteURL: "Import/Export" -> then import your backup -> at the end of importing you will see "Open site" button, click on it
6.3 openSiteURL: "WP admin" link in the header
6.4 openSiteURL:"Open site" link in the header
6.5 openSiteURL:"Open site" link in the header
6.6 openSiteURL:"Overview" -> Click on the preview (under "Theme" sub title)
6.7 openSiteURL:"Overview" -> "Site Editor" && "Styles" && "Patterns" && "Navigation" && "Templates" && "Pages"
6.8 openURL: in top right corner of the Studio's window click on (?)
6.9 openURL: "Import / Export" -> Click on "Learn more" in the tab description
6.10 "Menu bar" -> "Studio" -> "About Studio" -> Click on any link
6.11 "Menu bar" -> "Help" -> "Studio Help"
Install VS code -> Rerun Studio -> "Overview" -> Click on "VS Code button" -> Find VS Code .exe file and rename it -> Click again on VS Code in Studio's Overview tab -> Assert that you see VS code related error

nightnei · 2025-01-23T21:19:26Z

src/components/auth-provider.tsx

+		try {
+			await getIpcApi().authenticate();
+		} catch ( error ) {
+			getIpcApi().showErrorMessageBox( {
+				title: __( 'Failed to open browser' ),
+				error,
+			} );
+		}


I am thinking about creating HOF to avoid copy-paste, but in general it's an optional approach which I see.

Calling showErrorMessageBox directly from authenticate and openSiteURL seems like a good idea 👍 They're both part of ipc-handlers.ts, so it should hopefully be relatively straightforward.

fredrikekelund

I think this is a good approach 👍 Let's wrap it up and

fredrikekelund · 2025-01-24T10:10:25Z

src/utils/shellOpenExternalWrapper.ts

+const shouldMute = ( error: Error ) => {
+	return (
+		error.message.startsWith( 'Failed to open:' ) &&
+		( error.message.endsWith( '(0x483)' ) || error.message.endsWith( '(0x800401F5)' ) )


According to ChatGPT, the 0x483 error code indicates a different type of error. I'm not sure we should include it here…

The Windows error code 0x483 corresponds to the error message "ERROR_SWAPERROR." It indicates a system error related to the paging file or swap file. This error typically occurs when there is an issue accessing the paging file, which can be due to insufficient disk space, file corruption, or other disk-related issues. To resolve this error, you might need to check your disk for errors, ensure there is adequate free space, and verify that your system's virtual memory settings are configured correctly.

... file corruption... - I think it's the reason.
Also, I left comment with example above // "Failed to open: 没有应用程序与此操作的指定文件有关联。 (0x483)".
I suppose it's teh same error, but maybe in Windows with Japan localization. Since according to breadcrumbs in Sentry - it's the same button.
So I would keep this condition here.

I'm still not entirely convinced (0x483) is a good fit for muting in code… If we're uncertain, I would rather have the info in Sentry and mute/resolve as needed there.

fredrikekelund · 2025-01-24T10:11:15Z

src/utils/shellOpenExternalWrapper.ts

+		let message = '';
+		if ( url.startsWith( 'vscode://file/' ) ) {
+			message = 'Studio is unable to open VS Code.';
+		} else if ( url.startsWith( 'phpstorm://open?file=' ) ) {
+			message = 'Studio is unable to open PHPStorm.';
+		} else {
+			message = 'Studio is unable to open your default browser.';
+		}
+
+		message += 'Please ensure it is functioning correctly';


Suggested change

let message = '';

if ( url.startsWith( 'vscode://file/' ) ) {

message = 'Studio is unable to open VS Code.';

} else if ( url.startsWith( 'phpstorm://open?file=' ) ) {

message = 'Studio is unable to open PHPStorm.';

} else {

message = 'Studio is unable to open your default browser.';

}

message += 'Please ensure it is functioning correctly';

let message = '';

if ( url.startsWith( 'vscode://file/' ) ) {

message = __( 'Studio is unable to open VS Code. Please ensure it is functioning correctly' );

} else if ( url.startsWith( 'phpstorm://open?file=' ) ) {

message = __( 'Studio is unable to open PHPStorm. Please ensure it is functioning correctly' );

} else {

message = __( 'Studio is unable to open your default browser. Please ensure it is functioning correctly' );

}

Let's make the strings translatable.

fredrikekelund · 2025-01-24T10:12:15Z

src/lib/oauth.ts

 	const authUrl = `${ WP_AUTHORIZE_ENDPOINT }?response_type=token&client_id=${ CLIENT_ID }&redirect_uri=${ encodeURIComponent(
 		REDIRECT_URI
 	) }&scope=${ encodeURIComponent( SCOPES ) }`;
-	shell.openExternal( authUrl );
+
+	await shellOpenExternalWrapper( authUrl );


Is this a leftover? shellOpenExternalWrapper isn't async

No, it's async and we handle try/catch in the upper level

UPDATE: With my latest changes we don't need it

fredrikekelund · 2025-01-24T10:16:49Z

src/components/auth-provider.tsx

+		try {
+			await getIpcApi().authenticate();
+		} catch ( error ) {
+			getIpcApi().showErrorMessageBox( {
+				title: __( 'Failed to open browser' ),
+				error,
+			} );
+		}


Calling showErrorMessageBox directly from authenticate and openSiteURL seems like a good idea 👍 They're both part of ipc-handlers.ts, so it should hopefully be relatively straightforward.

nightnei · 2025-01-24T19:23:15Z

src/ipc-handlers.ts

 }

 export async function openURL( event: IpcMainInvokeEvent, url: string ) {
-	return shell.openExternal( url );
+	return shellOpenExternalWrapper( url );


I will remove this return and related conde on frontend on Monday, for consistency. We don't need it to be async function.

The same for authenticate function

fredrikekelund

LGTM 👍 I took the liberty of pushing a few small tweaks to avoid the additional back and forth, given that this has already been a gnarly issue.

The only remaining question I see is what to do about the (0x483) error. I'm not sure we want to mute it, since it seems only tangentially related. We can archive or resolve the issue in Sentry, but it might still be interesting to capture that data.

fredrikekelund · 2025-01-28T09:47:28Z

src/ipc-handlers.ts

@@ -639,8 +640,8 @@ export function logRendererMessage(
 	writeLogToFile( level, processId, ...args );
 }

-export async function authenticate( _event: IpcMainInvokeEvent ): Promise< void > {
-	return oauthClient.authenticate();
+export function authenticate( _event: IpcMainInvokeEvent ): void {


Suggested change

export function authenticate( _event: IpcMainInvokeEvent ): void {

export function authenticate( _event: IpcMainInvokeEvent ) {

Maybe someone disagrees with me, but I don't think it's necessary to explicitly add a void return type here.

fredrikekelund · 2025-01-28T09:49:48Z

src/utils/shellOpenExternalWrapper.ts

+// "Failed to open: Application not found (0x800401F5)"
+const shouldMute = ( error: Error ) => {
+	return (
+		error.message.startsWith( 'Failed to open:' ) &&


Suggested change

error.message.startsWith( 'Failed to open:' ) &&

As the error message can be in different languages, I think it's better to look just at the error code and disregard the natural language part of the message.

I was thinking about it and wanted to be more specific and mute as less as possible, so that we could be aware about other errors, is they appear in other languages.
This conditions which I added are specific for cases that we already have in Sentry. So I wanted to monitor Sentry in the future and update condition step by step.
Did you get what I mean and my thoughts? 😅

I understand, but I feel this strikes an odd balance between muting in code and submitting in Sentry. If we should mute anything in code, I would argue we should only look for the (0x800401F5) error code.

I would honestly be open to not muting anything in the code, too, if we think that's a clearer approach.

I see you point. Updated the condition to have only (0x800401F5).

I would honestly be open to not muting anything in the code, too, if we think that's a clearer approach.

Hm, didn't get it. From my perspective - we know about this specific issue with browser and if we not mute it, then it will just flood our logs in Sentry. It's the main point why I want to mute it. To avoid distraction of us with these errors.

From my perspective - we know about this specific issue with browser and if we not mute it, then it will just flood our logs in Sentry

I agree we want to mute it, but Sentry also has built-in tools to do that. I'm OK with doing it in code for this specific error, but principally, I think it's best to mute in Sentry rather than in code.

fredrikekelund · 2025-01-28T09:51:41Z

src/utils/shellOpenExternalWrapper.ts

+const shouldMute = ( error: Error ) => {
+	return (
+		error.message.startsWith( 'Failed to open:' ) &&
+		( error.message.endsWith( '(0x483)' ) || error.message.endsWith( '(0x800401F5)' ) )


I'm still not entirely convinced (0x483) is a good fit for muting in code… If we're uncertain, I would rather have the info in Sentry and mute/resolve as needed there.

fredrikekelund · 2025-01-28T11:48:18Z

src/utils/shellOpenExternalWrapper.ts

One more thing: let's move this file to src/lib and use kebab case for the file name (shell-open-external-wrapper.ts)

nightnei force-pushed the nightnei/handleBrokenBrowserOnWindows branch from 100f54b to 0384abc Compare January 23, 2025 20:27

nightnei changed the title ~~chore(electron): update to latest version 33.3.1~~ Handle error of failed opening browser on Windows Jan 23, 2025

nightnei changed the title ~~Handle error of failed opening browser on Windows~~ Handle errors of failed opening browser on Windows Jan 23, 2025

nightnei changed the title ~~Handle errors of failed opening browser on Windows~~ [WIP] Handle errors of failed opening browser on Windows Jan 23, 2025

nightnei commented Jan 23, 2025

View reviewed changes

nightnei requested a review from fredrikekelund January 23, 2025 21:19

fredrikekelund reviewed Jan 24, 2025

View reviewed changes

nightnei commented Jan 24, 2025

View reviewed changes

nightnei changed the title ~~[WIP] Handle errors of failed opening browser on Windows~~ Handle errors of failed opening browser on Windows Jan 24, 2025

fredrikekelund approved these changes Jan 28, 2025

View reviewed changes

fredrikekelund reviewed Jan 28, 2025

View reviewed changes

nightnei added 19 commits January 28, 2025 14:30

fix(windows): handle error of broken browser

e4e98ad

chore: move handling authentificate to lower level

c8d15b2

chore: rewrite other places

6e3530e

chore: revert

df987fa

chore: comment

7acbc46

chore: revert

a216128

menu bar

79f9ff8

refactor

589636b

chore: remove unused

2d024c7

chore: revert

200675a

chore: revert

8fbb490

chore: comment

2df4b81

chore: revert

315900e

chore: revert

549c670

chore: comment

e68b552

chore: revert

74dc3d3

chore: lint

4bf7b27

remove redundant promisses

7e5842d

remove latest try catch

b0abd43

fredrikekelund and others added 9 commits January 28, 2025 14:30

Fix test and lint error

8098ec8

Refine comments

4c5166a

String tweaks

30473ea

revert throw

b7d7e7a

chore: remove void

e9b02ac

fix condition for muting

9674139

chore: move module

e72e68b

chore: fix paths

6160381

chore: resolve conflicts

489dd90

nightnei force-pushed the nightnei/handleBrokenBrowserOnWindows branch from a02c647 to 489dd90 Compare January 28, 2025 14:32

nightnei merged commit d66b200 into trunk Jan 28, 2025
6 checks passed

nightnei deleted the nightnei/handleBrokenBrowserOnWindows branch January 28, 2025 14:45

bgrgicak pushed a commit that referenced this pull request Jan 29, 2025

Handle errors of failed opening browser on Windows (#840)

619bd3f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle errors of failed opening browser on Windows #840

Handle errors of failed opening browser on Windows #840

nightnei commented Jan 23, 2025 •

edited

Loading

nightnei Jan 23, 2025

fredrikekelund Jan 24, 2025

fredrikekelund left a comment

fredrikekelund Jan 24, 2025

nightnei Jan 24, 2025

fredrikekelund Jan 28, 2025

fredrikekelund Jan 24, 2025

fredrikekelund Jan 24, 2025

nightnei Jan 24, 2025 •

edited

Loading

nightnei Jan 24, 2025

fredrikekelund Jan 24, 2025

nightnei Jan 24, 2025

nightnei Jan 24, 2025

fredrikekelund left a comment

fredrikekelund Jan 28, 2025

fredrikekelund Jan 28, 2025

nightnei Jan 28, 2025

fredrikekelund Jan 28, 2025

nightnei Jan 28, 2025 •

edited

Loading

fredrikekelund Jan 28, 2025

fredrikekelund Jan 28, 2025

fredrikekelund Jan 28, 2025

nightnei Jan 28, 2025

	export function authenticate( _event: IpcMainInvokeEvent ): void {
	export function authenticate( _event: IpcMainInvokeEvent ) {

Handle errors of failed opening browser on Windows #840

Handle errors of failed opening browser on Windows #840

Conversation

nightnei commented Jan 23, 2025 • edited Loading

Related issues

Proposed Changes

Testing Instructions

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredrikekelund left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nightnei Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredrikekelund left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nightnei Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nightnei commented Jan 23, 2025 •

edited

Loading

nightnei Jan 24, 2025 •

edited

Loading

nightnei Jan 28, 2025 •

edited

Loading