Improve site path uniqueness validation using normalized comparisons - #976
Conversation
|
@Automattic/yolo function isPathEndingWith(path: string, suffix: string): boolean {
// Normalize paths by standardizing separators and case
const normalize = (p: string) => p.toLowerCase().replace(/[\\\/]+/g, '/');
return normalize(path).endsWith(normalize(suffix));
}I opted for consistency and reliability, that this solution doesn't offer, but at the cost of some more calls to the IPC. |
wojtekn
left a comment
wojtekn
left a comment
There was a problem hiding this comment.
I tested it on Windows and Mac and it works fine.
fredrikekelund
left a comment
fredrikekelund
left a comment
There was a problem hiding this comment.
Making generateSiteName async so that we can properly validate the proposed path seems to work well, but I think we should consider my proposal of dropping isPathEndingWith in favor of generateProposedSitePath
| const sanitizedPath = sanitizeFolderName( name ); | ||
| return ! usedSites.some( ( site ) => site.path.toLowerCase().endsWith( '/' + sanitizedPath ) ); | ||
| for ( const site of usedSites ) { | ||
| if ( await getIpcApi().isPathEndingWith( site.path, `/${ sanitizedPath }` ) ) { | ||
| return false; | ||
| } | ||
| } | ||
| return true; |
There was a problem hiding this comment.
const { isEmpty } = await getIpcApi().generateProposedSitePath( name );
return isEmpty;Could we do something like this instead? It seems like we're kind of duplicating logic now between generateProposedSitePath and isPathEndingWith now, and this solution seems to work well in my testing.
This would also allow us to remove isPathEndingWith altogether, simplifying this PR.
| useEffect( () => { | ||
| const run = async () => { | ||
| const { path, name, isWordPress } = await getIpcApi().generateProposedSitePath( | ||
| generateSiteName( [] ) | ||
| ); | ||
| setSiteName( name ); | ||
| setProposedSitePath( path ); | ||
| setSitePath( '' ); | ||
| setError( '' ); | ||
| setDoesPathContainWordPress( isWordPress ); | ||
| }; | ||
| run(); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [] ); | ||
| const run = async () => { | ||
| const siteName = await generateSiteName( [] ); | ||
| const { path, name, isWordPress } = await getIpcApi().generateProposedSitePath( siteName ); | ||
| setSiteName( name ); | ||
| setProposedSitePath( path ); | ||
| setSitePath( '' ); | ||
| setError( '' ); | ||
| setDoesPathContainWordPress( isWordPress ); | ||
| }; | ||
| run(); |
There was a problem hiding this comment.
I don't understand this change. Why should we move this logic outside the useEffect callback? 🤔
There was a problem hiding this comment.
We shouldn't 😅
I rolled back the change.
|
@fredrikekelund thanks for the suggestion, I agree that's an improvement! |
fredrikekelund
left a comment
fredrikekelund
left a comment
There was a problem hiding this comment.
LGTM and works as expected 👍
| setDoesPathContainWordPress( isWordPress ); | ||
| }; | ||
| run(); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
Let's bring this line back to signal that we are intentionally excluding those effect dependecies.
3 participants
Related issues
Proposed Changes
generateProposedSitePathto check if the path is available for a new sitegenerateSiteNameto be async to support proper path validationTesting Instructions
Pre-merge Checklist