fix: Improve site creation error handling #4

dcalhoun · 2024-04-22T12:35:00Z

Related to https://github.com/Automattic/dotcom-forge/issues/6590.

Proposed Changes

Hoist site creation error handling from the remote method to the renderer to ensure all errors originating from the remote method are caught and reported so that we may monitor escalations.
Use a generic, user-friendly error message during site creation to avoid displaying a highly technical error message to users.

Testing Instructions

Create site from populated, non-WordPress directory

Add a site via onboarding or the add site modal.
Select an empty directory.
Open your file browser and place a folder/file in the directory.
Continue the site creation.
Verify a user-friendly message is displayed.

Reset a custom local path to the proposed path

Add a site via onboarding or the add site modal.
Select a non-empty, non-WordPress directory.
Verify a user-friendly error message is displayed.
Select the default site directory path (e.g., ~/Studio on macOS).
Modify the site name by adding words.
Verify the local path is set to the proposed path with modifications included, and continuing site creations succeeds.

Before	After

Pre-merge Checklist

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

Basedd on the surrounding code, the lack of a return appears to be an oversight. Introducing the return means creating a site in a non-empty, non-WordPress directory no longer displays an error, and instead dismisses the modal after moving the directory's content to the OS trash. The apparent rationale for doing so is allowing the user to retry with a clean directory. A future improvement would be communicating the non-empty state to the user, allowing them to take whatever action they deem appropriate.

The flex layout caused the element to shrink.

The error messages provided from the remote process can be highly technical and unhelpful. To start, they are all prefixed with confusing technical details: > Error invoking remote method <name> Also, the error origin can be very technical itself, e.g.: > Error: ENOENT: no such file or directory, scandir '/Users/ajayghaghretiya/Library/Application Support/Studio/server-files/wordpress-versions/latest/wp-content/themes/twentytwentythree/parts' To improve the user experience, we provide a generic, user-friendly message instead, regardless of the origin.

Site creation silently failing is confusing. Instead, we should communicate the error and ask that they retry.

Ensure we can monitor escalation of this type of error.

Site creation silently failing is confusing. Instead, we should communicate the error and ask that they retry.

This is no longer presented to the end-user. We can instead rely upon the original error message.

Surrounding logic does not rely upon the `err` abbreviation.

Hoisting the `try`/`catch` to the main process invocation site ensures that all errors originating from logic within the main process method are caught. Previously, only certain portions of the logic were wrapped in `try`/`catch`, meaning some errors would go unreported.

Allow selecting the default site path (e.g., `~/Studio` on macOS) as a means of resetting prior path selections. This allows users to recover from mistakenly changing the local path to a non-empty, non-WordPress directory.

dcalhoun · 2024-04-22T14:36:03Z

src/components/site-form.tsx

 				>
 					<Icon
-						className={ error ? 'fill-red-500' : '' }
+						className={ cx( 'shrink-0 basis-4', error ? 'fill-red-500' : '' ) }


Prevent form validation message icons from shrinking due to sibling content.

dcalhoun · 2024-04-22T14:37:15Z

src/hooks/use-add-site.ts

-			setSitePath( path );
+			const pathResetToDefaultSitePath =
+				path === proposedSitePath.substring( 0, proposedSitePath.lastIndexOf( '/' ) );
+			setSitePath( pathResetToDefaultSitePath ? '' : path );


Allow users to reset the path selection value by selecting the default Studio directory. The outcome is using the currently proposed path instead, including any future changes to the site name.

dcalhoun · 2024-04-22T14:41:03Z

