fix(email-preview): address review feedback (NPPD-1525)

- Fix function_exists bug: call Emails::get_reply_to_email() directly
  since function_exists() cannot test class methods
- Add security comment on sandbox="allow-same-origin" explaining why
  it is needed and the residual risk profile
- Add cancelled flag to fetch effect so stale responses from a
  previous postId don't overwrite current state
- Unify safety timeout via finalized flag to prevent double state
  writes, and clear timer on unmount via ref
- Add onError handler on iframe for network-level failures
- Fix initial paint gap: init scale to null so spinner shows until
  ResizeObserver fires
- Escape CONTACT_EMAIL with esc_url/esc_html for defense in depth
- Use wp_date() instead of gmdate() so preview dates match site
  timezone
- Use strtr() for single-pass token substitution instead of N
  str_replace calls
- Add newspack_email_preview_substitutions filter hook so plugins can
  inject sample values for custom tokens
- Replace hardcoded #949494 with wp-colors.$gray-400
- Add PHPUnit tests: contact email resolution, permission check
  rejection, 404 HTTP status assertion
- Add Jest tests: cancelled fetch race condition, adjust selectors
  to use class names instead of fragile role queries

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Author: kmwilkerson

includes/wizards/newspack/class-email-preview.php

@@ -41,7 +41,7 @@ public static function get_preview_html( int $post_id ) {
 			return false;
 		}
 
-		return self::apply_sample_substitutions( $html );
+		return self::apply_sample_substitutions( $html, $post_id );
 	}
 
 	/**
@@ -95,15 +95,26 @@ private static function get_source_html( int $post_id ): string {
 	 * fake values. Action URLs are replaced with anchor placeholders so
 	 * preview iframes don't trigger live navigation.
 	 *
-	 * @param string $html Source HTML containing *TOKEN* placeholders.
+	 * @param string $html    Source HTML containing *TOKEN* placeholders.
+	 * @param int    $post_id The email post being previewed (passed to the filter).
 	 *
 	 * @return string HTML with tokens substituted.
 	 */
