Start proxy during site start and improve error wording by wojtekn · Pull Request #1089 · Automattic/studio

wojtekn · 2025-03-18T15:22:16Z

Related issues

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

Proposed Changes

A "Custom domain set up failed" error is displayed immediately after user starts Studio, even if the user doesn't use a custom domain. I propose to improve Proxy error wording to be more informative about what happened:

Also, I delay starting the proxy until the user tries to start a site that uses custom domains, so users who have port 80 busy and start Studio won't see error immediately.

Testing Instructions

Start a local web server app, e.g., Local
Start Studio
Confirm there is no error about custom domains displayed
Start a site that doesn't use custom domains
Confirm there is no error about custom domains displayed
Start a site that uses custom domains
Confirm that the error message is displayed and that the new wording is clear
Close Studio and other web server that uses 80 port
Repeat above steps and confirm there is no error message and that site is accessible using custom domain

Pre-merge Checklist

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

wojtekn · 2025-03-19T10:45:24Z

 				.on( 'error', ( err ) => {
-					console.log( err );
-					console.error( `Error starting proxy server on port 80:`, err );
+					console.error( `Error starting proxy server on port 80` );


Let's clean up the error logging, we are logging this error too many times.

wojtekn · 2025-03-19T10:45:57Z

-				buttons: [ __( 'OK' ) ],
-			} );
-			return false;
+			throw new Error( 'Studio failed to start the proxy server' );


Let's move the part responsible for dialog to the renderer.

It looks a bit odd that we throw a new exception for known error types but just return false for unknown error types. The known error types indicate that port 80 is busy, but other errors also indicate that Studio failed to start the proxy server.

I've updated that logic to handle error cases only through exceptions and cleaned up the logic around false.

Nice improvement 👍

bcotrim

Code LGTM, and a much better user experience 👍
Just FYI we have this related PR: #1090 there might be some code merging to do, and we should retest, depending on what is merged first.

My only suggestion would be to include unit tests for this scenario.

sejas

It works as expected. I confirm that the alert appears if port 80 is already in use. I also confirm that sites without a custom domain function as expected without any errors, even if port 80 is occupied. After freeing port 80, I was able to start two Studio sites with custom domains.

Great work!

Here are the logs when running two sites with custom domains:

Server stopped
Stopped watching for file changes
SQLite folder already exists. Skipping download.
Domain example-2.wp.local added to hosts file for port 8901
Proxy server started on port 80
Custom domain proxy server started successfully
Starting server for 'example 2'
directory: /Users/macbookpro/Studio/example-2
mode: wordpress
php: 8.2
wp: latest
WordPress latest folder already exists. Skipping download.
SQLite folder already exists. Skipping download.
Server running at http://example-2.wp.local
Server started for 'example 2'
Domain example.wp.local added to hosts file for port 8900
Custom domain proxy server started successfully
Starting server for 'example'
directory: /Users/macbookpro/Studio/example
mode: wordpress
php: 8.2
wp: latest
WordPress latest folder already exists. Skipping download.
SQLite folder already exists. Skipping download.
Server running at http://example.wp.local
Server started for 'example'

sejas · 2025-03-19T19:39:20Z

+					getIpcApi().showErrorMessageBox( {
+						title: __( 'Studio failed to initialize custom domains' ),
+						message: __(
+							'Studio needs to use port 80, but it’s already in use by another app. Close any local development apps and restart Studio.'


Should we mention that the port 80 is only necessary for custom domains?

The title says it failed to initialize custom domains, but it makes sense to clarify the message. I've added that in c82c902

sejas · 2025-03-19T19:48:44Z

+		try {
+			const proxyStarted = await startProxyServer();
+			if ( ! proxyStarted ) {
+				console.warn(
+					'Failed to start custom domain proxy server - custom domains will require port numbers'
+				);
+			}
+			console.log( 'Custom domain proxy server started successfully' );


No need to change anything. Just an observation. I noticed we display Custom domain proxy server started successfully each time we start a server with custom domain.
I think it's innocuous as we also display the Domain example.wp.local added to hosts file for port 8900

I removed redundant logging in 67bdbcf and moved the hosts log message to the proper place in f29028e

# Conflicts: # src/lib/proxy-server.ts

fredrikekelund · 2025-03-20T09:43:05Z

-				} );
+				if (
+					error instanceof Error &&
+					error.message.includes( 'Studio failed to start the proxy server' )


If this error only indicates that port 80 is busy, I would argue we should use a more specific error message.

I've improved wording in e06d062

Actually, as those are used internally to mitigate IPC handler that flattens our exceptions, we could change descriptive messages to codes e.g. PROXY_ERROR_80 or prepend messages with the prefix e.g. PROXY_ERROR_80: 'Studio failed to [...]. Thoughts?

I agree we could use a message like PROXY_ERROR_80, since this is only used internally 👍

wojtekn self-assigned this Mar 18, 2025

wojtekn changed the title ~~Improve Proxy error wording~~ Mar 18, 2025

wojtekn marked this pull request as ready for review March 19, 2025 10:36

wojtekn commented Mar 19, 2025

View reviewed changes

wojtekn requested a review from a team March 19, 2025 10:46

bcotrim approved these changes Mar 19, 2025

View reviewed changes

sejas approved these changes Mar 19, 2025

View reviewed changes

wojtekn added 8 commits March 20, 2025 10:02

Improve Proxy error wording

55b9e5b

Make message clearer

0f92c81

Move starting proxy from Studio start to site start

c287742

Move error dialog handling from proxy class to UI

b16f0be

# Conflicts: # src/lib/proxy-server.ts

Removed unnecessary import

dcec702

Remove reduntant logging

67bdbcf

Log host edit message in correct place

f29028e

Clarify error message

c82c902

wojtekn force-pushed the update/proxy-error-wording branch from dc5b1ee to c82c902 Compare March 20, 2025 09:23

fredrikekelund reviewed Mar 20, 2025

View reviewed changes

wojtekn added 3 commits March 20, 2025 13:12

Improve internal exception text wording

e06d062

Handle not known exceptions gracefully

26b885d

Replace internal exception messages with codes

5ac9609

wojtekn merged commit cf9a212 into trunk Mar 21, 2025

wojtekn deleted the update/proxy-error-wording branch March 21, 2025 09:29

fredrikekelund mentioned this pull request Mar 26, 2025

SSL Support: Hide all HTTPS help messages when the certificate is trusted #1117

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Start proxy during site start and improve error wording - #1089

Start proxy during site start and improve error wording#1089
wojtekn merged 11 commits into
trunkfrom
update/proxy-error-wording

wojtekn commented Mar 18, 2025 •

edited

Loading

wojtekn Mar 19, 2025

wojtekn Mar 19, 2025

fredrikekelund Mar 20, 2025

wojtekn Mar 20, 2025

fredrikekelund Mar 20, 2025

bcotrim left a comment •

edited

Loading

sejas left a comment

sejas Mar 19, 2025

wojtekn Mar 20, 2025

sejas Mar 19, 2025 •

edited

Loading

wojtekn Mar 20, 2025

fredrikekelund Mar 20, 2025

wojtekn Mar 20, 2025

wojtekn Mar 20, 2025

fredrikekelund Mar 20, 2025

Labels

4 participants

Uh oh!

Conversation

wojtekn commented Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related issues

Proposed Changes

Testing Instructions

Pre-merge Checklist

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

bcotrim left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

sejas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sejas Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Labels

4 participants

wojtekn commented Mar 18, 2025 •

edited

Loading

bcotrim left a comment •

edited

Loading

sejas Mar 19, 2025 •

edited

Loading