src/hooks/use-add-site.ts

 			await createSite( path, siteName ?? '' );
 		} catch ( e ) {
-			setError( ( e as Error )?.message );
+			Sentry.captureException( e );


Error reporting was hoisted here from src/ipc-handlers.ts to ensure all errors originating from the remote createSite method were caught. Previously, portions of the remote method could result in displaying error messages, but not reporting them to Sentry — e.g, the error shared in https://github.com/Automattic/dotcom-forge/issues/6590 — as we did not wrap all logic in createSite with try/catch blocks.

dcalhoun · 2024-04-22T14:44:54Z

src/hooks/use-add-site.ts

+			setError(
+				__(
+					'An error occurred while creating the site. Verify your selected local path is an empty directory or an existing WordPress folder and try again. If this problem persists, please contact support.'
+				)
+			);


A generic error message here replaces displaying the previous error messages thrown from src/ipc-handlers.ts. This was done for a few reasons:

All error messages originating from remote methods are prefixed with a non-user-friendly message of Error invoking remote method <method-name>. We could remove this with regular expression, but...

Error messages from remote messages could originate from numerous failures, including references to highly technical details as outlined in Automattic/dotcom-forge#6590. It is challenging to filter out these "unknown" errors aside from parsing the error message string because...

Electron does not provide an ability to communicate custom errors across the bridge (quoted below) aside from the message itself. Parsing the message to determine known vs unknown errors feels brittle.

Errors thrown through handle in the main process are not transparent as they are serialized and only the message property from the original error is provided to the renderer process. Please refer to #24427 for details.

dcalhoun · 2024-04-22T14:54:58Z

src/ipc-handlers.ts

 	if ( ! ( await isEmptyDir( path ) ) && ! isWordPressDirectory( path ) ) {
-		wasPathEmpty = true;


The combination of ! ( await isEmptyDir( path ) ) and setting wasPathEmpty = true; appears contradictory. Additionally, now that all error handling is hoisted to the renderer, I removed this assignment and related logic.

@sejas are you able to share the original intent behind this logic (when and why it should run) and whether its removal will cause problems? 🙇🏻

I shared additional thoughts in https://github.com/Automattic/local-environment/pull/135#discussion_r1571371185.

I agree that it is contradictory.
The intention was when creating a site failed, we should remove all the new files created to allow the user to recreate it again with the same folder name.

dcalhoun · 2024-04-22T14:56:41Z

src/ipc-handlers.ts


 	if ( ! ( await isEmptyDir( path ) ) && ! isWordPressDirectory( path ) ) {
-		wasPathEmpty = true;
-		userData.sites;


@kozer I noted this one reference to userData.sites did not have a return statement like all others in this method. I presumed this was oversight.

Are you able to share the original intent of returning userData.sites? Will this removal cause issue? 🙇🏻

I shared additional thoughts in https://github.com/Automattic/local-environment/pull/55#discussion_r1571371113.

sejas

Tested both flows where I added a folder inside the empty folder and I received the error message 👌
After selecting Studio, the name selection worked fine.

Thanks for this PR, this kind of PRs are my favorite. Removing code and simplifying the logic. Congrats!

I think the cases were the process of creating a site could fail were reduced almost to zero, so I think we don't need the wasEmpty logic anymore.

creating-site.mp4

sejas · 2024-04-23T19:52:53Z

src/ipc-handlers.ts

-			if ( wasPathEmpty ) {
-				// Clean the path to let the user try again
-				await shell.trashItem( path );
-			}


Here is where we removed all the files after a failed site creation in an empty directory.
I'm seeing you have removed the try/catch so I guess we assume the site creation is not likely to fail.

I'm seeing you have removed the try/catch so I guess we assume the site creation is not likely to fail.

No, not necessarily. I believe site creation could still fail, the primary focus of the proposed changes is hoisting error handling higher in the stack so that all errors that might occur in createSite method are caught.

However, as you noted, I also removed the clean up logic as I am not confident it functions as expected.

Currently, I'm perplexed by the original logic, which is why I inquired about its original intent in #4 (comment). It could be that we need to retain this logic — I'm not sure. I'd like your personal perspective on the logic's intent/necessity and the overall stability of this new site creation logic.

Here is where we removed all the files after a failed site creation in an empty directory.

Correct. I am not sure when this logic would actually execute. We set wasPathEmpty = true; when the directory is a non-empty, non-WP directory, but then only run the clean up logic when the path is a WP directory. That appears contradictory to me and I'm not sure in what circumstance that situation would be possible.

I think the cases were the process of creating a site could fail were reduced almost to zero, so I think we don't need the wasEmpty logic anymore.

Copied this quote from your top-level review comment as I felt it related to this specific discussion as well. I'm not confident it will never fail, but my hope is that we will at least catch any errors that occur and provide a helpful error message.

Thank you for helping me understand this logic! It's definitely an important piece of functionality. 🙇🏻

Noting that a recent report (ref: pdKhl6-3bi-p2#comment-6383) suggests site creation can fail leaving a partial WP directory installation. My perception is that the clean up logic intended to help rectify such a situation. However, as I mentioned in #4 (comment), I lack confidence that it works as expected and I wonder if we should prompt for user permission before taking this action, as removing a directory might inadvertently discard valuable user files.

@sejas following up to see I you have additional insight on the findings I expressed in my previous comments.

Completely fine if we need more time to review this. I'd prefer to understand the prior logic's intention before removing it — I'd hate to introduce bugs or make the UX less stable.

Thanks in advance for the help! 🙇🏻

@dcalhoun, Ideally we could keep the logic of deleting the site if the creation fails.

I wonder if we should prompt for user permission before taking this action, as removing a directory might inadvertently discard valuable user files.

That's why I introduced the wasEmpty flag. We should wrap it around a correct if statement that correctly checks if the given folder is empty. Then we can assume the user doesn't have any "custom" files in that folder, and all the files in that folder are created by our app. Also note that the app moves that folder into the trash and don't really remove it from the disk.

@sejas I interpret your latest comment to mean the logic's intention was to clean up only app-generated files after failed attempts to create a new site in an empty directory, but that I am correct in that it currently does not function as intended because of the conflicting conditionals I referenced in #4 (comment). You believe it is ideal to retain the clean up logic, provided we fix the currently broken state.

Is all of that interpretation accurate? If so, I will work to reinstate the clean up logic.

Thanks for your patience, I repeat all this back to avoid any misunderstandings.

Yes!, all that is correct.

We can also re-introduce the correct condition, ^{_{"clean up the directory when the create fails and all the files were created by Studio"}}, in a separate PR if you prefer that.

Merged these changes as-is and created #68 to follow up on this specific discussion. Thanks!

dcalhoun added the bug label Apr 22, 2024

dcalhoun self-assigned this Apr 22, 2024

dcalhoun added 11 commits April 22, 2024 10:26

fix: Correct error icon size

2092e55

The flex layout caused the element to shrink.

fix: Notify users of site creation failure due to invalid local path

e5b4f50

Site creation silently failing is confusing. Instead, we should communicate the error and ask that they retry.

fix: Report failed working directory prep

dca992e

Ensure we can monitor escalation of this type of error.

fix: Notify users of site creation failure due to directory creation

690e9f2

Site creation silently failing is confusing. Instead, we should communicate the error and ask that they retry.

refactor: Remove unnecessary bespoke error message

1e5e023

This is no longer presented to the end-user. We can instead rely upon the original error message.

refactor: Rename error argument for consistency

b9794de

Surrounding logic does not rely upon the `err` abbreviation.

test: Assert add site error message

e3efda7

dcalhoun force-pushed the fix/improve-site-creation-error-handling branch from 11397ce to e3efda7 Compare April 22, 2024 14:27

dcalhoun marked this pull request as ready for review April 22, 2024 15:08

dcalhoun requested review from a team, kozer and sejas April 22, 2024 15:11

dcalhoun commented Apr 22, 2024

View reviewed changes

sejas approved these changes Apr 23, 2024

View reviewed changes

dcalhoun merged commit f4632cf into trunk Apr 30, 2024

dcalhoun deleted the fix/improve-site-creation-error-handling branch April 30, 2024 13:55

dcalhoun mentioned this pull request Apr 30, 2024

fix: Failed site creation resets the directory #68

Merged

1 task

This was referenced Nov 10, 2025

Add toggle to settings modal to un-/install CLI #2041

Open

Add pm2 cli commands #2033

Merged

		if ( ! ( await isEmptyDir( path ) ) && ! isWordPressDirectory( path ) ) {
		wasPathEmpty = true;

fix: Improve site creation error handling #4

fix: Improve site creation error handling #4

Uh oh!

Conversation

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

Uh oh!

Proposed Changes

Testing Instructions

Pre-merge Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sejas left a comment

Choose a reason for hiding this comment

Uh oh!

sejas Apr 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sejas Apr 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dcalhoun commented Apr 22, 2024 •

edited

Loading

sejas Apr 23, 2024 •

edited

Loading

sejas Apr 26, 2024 •

edited

Loading

dcalhoun Apr 26, 2024 •

edited

Loading