Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion src/components/site-form.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ function FormPathInputComponent( {
) }
>
<Icon
className={ error ? 'fill-red-500' : '' }
className={ cx( 'shrink-0 basis-4', error ? 'fill-red-500' : '' ) }
Copy link
Member Author

Choose a reason for hiding this comment

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

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

icon={ error ? warning : tip }
width={ 16 }
height={ 16 }
Expand Down
61 changes: 61 additions & 0 deletions src/components/tests/add-site.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -167,4 +167,65 @@ describe( 'AddSite', () => {
expect( screen.getByDisplayValue( 'My WordPress Website' ) ).toBeInTheDocument();
expect( screen.getByDisplayValue( '/default_path/my-wordpress-website' ) ).toBeInTheDocument();
} );

it( 'should reset to the proposed path when the path is set to default app directory', async () => {
const user = userEvent.setup();
mockGenerateProposedSitePath.mockImplementation( ( name ) => {
const path = `/default_path/${ name.replace( /\s/g, '-' ).toLowerCase() }`;
return Promise.resolve( {
path,
name,
isEmpty: true,
isWordPress: false,
} );
} );
mockShowOpenFolderDialog.mockResolvedValue( {
path: 'populated-non-wordpress-directory',
name: 'My WordPress Website',
isEmpty: false,
isWordPress: false,
} );
render( <AddSite /> );

await user.click( screen.getByRole( 'button', { name: 'Add site' } ) );
await user.click( screen.getByTestId( 'select-path-button' ) );

mockShowOpenFolderDialog.mockResolvedValue( {
path: '/default_path',
name: 'My WordPress Website',
isEmpty: false,
isWordPress: false,
} );
await user.click( screen.getByTestId( 'select-path-button' ) );
await user.click( screen.getByDisplayValue( 'My WordPress Website' ) );
await user.type( screen.getByDisplayValue( 'My WordPress Website' ), ' mutated' );
// screen.debug( screen.getByRole( 'dialog' ) );

expect(
screen.getByDisplayValue( '/default_path/my-wordpress-website-mutated' )
).toBeInTheDocument();
} );

it( 'should display a helpful error message when an error occurs while creating the site', async () => {
const user = userEvent.setup();
mockGenerateProposedSitePath.mockResolvedValue( {
path: '/default_path/my-wordpress-website',
name: 'My WordPress Website',
isEmpty: true,
isWordPress: false,
} );
mockCreateSite.mockImplementation( () => {
throw new Error( 'Failed to create site' );
} );
render( <AddSite /> );

await user.click( screen.getByRole( 'button', { name: 'Add site' } ) );
await user.click( screen.getByRole( 'button', { name: 'Add site' } ) );

await waitFor( () => {
expect( screen.getByRole( 'alert' ) ).toHaveTextContent(
'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.'
);
} );
} );
} );
18 changes: 13 additions & 5 deletions src/hooks/use-add-site.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import * as Sentry from '@sentry/electron/renderer';
import { useI18n } from '@wordpress/react-i18n';
import { useCallback, useMemo, useState } from 'react';
import { getIpcApi } from '../lib/get-ipc-api';
Expand Down Expand Up @@ -28,11 +29,13 @@ export function useAddSite() {
const { path, name, isEmpty, isWordPress } = response;
setDoesPathContainWordPress( false );
setError( '' );
setSitePath( path );
const pathResetToDefaultSitePath =
path === proposedSitePath.substring( 0, proposedSitePath.lastIndexOf( '/' ) );
setSitePath( pathResetToDefaultSitePath ? '' : path );
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

if ( siteWithPathAlreadyExists( path ) ) {
return;
}
if ( ! isEmpty && ! isWordPress ) {
if ( ! isEmpty && ! isWordPress && ! pathResetToDefaultSitePath ) {
setError(
__(
'This directory is not empty. Please select an empty directory or an existing WordPress folder.'
Expand All @@ -45,20 +48,25 @@ export function useAddSite() {
setSiteName( name ?? null );
}
}
}, [ __, siteWithPathAlreadyExists, siteName ] );
}, [ __, siteWithPathAlreadyExists, siteName, proposedSitePath ] );