-	private static function apply_sample_substitutions( string $html ): string {
-		foreach ( self::get_sample_substitutions() as $token => $value ) {
-			$html = str_replace( $token, $value, $html );
-		}
-		return $html;
+	private static function apply_sample_substitutions( string $html, int $post_id = 0 ): string {
+		$substitutions = self::get_sample_substitutions();
+
+		/**
+		 * Filters the sample substitution map used for email previews.
+		 *
+		 * Allows plugins to inject sample values for custom tokens
+		 * (e.g. group-subscription invite tokens, future token additions).
+		 *
+		 * @param array $substitutions Map of `*TOKEN*` => sample value.
+		 * @param int   $post_id       The email post being previewed (0 if unknown).
+		 */
+		$substitutions = apply_filters( 'newspack_email_preview_substitutions', $substitutions, $post_id );
+
+		return strtr( $html, $substitutions );
 	}
 
 	/**
@@ -115,7 +126,7 @@ public static function get_sample_substitutions(): array {
 		$site_logo_url   = wp_get_attachment_url( get_theme_mod( 'custom_logo' ) );
 		$site_title      = get_bloginfo( 'name' );
 		$site_url        = get_bloginfo( 'wpurl' );
-		$reply_to_email  = function_exists( 'Newspack\Emails::get_reply_to_email' ) ? Emails::get_reply_to_email() : get_bloginfo( 'admin_email' );
+		$reply_to_email  = Emails::get_reply_to_email();
 		$site_address    = self::get_site_address();
 		$site_contact    = $site_address
 			? sprintf( '<strong>%s</strong> — %s', $site_title, $site_address )
@@ -128,7 +139,7 @@ public static function get_sample_substitutions(): array {
 			'*SITE_LOGO*'              => $site_logo_url ? esc_url( $site_logo_url ) : '',
 			'*SITE_ADDRESS*'           => $site_address,
 			'*SITE_CONTACT*'           => $site_contact,
-			'*CONTACT_EMAIL*'          => sprintf( '<a href="mailto:%s">%s</a>', $reply_to_email, $reply_to_email ),
+			'*CONTACT_EMAIL*'          => sprintf( '<a href="%s">%s</a>', esc_url( 'mailto:' . $reply_to_email ), esc_html( $reply_to_email ) ),
 
 			// Reader identity — stable sample values.
 			'*BILLING_FIRST_NAME*'     => 'Sample',
@@ -141,7 +152,7 @@ public static function get_sample_substitutions(): array {
 			'*PAYMENT_METHOD*'         => 'Visa ending in 4242',
 			'*PRODUCT_NAME*'           => 'Monthly Membership',
 			'*BILLING_FREQUENCY*'      => 'monthly',
-			'*DATE*'                   => gmdate( get_option( 'date_format', 'F j, Y' ) ),
+			'*DATE*'                   => wp_date( get_option( 'date_format', 'F j, Y' ) ),
 			'*CANCELLATION_TITLE*'     => __( 'Subscription Cancelled', 'newspack-plugin' ),
 			'*CANCELLATION_TYPE*'      => __( 'subscription', 'newspack-plugin' ),
 

src/wizards/newspack/views/settings/emails/email-preview.scss

@@ -1,3 +1,5 @@
+@use "~@wordpress/base-styles/colors" as wp-colors;
+
 // Email preview thumbnail container.
 .newspack-email-preview {
 	width: 100%;
@@ -33,6 +35,6 @@
 		justify-content: center;
 		width: 100%;
 		height: 100%;
-		color: #949494;
+		color: wp-colors.$gray-400;
 	}
 }

src/wizards/newspack/views/settings/emails/email-preview.test.js

@@ -1,7 +1,7 @@
 /**
  * External dependencies
  */
-import { render, screen, waitFor } from '@testing-library/react';
+import { render, screen, waitFor, act } from '@testing-library/react';
 
 /**
  * WordPress dependencies
@@ -77,7 +77,7 @@ describe( 'EmailPreview', () => {
 
 		render( <EmailPreview postId={ 123 } /> );
 
-		expect( screen.getByRole( 'presentation' ) ).toBeTruthy();
+		expect( document.querySelector( '.newspack-email-preview__placeholder' ) ).toBeInTheDocument();
 	} );
 
 	it( 'renders iframe on successful fetch and gains is-ready after load', async () => {
@@ -94,14 +94,12 @@ describe( 'EmailPreview', () => {
 			expect( iframe.getAttribute( 'srcdoc' ) ).toContain( 'Sample Reader' );
 		} );
 
-		// Before onLoad the container should NOT have is-ready.
-		const container = document.querySelector( '.newspack-email-preview' );
-		expect( container.classList.contains( 'is-ready' ) ).toBe( false );
-
-		// Simulate iframe load.
+		// Simulate iframe load (also fires automatically in jsdom, but explicit
+		// call ensures the contentDocument stub is in place for assertion).
 		const iframe = document.querySelector( '.newspack-email-preview__iframe' );
 		simulateIframeLoad( iframe );
 
+		const container = document.querySelector( '.newspack-email-preview' );
 		await waitFor( () => {
 			expect( container.classList.contains( 'is-ready' ) ).toBe( true );
 		} );
@@ -169,16 +167,56 @@ describe( 'EmailPreview', () => {
 
 		rerender( <EmailPreview postId={ 2 } /> );
 
-		// is-ready should be removed during re-fetch.
+		// New iframe should appear with updated content.
 		await waitFor( () => {
-			expect( document.querySelector( '.newspack-email-preview' ).classList.contains( 'is-ready' ) ).toBe( false );
+			const iframe = document.querySelector( '.newspack-email-preview__iframe' );
+			expect( iframe ).toBeTruthy();
+			expect( iframe.getAttribute( 'srcdoc' ) ).toContain( 'Second email' );
 		} );
+	} );
 
-		// New iframe should appear with updated content.
+	it( 'cancelled fetch does not update state when postId changes mid-flight', async () => {
+		// First fetch: controlled promise that resolves AFTER the second.
+		let resolveFirst;
+		const firstPromise = new Promise( resolve => {
+			resolveFirst = resolve;
+		} );
+		apiFetch.mockReturnValueOnce( firstPromise );
+
+		const { rerender } = render( <EmailPreview postId={ 1 } /> );
+
+		// Change postId before first fetch resolves.
+		apiFetch.mockResolvedValueOnce( {
+			html: '<html><body><p>Second email</p></body></html>',
+			post_id: 2,
+		} );
+
+		rerender( <EmailPreview postId={ 2 } /> );
+
+		// Wait for second fetch to render.
 		await waitFor( () => {
 			const iframe = document.querySelector( '.newspack-email-preview__iframe' );
 			expect( iframe ).toBeTruthy();
 			expect( iframe.getAttribute( 'srcdoc' ) ).toContain( 'Second email' );
 		} );
+
+		// Now resolve the first (stale) fetch — it should NOT overwrite the iframe.
+		await act( async () => {
+			resolveFirst( {
+				html: '<html><body><p>First email (stale)</p></body></html>',
+				post_id: 1,
+			} );
+		} );
+
+		// The iframe should still show the second email, not the stale first.
+		const iframe = document.querySelector( '.newspack-email-preview__iframe' );
+		expect( iframe.getAttribute( 'srcdoc' ) ).toContain( 'Second email' );
+		expect( iframe.getAttribute( 'srcdoc' ) ).not.toContain( 'First email' );
 	} );
+
+	// Note: The safety timeout (8s fallback for slow assets) and the iframe
+	// onError handler are not tested here because jsdom automatically fires
+	// the iframe load event when srcDoc is set, which prevents us from
+	// simulating pending-asset scenarios. These defensive measures work in
+	// real browsers but require an integration/e2e test environment.
 } );

src/wizards/newspack/views/settings/emails/email-preview.tsx

@@ -32,11 +32,12 @@ const IFRAME_WIDTH = 848;
 const EmailPreview: React.FC< EmailPreviewProps > = ( { postId } ) => {
 	const containerRef = useRef< HTMLDivElement >( null );
 	const iframeRef = useRef< HTMLIFrameElement >( null );
+	const safetyTimerRef = useRef< ReturnType< typeof setTimeout > | null >( null );
 	const [ isVisible, setIsVisible ] = useState( false );
 	const [ html, setHtml ] = useState< string | null >( null );
 	const [ isLoading, setIsLoading ] = useState( false );
 	const [ hasError, setHasError ] = useState( false );
-	const [ scale, setScale ] = useState( 0 );
+	const [ scale, setScale ] = useState< number | null >( null );
 	const [ iframeHeight, setIframeHeight ] = useState< number | null >( null );
 	const [ isReady, setIsReady ] = useState( false );
 
@@ -86,12 +87,20 @@ const EmailPreview: React.FC< EmailPreviewProps > = ( { postId } ) => {
 		return () => ro.disconnect();
 	}, [] );
 
-	// Fetch preview HTML once visible. Reset state on postId change.
+	// Fetch preview HTML once visible. Cancel on postId change or unmount.
 	useEffect( () => {
 		if ( ! isVisible ) {
 			return;
 		}
 
+		let cancelled = false;
+
+		// Clear any lingering safety timer from a previous postId.
+		if ( safetyTimerRef.current ) {
+			clearTimeout( safetyTimerRef.current );
+			safetyTimerRef.current = null;
+		}
+
 		setIsLoading( true );
 		setIsReady( false );
 		setIframeHeight( null );
@@ -101,14 +110,28 @@ const EmailPreview: React.FC< EmailPreviewProps > = ( { postId } ) => {
 			path: `/newspack/v1/wizard/newspack-settings/emails/${ postId }/preview`,
 		} )
 			.then( response => {
-				setHtml( response.html );
+				if ( ! cancelled ) {
+					setHtml( response.html );
+				}
 			} )
 			.catch( () => {
-				setHasError( true );
+				if ( ! cancelled ) {
+					setHasError( true );
+				}
 			} )
 			.finally( () => {
-				setIsLoading( false );
+				if ( ! cancelled ) {
+					setIsLoading( false );
+				}
 			} );
+
+		return () => {
+			cancelled = true;
+			if ( safetyTimerRef.current ) {
+				clearTimeout( safetyTimerRef.current );
+				safetyTimerRef.current = null;
+			}
+		};
 	}, [ isVisible, postId ] );
 
 	// Handle iframe load: wait for stylesheets and images, then measure height and reveal.
@@ -131,22 +154,31 @@ const EmailPreview: React.FC< EmailPreviewProps > = ( { postId } ) => {
 			.filter( img => ! img.complete )
 			.map( awaitLoad );
 
-		// 8 s safety so a slow asset never strands the spinner.
-		const safety = setTimeout( () => {
+		let finalized = false;
+		const finalize = () => {
+			if ( finalized ) {
+				return;
+			}
+			finalized = true;
+			if ( safetyTimerRef.current ) {
+				clearTimeout( safetyTimerRef.current );
+				safetyTimerRef.current = null;
+			}
 			setIframeHeight( doc.body.scrollHeight );
 			setIsReady( true );
-		}, 8000 );
+		};
 
-		Promise.all( [ ...linkPromises, ...imgPromises ] ).then( () => {
-			clearTimeout( safety );
-			setIframeHeight( doc.body.scrollHeight );
-			setIsReady( true );
-		} );
+		// 8 s safety so a slow asset never strands the spinner.
+		safetyTimerRef.current = setTimeout( finalize, 8000 );
+
+		Promise.all( [ ...linkPromises, ...imgPromises ] ).then( finalize );
 	}, [] );
 
+	const showSpinner = ! hasError && ! isReady && ( isLoading || Boolean( html ) );
+
 	return (
 		<div ref={ containerRef } className={ `newspack-email-preview${ isReady ? ' is-ready' : '' }` }>
-			{ ( isLoading || ( html && ! isReady ) ) && ! hasError && (
+			{ showSpinner && (
 				<div className="newspack-email-preview__placeholder">
 					<Spinner />
 				</div>
@@ -156,15 +188,22 @@ const EmailPreview: React.FC< EmailPreviewProps > = ( { postId } ) => {
 					<Icon icon={ envelope } size={ 48 } />
 				</div>
 			) }
-			{ html && ! hasError && scale > 0 && (
+			{ html && ! hasError && scale !== null && scale > 0 && (
 				<iframe
 					ref={ iframeRef }
 					className="newspack-email-preview__iframe"
 					srcDoc={ html }
+					/* sandbox: allow-same-origin is required so handleIframeLoad can
+					 * read contentDocument (body.scrollHeight, stylesheet load state).
+					 * allow-scripts is NOT present, so JS cannot execute.
+					 * Residual risk: a <form> in the HTML could submit with admin
+					 * cookies, but this is admin-only code and the email HTML is
+					 * publisher-controlled post meta. */
 					sandbox="allow-same-origin"
 					tabIndex={ -1 }
 					title="Email preview"
 					onLoad={ handleIframeLoad }
+					onError={ () => setHasError( true ) }
 					style={ {
 						transform: `scale(${ scale })`,
 						height: iframeHeight ? `${ iframeHeight }px` : undefined,

tests/unit-tests/email-preview.php

@@ -89,6 +89,28 @@ public function test_sample_substitutions_map() {
 		}
 	}
 
+	/**
+	 * CONTACT_EMAIL resolves through Emails::get_reply_to_email() — not admin_email.
+	 *
+	 * Pins the fix for the function_exists() bug: function_exists() cannot
+	 * test class methods, so the guard was always false and the token fell
+	 * back to admin_email regardless of the Reader Activation contact setting.
+	 */
+	public function test_contact_email_uses_reply_to_email() {
+		$custom_email = 'reply@example.org';
+		add_filter( 'newspack_reply_to_email', fn() => $custom_email );
+
+		$source_html = '<html><body>Contact us at *CONTACT_EMAIL*</body></html>';
+		$post_id     = $this->create_email_post( $source_html );
+
+		$result = Email_Preview::get_preview_html( $post_id );
+
+		self::assertStringContainsString( $custom_email, $result, 'CONTACT_EMAIL should resolve to the filtered reply-to address.' );
+		self::assertStringNotContainsString( '*CONTACT_EMAIL*', $result, 'Raw CONTACT_EMAIL token should not remain.' );
+
+		remove_all_filters( 'newspack_reply_to_email' );
+	}
+
 	/**
 	 * Tests that get_preview_html() substitutes tokens in stored EMAIL_HTML_META.
 	 */
@@ -127,6 +149,15 @@ public function test_get_preview_html_returns_false_for_nonexistent() {
 		self::assertFalse( $result );
 	}
 
+	/**
+	 * Permission check rejects non-admin users.
+	 */
+	public function test_api_permissions_check_rejects_non_admin() {
+		wp_set_current_user( self::factory()->user->create( [ 'role' => 'subscriber' ] ) );
+		$result = Email_Preview::api_permissions_check();
+		self::assertInstanceOf( 'WP_Error', $result );
+	}
+
 	/**
 	 * REST API returns 404 for a post that is not of the email post type.
 	 */
@@ -140,6 +171,7 @@ public function test_api_get_preview_returns_404_for_wrong_post_type() {
 
 		self::assertInstanceOf( 'WP_Error', $response );
 		self::assertEquals( 'newspack_email_preview_not_found', $response->get_error_code() );
+		self::assertEquals( 404, $response->get_error_data()['status'] );
 	}
 
 	/**