Boost: Improve critical css generation process (#41946)

* Add watch command

* Check http code before proceeding with C.CSS generation

We need to do this, otherwise the library will generate css for
the 404 page.

* Update css generation to catch invalid URL errors and surface them

* Update recommendations to be split by type as well as provider key

WIP

* Add changelogs

* Refactor recommendations to allow dismissing unique provider errors

* Update URL validation

* Update helper description

* Add proper troubleshooting steps for invalid URLs

* Update status code validation

* Fix tests

* Throw an error on invalid url

Co-authored-by: Liam Sarsfield <[email protected]>

* Remove redundant fragment

Co-authored-by: Liam Sarsfield <[email protected]>

* Improve type safety and readability of error grouping

* Update step

Co-authored-by: Adnan Haque <[email protected]>

* Add an additional step

* Update http status check to check for a 404 instead

* Revert slashes to see if opaque redirects work

* Simplify 404 check

---------

Co-authored-by: Liam Sarsfield <[email protected]>
Co-authored-by: Adnan Haque <[email protected]>

Author: 3 people

projects/js-packages/critical-css-gen/changelog/fix-critical-css-gen-invalid-url-breaking-whole-process

@@ -0,0 +1,4 @@
+Significance: patch
+Type: fixed
+
+Critical CSS: Prevent invalid URLs from breaking the whole process.

projects/js-packages/critical-css-gen/composer.json

@@ -19,6 +19,10 @@
 		],
 		"test-js": [
 			"pnpm exec playwright install --with-deps chromium && pnpm run test"
+		],
+		"watch": [
+			"Composer\\Config::disableProcessTimeout",
+			"pnpm run watch"
 		]
 	},
 	"repositories": [

projects/js-packages/critical-css-gen/package.json

@@ -20,7 +20,8 @@
 		"clean": "rm -rf build/",
 		"clean:test": "rm -rf tests/build/",
 		"test": "pnpm build && pnpm build:test && NODE_ENV=test NODE_PATH=./node_modules jest --forceExit --config=tests/config/jest.config.js",
-		"test-coverage": "pnpm run test --coverage"
+		"test-coverage": "pnpm run test --coverage",
+		"watch": "tsc --watch"
 	},
 	"main": "./build/browser.js",
 	"devDependencies": {

projects/js-packages/critical-css-gen/src/browser-interface-iframe.ts

@@ -7,6 +7,7 @@ import {
 	UrlVerifyError,
 	UnknownError,
 	XFrameDenyError,
+	InvalidURLError,
 	UrlError,
 } from './errors.js';
 import { Viewport, NullableViewport } from './types.js';
@@ -96,13 +97,12 @@ export class BrowserInterfaceIframe extends BrowserInterface {
 		return method( { innerWindow: this.iframe.contentWindow, args } );
 	}
 
-	addGetParameters( rawUrl: string ): string {
-		const urlObject = new URL( rawUrl );
+	addGetParameters( url: URL ): string {
 		for ( const key of Object.keys( this.requestGetParameters ) ) {
-			urlObject.searchParams.append( key, this.requestGetParameters[ key ] );
+			url.searchParams.append( key, this.requestGetParameters[ key ] );
 		}
 
-		return urlObject.toString();
+		return url.toString();
 	}
 
 	async diagnoseUrlError( url: string ): Promise< UrlError | null > {
@@ -131,6 +131,12 @@ export class BrowserInterfaceIframe extends BrowserInterface {
 		}
 	}
 
+	async is404Page( url: string ): Promise< boolean > {
+		const response = await this.fetch( url, { redirect: 'manual' }, 'html' );
+
+		return response.status === 404;
+	}
+
 	sameOrigin( url: string ): boolean {
 		return new URL( url ).origin === window.location.origin;
 	}
@@ -140,7 +146,16 @@ export class BrowserInterfaceIframe extends BrowserInterface {
 			return;
 		}
 
-		const fullUrl = this.addGetParameters( rawUrl );
+		// Make sure URL is valid.
+		let url;
+		try {
+			url = new URL( rawUrl );
+		} catch ( err ) {
+			this.trackUrlError( rawUrl, err );
+			throw new InvalidURLError( { url: rawUrl } );
+		}
+
+		const fullUrl = this.addGetParameters( url );
 
 		return new Promise( ( resolve, rawReject ) => {
 			// Track all URL errors.
@@ -167,6 +182,12 @@ export class BrowserInterfaceIframe extends BrowserInterface {
 					this.iframe.onload = null;
 					clearTimeout( timeoutId );
 
+					// Check HTTP status code first.
+					const is404 = await this.is404Page( fullUrl );
+					if ( is404 ) {
+						throw new HttpError( { url, code: 404 } );
+					}
+
 					// Verify the inner document is readable.
 					if ( ! this.iframe.contentDocument || ! this.iframe.contentWindow ) {
 						throw (

projects/js-packages/critical-css-gen/src/errors.ts

@@ -140,3 +140,12 @@ export class XFrameDenyError extends UrlError {
 		);
 	}
 }
+
+/**
+ * InvalidURLError - Indicates that a requested URL is invalid.
+ */
+export class InvalidURLError extends UrlError {
+	constructor( { url }: { url: string } ) {
+		super( 'InvalidURLError', { url }, `Invalid URL: ${ url }` );
+	}
+}

projects/plugins/boost/app/assets/src/js/features/critical-css/error-description/error-description.tsx

@@ -13,12 +13,18 @@ import getCriticalCssErrorSetInterpolateVars from '$lib/utils/get-critical-css-e
  * Remove GET parameters that are used to cache-bust from display URLs, as they add visible noise
  * to the error output with no real benefit to users understanding which URLs are problematic.
  *
+ * If the URL is invalid, return the original URL.
+ *
  * @param url The URL to strip cache parameters from.
  */
 export function stripCacheParams( url: string ): string {
-	const urlObj = new URL( url );
-	urlObj.searchParams.delete( 'donotcachepage' );
-	return urlObj.toString();
+	try {
+		const urlObj = new URL( url );
+		urlObj.searchParams.delete( 'donotcachepage' );
+		return urlObj.toString();
+	} catch ( err ) {
+		return url;
+	}
 }
 
 const CriticalCssErrorDescription: React.FC< CriticalCssErrorDescriptionTypes > = ( {

projects/plugins/boost/app/assets/src/js/features/critical-css/lib/critical-css-errors.ts

@@ -7,6 +7,7 @@ import {
 	Critical_CSS_Error_Type,
 	Provider,
 } from './stores/critical-css-state-types';
+import { ProviderRecommendation } from './stores/recommendation-types';
 
 /**
  * Specification for a set of errors that can appear as a part of a recommendation.
@@ -140,3 +141,55 @@ export function groupKey( error: CriticalCssErrorDetails ) {
 
 	return error.type;
 }
+
+type RecommendationsResult = {
+	activeRecommendations: ProviderRecommendation[];
+	dismissedRecommendations: ProviderRecommendation[];
+};
+
+type ErrorsByType = Record< Critical_CSS_Error_Type, CriticalCssErrorDetails[] >;
+
+export function groupRecommendationsByStatus(
+	providersWithIssues: Provider[]
+): RecommendationsResult {
+	const activeRecommendations: ProviderRecommendation[] = [];
+	const dismissedRecommendations: ProviderRecommendation[] = [];
+
+	providersWithIssues.forEach( provider => {
+		const providerErrors = provider.errors || [];
+		// Group errors by type first
+		const errorsByType = providerErrors.reduce( ( acc, error ) => {
+			if ( ! acc[ error.type ] ) {
+				acc[ error.type ] = [];
+			}
+			acc[ error.type ].push( error );
+			return acc;
+		}, {} as ErrorsByType );
+
+		const errorTypeGroups = Object.entries( errorsByType ) as [
+			Critical_CSS_Error_Type,
+			CriticalCssErrorDetails[],
+		][];
+
+		// For each error type group, check if it's dismissed
+		errorTypeGroups.forEach( ( [ errorType, errors ] ) => {
+			if ( provider.dismissed_errors?.includes( errorType ) ) {
+				dismissedRecommendations.push( {
+					key: provider.key,
+					label: provider.label,
+					errorType: errorType,
+					errors: errors,
+				} );
+			} else {
+				activeRecommendations.push( {
+					key: provider.key,
+					label: provider.label,
+					errorType: errorType,
+					errors: errors,
+				} );
+			}
+		} );
+	} );
+
+	return { activeRecommendations, dismissedRecommendations };
+}

projects/plugins/boost/app/assets/src/js/features/critical-css/lib/describe-critical-css-recommendations.ts

@@ -464,6 +464,46 @@ const errorTypeSpecs: { [ type: string ]: ErrorTypeSpec } = {
 			],
 		} ),
 	},
+
+	InvalidURLError: {
+		describeSet: set =>
+			_n(
+				'Jetpack Boost found an invalid URL:',
+				'Jetpack Boost found invalid URLs:',
+				urlCount( set ),
+				'jetpack-boost'
+			),
+		suggestion: _set => ( {
+			paragraph: __(
+				'Jetpack Boost needs valid URLs in order to generate Critical CSS. It seems that one or more of the URLs you provided are invalid.',
+				'jetpack-boost'
+			),
+			list: [
+				__( 'It is okay to ignore URLs that are not meant to be shown publicly.', 'jetpack-boost' ),
+				__(
+					'If they are meant to be shown publicly, check if you have any plugins that modify your permalinks or URL structure.',
+					'jetpack-boost'
+				),
+				__(
+					'Verify your WordPress permalink settings are configured correctly in Settings → Permalinks.',
+					'jetpack-boost'
+				),
+				__(
+					'If you use custom post types, ensure they are properly registered and their permalinks are working.',
+					'jetpack-boost'
+				),
+				__(
+					'Try visiting the problematic URLs directly in your browser to confirm they work.',
+					'jetpack-boost'
+				),
+				__( '<retry>Try again</retry> to generate the Critical CSS.', 'jetpack-boost' ),
+			],
+			closingParagraph: __(
+				'If the issue persists, you may want to check your theme and plugins for any custom code that modifies URLs or permalinks.',
+				'jetpack-boost'
+			),
+		} ),
+	},
 };
 
 function getErrorSpec( type: string ): ErrorTypeSpec {

projects/plugins/boost/app/assets/src/js/features/critical-css/lib/stores/critical-css-state-types.ts

@@ -12,6 +12,7 @@ const CriticalCssErrorType = z.enum( [
 	'UrlVerifyError',
 	'EmptyCSSError',
 	'XFrameDenyError',
+	'InvalidURLError',
 ] );
 
 export const CriticalCssErrorDetailsSchema = z.object( {
@@ -37,8 +38,8 @@ export const ProviderSchema = z.object( {
 		.catch( 'validation-error' ),
 	// Error details
 	errors: z.array( CriticalCssErrorDetailsSchema ).optional(),
-	// If this an error, has it been dismissed?
-	error_status: z.enum( [ 'active', 'dismissed' ] ).optional(),
+	// List of error types that have been dismissed for this provider
+	dismissed_errors: z.array( CriticalCssErrorType ).optional(),
 } );
 
 export const CriticalCssStateSchema = z

projects/plugins/boost/app/assets/src/js/features/critical-css/lib/stores/critical-css-state.ts

@@ -141,7 +141,8 @@ export function useSetProviderErrorDismissedAction() {
 		'set-provider-errors-dismissed',
 		z.array(
 			z.object( {
-				key: z.string(),
+				provider: z.string(),
+				error_type: z.string(),
 				dismissed: z.boolean(),
 			} )
 		)

projects/plugins/boost/app/assets/src/js/features/critical-css/lib/stores/recommendation-types.ts

@@ -0,0 +1,19 @@
+import { Critical_CSS_Error_Type, CriticalCssErrorDetails } from './critical-css-state-types';
+
+export type DismissedItem = {
+	provider: string;
+	dismissed: boolean;
+	errorType: Critical_CSS_Error_Type;
+};
+
+export type RecommendationProps = {
+	recommendation: ProviderRecommendation;
+	setDismissed: ( dismissedItems: DismissedItem[] ) => void;
+};
+
+export type ProviderRecommendation = {
+	key: string;
+	label: string;
+	errors: CriticalCssErrorDetails[];
+	errorType: Critical_CSS_Error_Type;
+};

projects/plugins/boost/app/assets/src/js/features/critical-css/more-list/more-list.tsx

@@ -13,14 +13,28 @@ const MoreList: React.FC< MoreListTypes > = ( { entries = [], showLimit = 2 } )
 	const listItems = expanded ? entries : entries.slice( 0, showLimit );
 	const showExpandButton = ! expanded && entries.length > showLimit;
 
+	const isValidUrl = ( url: string ) => {
+		try {
+			const urlObj = new URL( url );
+			// Check that it's a web URL (http or https protocol)
+			return urlObj.protocol === 'http:' || urlObj.protocol === 'https:';
+		} catch ( err ) {
+			return false;
+		}
+	};
+
 	return (
 		<>
 			<ul className={ styles[ 'more-list' ] }>
 				{ listItems.map( ( { href, label }, index ) => (
 					<li key={ index }>
-						<a href={ href } target="_blank" rel="noreferrer">
-							{ label }
-						</a>
+						{ isValidUrl( href ) ? (
+							<a href={ href } target="_blank" rel="noreferrer">
+								{ label }
+							</a>
+						) : (
+							<code>{ label }</code>
+						) }
 					</li>
 				) ) }
 

projects/plugins/boost/app/assets/src/js/lib/utils/get-support-link-critical-css.ts

@@ -10,6 +10,7 @@ function getPrettyError( error: string ) {
 		UrlVerifyError: 'url-verify-error',
 		EmptyCSSError: 'empty-css-error',
 		XFrameDenyError: 'x-frame-deny-error',
+		InvalidURLError: 'invalid-url-error',
 	};
 
 	return errorMap[ error as keyof typeof errorMap ] || error;