const handleAddSiteClick = useCallback( async () => {
setIsAddingSite( true );
try {
const path = sitePath ? sitePath : proposedSitePath;
await createSite( path, siteName ?? '' );
} catch ( e ) {
setError( ( e as Error )?.message );
Sentry.captureException( e );
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

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.'
)
);
Comment on lines +60 to +64
Copy link
Member Author

Choose a reason for hiding this comment

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

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

  1. 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...
  2. 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...
  3. 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.

setIsAddingSite( false );
throw e;
}
setIsAddingSite( false );
}, [ createSite, proposedSitePath, siteName, sitePath ] );
}, [ createSite, proposedSitePath, siteName, sitePath, __ ] );

const handleSiteNameChange = useCallback(
async ( name: string ) => {
Expand Down
46 changes: 16 additions & 30 deletions src/ipc-handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -112,20 +112,15 @@ export async function createSite(
): Promise< SiteDetails[] > {
const userData = await loadUserData();
const forceSetupSqlite = false;
let wasPathEmpty = false;
// We only try to create the directory recursively if the user has
// not selected a path from the dialog (and thus they use the "default" path)
// We only recursively create the directory if the user has not selected a
// path from the dialog (and thus they use the "default" or suggested path).
if ( ! ( await pathExists( path ) ) && path.startsWith( DEFAULT_SITE_PATH ) ) {
try {
fs.mkdirSync( path, { recursive: true } );
} catch ( err ) {
return userData.sites;
}
fs.mkdirSync( path, { recursive: true } );
}

if ( ! ( await isEmptyDir( path ) ) && ! isWordPressDirectory( path ) ) {
wasPathEmpty = true;
Comment on lines 121 to -127
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

userData.sites;
Copy link
Member Author

Choose a reason for hiding this comment

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

@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.

// Form validation should've prevented a non-empty directory from being selected
throw new Error( 'The selected directory is not empty nor an existing WordPress site.' );
}

const allPaths = userData?.sites?.map( ( site ) => site.path ) || [];
Expand All @@ -148,27 +143,18 @@ export async function createSite(
const server = SiteServer.create( details );

if ( isWordPressDirectory( path ) ) {
try {
// If the directory contains a WordPress installation, and user wants to force SQLite
// integration, let's rename the wp-config.php file to allow WP Now to create a new one
// and initialize things properly.
if ( forceSetupSqlite && ( await pathExists( nodePath.join( path, 'wp-config.php' ) ) ) ) {
fs.renameSync(
nodePath.join( path, 'wp-config.php' ),
nodePath.join( path, 'wp-config-studio.php' )
);
}
// If the directory contains a WordPress installation, and user wants to force SQLite
// integration, let's rename the wp-config.php file to allow WP Now to create a new one
// and initialize things properly.
if ( forceSetupSqlite && ( await pathExists( nodePath.join( path, 'wp-config.php' ) ) ) ) {
fs.renameSync(
nodePath.join( path, 'wp-config.php' ),
nodePath.join( path, 'wp-config-studio.php' )
);
}

if ( ! ( await pathExists( nodePath.join( path, 'wp-config.php' ) ) ) ) {
await setupSqliteIntegration( path );
}
} catch ( error ) {
Sentry.captureException( error );
if ( wasPathEmpty ) {
// Clean the path to let the user try again
await shell.trashItem( path );
}
Comment on lines -167 to -170
Copy link
Member

@sejas sejas Apr 23, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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. 🙇🏻

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@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! 🙇🏻

Copy link
Member

@sejas sejas Apr 26, 2024

Choose a reason for hiding this comment

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

@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.

Copy link
Member Author

@dcalhoun dcalhoun Apr 26, 2024

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

throw new Error( 'Error creating the site. Please contact support.' );
if ( ! ( await pathExists( nodePath.join( path, 'wp-config.php' ) ) ) ) {
await setupSqliteIntegration( path );
}
}

Expand Down