feat(emails): inline EmailPreview component#4730
Conversation
kmwilkerson
added
the
[Status] Needs Review
kmwilkerson
force-pushed
the
nppd-1525-email-preview-component
branch
from
2460fae to
10b84df
Compare
thomasguillot
left a comment
thomasguillot
left a comment
There was a problem hiding this comment.
Nice work on the lazy-load + scaling plumbing. Before this lands, please align the rendering contract with our existing NewsletterPreview in newspack-newsletters so the two grids feel like the same surface side-by-side. These are separate plugins — copy the relevant logic into newspack-plugin rather than extracting a shared package. Use newspack-newsletters as a read-only reference for the shape.
Reference (read-only, in newspack-newsletters)
- Component:
src/components/newsletter-preview/index.js— see lines 43–215, particularly theuseInlineStyleshook's iframe-readiness detection (lines 70–193) and theis-ready/fade-in pattern (lines 195–214). - Styles:
src/components/newsletter-preview/style.scss— note the.block-editor-block-preview__container { opacity: 0; transition: opacity 200ms ease-out; }→.is-ready &fade. - Grid wrapper:
src/admin-shell/screens/layouts-list/fields.jsPreviewCard(lines 197–242) — usesaspectRatio: '1',viewportWidth={ 848 }, androotMargin: "200px". - Lazy helper:
src/admin-shell/screens/layouts-list/lazy-preview.js.
Concrete changes to make in this PR
All paths below are in newspack-plugin.
1. Aspect ratio: 1 : 1
File: src/wizards/newspack/views/settings/emails/email-preview.scss
-.newspack-email-preview {
- width: 100%;
- aspect-ratio: 4 / 3;
- overflow: hidden;
- ...
- background: #f6f7f7;
+.newspack-email-preview {
+ width: 100%;
+ aspect-ratio: 1;
+ overflow: hidden;
+ ...
+ background: transparent;Rationale: newsletters layouts grid uses 1 : 1 (<LazyPreview placeholderStyle={{ aspectRatio: '1' }} />). Cards sit side-by-side in the same DataView shell, so matching makes them visually coherent. Transparent background lets the card chrome show through consistently.
2. Source viewport: 848 px wide, intrinsic height
File: src/wizards/newspack/views/settings/emails/email-preview.tsx
Replace the IFRAME_WIDTH = 600 constant and the fixed-size iframe with width 848, height measured from the loaded document. Concretely:
const IFRAME_WIDTH = 848; // matches NewsletterPreview's `viewportWidth={ 848 }`Update the SCSS so __iframe no longer hardcodes height:
&__iframe {
width: 848px;
height: auto; // set dynamically after load
border: 0;
position: absolute;
top: 0;
left: 0;
transform-origin: top left;
pointer-events: none;
}Then measure the natural document height after the iframe loads and set it inline. Add an onLoad handler:
const iframeRef = useRef<HTMLIFrameElement>( null );
const [ iframeHeight, setIframeHeight ] = useState< number | null >( null );
const [ isReady, setIsReady ] = useState( false );
const handleIframeLoad = () => {
const doc = iframeRef.current?.contentDocument;
if ( ! doc ) {
return;
}
// Mirror NewsletterPreview's readiness detection: wait for stylesheets
// and images before measuring + revealing, otherwise the height we read
// is mid-layout and the iframe pops in unstyled.
const awaitLoad = ( el: HTMLLinkElement | HTMLImageElement ) =>
new Promise< void >( resolve => {
el.addEventListener( 'load', () => resolve(), { once: true } );
el.addEventListener( 'error', () => resolve(), { once: true } );
} );
const linkPromises = Array.from(
doc.querySelectorAll< HTMLLinkElement >( 'link[rel="stylesheet"]' )
)
.filter( link => ! link.sheet )
.map( awaitLoad );
const imgPromises = Array.from( doc.querySelectorAll< HTMLImageElement >( 'img' ) )
.filter( img => ! img.complete )
.map( awaitLoad );
// 8s safety so a slow asset never strands the spinner — matches NewsletterPreview line 81.
const safety = setTimeout( () => {
setIframeHeight( doc.body.scrollHeight );
setIsReady( true );
}, 8000 );
Promise.all( [ ...linkPromises, ...imgPromises ] ).then( () => {
clearTimeout( safety );
setIframeHeight( doc.body.scrollHeight );
setIsReady( true );
} );
};And on the <iframe> element:
<iframe
ref={ iframeRef }
className="newspack-email-preview__iframe"
srcDoc={ html }
sandbox=""
tabIndex={ -1 }
title="Email preview"
onLoad={ handleIframeLoad }
style={ {
transform: `scale(${ scale })`,
height: iframeHeight ? `${ iframeHeight }px` : undefined,
} }
/>Scale stays containerWidth / IFRAME_WIDTH — but since IFRAME_WIDTH is now 848, the math changes accordingly.
Also: the PR description says 1200 × 900 but the SCSS as written is 600 × 1800. Please remove that line from the PR description once 848 × auto is the actual value.
3. Ready-state fade-in
Add the is-ready class pattern from NewsletterPreview.
Component:
<div
ref={ containerRef }
className={ `newspack-email-preview${ isReady ? ' is-ready' : '' }` }
>SCSS:
.newspack-email-preview {
...
&__iframe {
...
opacity: 0;
transition: opacity 200ms ease-out;
}
&.is-ready &__iframe {
opacity: 1;
}
}Keep the existing spinner placeholder mounted while ! isReady so the transition is spinner → fade-in iframe (no white flash).
4. Render gating
The current condition is html && ! hasError && ! isLoading && scale > 0. After the changes above, also render the iframe as soon as html arrives (so onLoad can fire and measure) — the iframe itself is hidden by opacity: 0 until isReady flips, and the spinner placeholder stays mounted in the meantime. Concretely:
{ html && ! hasError && scale > 0 && (
<iframe ... />
) }
{ ( isLoading || ( html && ! isReady ) ) && ! hasError && (
<div className="newspack-email-preview__placeholder">
<Spinner />
</div>
) }5. Iframe-state reset on postId change
When postId changes, reset html, isReady, iframeHeight, hasError so the next preview doesn't show stale content while loading. Add to the fetch effect:
useEffect( () => {
if ( ! isVisible ) {
return;
}
setIsLoading( true );
setIsReady( false );
setIframeHeight( null );
setHasError( false );
setHtml( null );
apiFetch...
}, [ isVisible, postId ] );6. Tests
Update src/wizards/newspack/views/settings/emails/email-preview.test.js:
- The "renders iframe on successful fetch" test will need to simulate
onLoad. Aftersrcdocis set, fireiframe.dispatchEvent(new Event('load'))and assert the container gains theis-readyclass. - Stub
iframe.contentDocumentto return an object withquerySelectorAllreturning[]andbody.scrollHeightreturning a number, sohandleIframeLoadresolves immediately. - Add a test for the reset-on-postId-change behaviour.
Out of scope (don't do)
- Don't extract a shared component into a package — these are separate plugins, duplication is fine.
- Don't honour
meta.background_color/ fonts here (system emails don't carry that meta; the newsletters version reads it from layout meta). - Don't reuse
LazyPreviewfrom newsletters — your inlineIntersectionObserveralready matches itsrootMargin: '200px'contract, which is what matters.
Acceptance check
When done, an Email card and a Newsletter Layout card placed side-by-side should:
- Be the same shape (1 : 1).
- Show a comparable amount of vertical content (header + first content block roughly fills the square).
- Fade in from spinner with no FOUC.
- Show transparent background behind the rendered HTML.
There was a problem hiding this comment.
Pull request overview
Adds a live “thumbnail” preview for each transactional email in the Newspack Settings → Emails DataViews UI by introducing a REST endpoint that returns rendered email HTML (with sample token substitution) and a React component that lazy-loads and displays that HTML in a sandboxed iframe.
Changes:
- Backend: Introduces
Email_Previewwith aGET /wizard/newspack-settings/emails/{post_id}/previewREST route to return cached email HTML (or template fallback) with token substitutions. - Frontend: Adds an
EmailPreviewcomponent (IntersectionObserver + ResizeObserver) and wires it into the Emails DataViews “Preview” column. - Tests: Adds PHPUnit coverage for preview HTML/token substitution and Jest coverage for the EmailPreview component behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
includes/wizards/newspack/class-email-preview.php |
Adds REST route + preview HTML rendering/token substitution logic. |
includes/class-newspack.php |
Includes the new class-email-preview.php loader. |
src/wizards/newspack/views/settings/emails/emails.tsx |
Replaces preview placeholder with <EmailPreview /> in the DataViews grid. |
src/wizards/newspack/views/settings/emails/email-preview.tsx |
New React component that lazy-loads and renders sandboxed iframe previews. |
src/wizards/newspack/views/settings/emails/email-preview.scss |
Styling for the preview thumbnail container/iframe/placeholder states. |
src/wizards/newspack/views/settings/emails/emails.scss |
Removes now-obsolete preview placeholder styles. |
tests/unit-tests/email-preview.php |
New PHPUnit tests for backend preview/substitution and REST handler behavior. |
src/wizards/newspack/views/settings/emails/email-preview.test.js |
New Jest tests for lazy-load gating, success/error states, and endpoint path. |
src/wizards/newspack/views/settings/emails/emails.test.js |
Adjusts Emails tests to mock EmailPreview and provide ResizeObserver. |
includes/reader-activation/integrations/README.md |
Adds documentation for the reader activation integrations framework. |
Comments suppressed due to low confidence (3)
includes/wizards/newspack/class-email-preview.php:114
get_sample_substitutions()usesfunction_exists( 'Newspack\Emails::get_reply_to_email' ), butfunction_existsdoes not work for class methods, so this condition will always be false and the preview will ignoreEmails::get_reply_to_email()(and any Reader Activation override). Usemethod_exists( Emails::class, 'get_reply_to_email' )oris_callable( [ Emails::class, 'get_reply_to_email' ] )instead, or just callEmails::get_reply_to_email()directly since this class already depends onNewspack\Emails.
$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' );
$site_address = self::get_site_address();
includes/wizards/newspack/class-email-preview.php:127
- The
*CONTACT_EMAIL*substitution builds an<a href="mailto:...">string using$reply_to_emailwithout escaping/sanitizing. Even if the value is expected to be an email, it should be passed throughsanitize_email()and escaped for both thehrefattribute and link text to avoid HTML injection issues and to keep output consistent with WP escaping practices.
'*SITE_ADDRESS*' => $site_address,
'*SITE_CONTACT*' => $site_contact,
'*CONTACT_EMAIL*' => sprintf( '<a href="mailto:%s">%s</a>', $reply_to_email, $reply_to_email ),
src/wizards/newspack/views/settings/emails/email-preview.tsx:71
EmailPreviewassumesResizeObserverexists. In environments withoutResizeObserver, this will throw and prevent the iframe from rendering. Add a feature check with a safe fallback (e.g. compute scale fromnode.getBoundingClientRect().widthonce, or defaultscaleto 1 and skip observing).
// Measure container width and compute iframe scale.
useEffect( () => {
const node = containerRef.current;
if ( ! node ) {
return;
}
const ro = new ResizeObserver( ( [ entry ] ) => {
setScale( entry.contentRect.width / IFRAME_WIDTH );
} );
ro.observe( node );
return () => ro.disconnect();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…r APIs (NPPD-1525) Address Copilot review feedback on PR #4730: - PHP: guard template include with is_readable() to avoid warnings if the registered template path is missing or unreadable. - JS: add typeof checks for IntersectionObserver and ResizeObserver so the component degrades gracefully in browsers without these APIs (falls back to eager loading and scale=1 respectively). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Addressed in f889cd6. All six items from the review:
Tests updated: |
kmwilkerson
force-pushed
the
nppd-1525-email-preview-component
branch
from
65d1752 to
ebb58fb
Compare
…r APIs (NPPD-1525) Address Copilot review feedback on PR #4730: - PHP: guard template include with is_readable() to avoid warnings if the registered template path is missing or unreadable. - JS: add typeof checks for IntersectionObserver and ResizeObserver so the component degrades gracefully in browsers without these APIs (falls back to eager loading and scale=1 respectively). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Code review — EmailPreview component (slice 2 of unified emails)Reviewed at Bugs1.
|
|
Addressed in 6b8552b. All actionable findings from the review: Bugs
Security
Resilience
PHP polish
Tests
CSS
Deferred (follow-up tickets)
|
…r APIs (NPPD-1525) Address Copilot review feedback on PR #4730: - PHP: guard template include with is_readable() to avoid warnings if the registered template path is missing or unreadable. - JS: add typeof checks for IntersectionObserver and ResizeObserver so the component degrades gracefully in browsers without these APIs (falls back to eager loading and scale=1 respectively). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
kmwilkerson
force-pushed
the
nppd-1525-email-preview-component
branch
from
6b8552b to
8cac5e6
Compare
Re-review at
|
| # | Finding | Where it landed | Notes |
|---|---|---|---|
| 1 | function_exists() → call Emails::get_reply_to_email() directly |
class-email-preview.php:129 |
$reply_to_email = Emails::get_reply_to_email(); — guard removed. Pinned by test_contact_email_uses_reply_to_email (tests/unit-tests/email-preview.php:99-112), which filters newspack_reply_to_email and asserts the custom address resolves into the preview HTML. |
| 2 | Document the allow-same-origin trade-off |
email-preview.tsx:196-201 |
Multi-line comment block above the sandbox attribute explains: (a) why same-origin is needed (contentDocument access), (b) why scripts are still blocked, (c) the residual form-action risk. Reads exactly as I'd want a future reader to encounter it. |
| 3 | Cancel in-flight fetch on postId change / unmount |
email-preview.tsx:96-134 |
let cancelled = false; checked in .then / .catch / .finally. Effect returns a cleanup that flips it. Mirrors the pattern in NewsletterPreview. |
| 4 | Clear safety timeout on cleanup | email-preview.tsx:35, 99-102, 128-134, 163-166, 172 |
safetyTimerRef = useRef<…>(null) — cleared in the fetch-effect cleanup AND inside finalize(). Belt and suspenders. |
| 5 | Single source of truth for finalize | email-preview.tsx:157-169 |
let finalized = false; const finalize = () => { if ( finalized ) return; finalized = true; … }. No double state writes. |
| 6 | onError on iframe |
email-preview.tsx:206 |
onError={ () => setHasError( true ) }. |
| 7 | Initial-paint blank-frame | email-preview.tsx:40, 177, 191 |
scale defaults to null; iframe is gated by scale !== null && scale > 0; spinner gated by a unified showSpinner = ! hasError && ! isReady && ( isLoading || Boolean( html ) ). Covers loading + fetched-but-not-ready + the post-ResizeObserver pre-scale frame. |
| 8 | Escape *CONTACT_EMAIL* |
class-email-preview.php:142 |
sprintf( '<a href="%s">%s</a>', esc_url( 'mailto:' . $reply_to_email ), esc_html( $reply_to_email ) ). |
| 9 | wp_date() instead of gmdate() |
class-email-preview.php:155 |
wp_date( get_option( 'date_format', 'F j, Y' ) ) — site timezone + locale. |
| 11 | strtr instead of 29 str_replace passes |
class-email-preview.php:117 |
Single-pass substitution. |
| 12 | Filter hook on substitutions | class-email-preview.php:106-115 |
apply_filters( 'newspack_email_preview_substitutions', $substitutions, $post_id ) with a real docblock explaining when to use it. apply_sample_substitutions() signature widened to accept $post_id so the filter has context. |
| 16 | PHPUnit test for finding (1) | tests/unit-tests/email-preview.php:99-112 |
See finding (1) above. |
| 17 | Permission-check negative test | tests/unit-tests/email-preview.php:155-159 |
Subscriber → WP_Error. |
| 18 | 404 test asserts the status | tests/unit-tests/email-preview.php:174 |
assertEquals( 404, $response->get_error_data()['status'] ) added next to the error-code assertion. |
| 19 | Race-condition Jest test | email-preview.test.js:178-215 |
Excellent — controlled firstPromise, second fetch resolves first, then the stale first resolves and is not allowed to overwrite the iframe. The exact race I described. Clean assertion: srcDoc must contain "Second email" and NOT "First email". |
| 21 | getByRole('presentation') → .newspack-email-preview__placeholder querySelector |
email-preview.test.js:80, 114 |
Replaced in both the loading and error tests. No longer depends on the implementation detail of WordPress Spinner's role. |
| 22 | Hardcoded #949494 → design token |
email-preview.scss:1, 38 |
@use "~@wordpress/base-styles/colors" as wp-colors; import added; color: wp-colors.$gray-400. |
Deferred (acknowledged in author response)
- (10) Template-path hardening —
include $template_pathis gated byis_readable()only; the attacker bar is "compromise thenewspack_email_configsfilter," which already implies PHP execution capability, so a follow-uprealpath()-within-known-dir check is defense-in-depth rather than load-bearing. Reasonable to defer. - (13) Extract
Emails::get_site_address()shared helper — cosmetic dedupe withEmails::get_email_payload()lines 186-211. Drift risk exists but is small. - (27) Data-store consolidation alongside NPPD-1531 — same scope as the slice 1 follow-up.
Two small new observations
A. apply_sample_substitutions $post_id = 0 default is unreachable
includes/wizards/newspack/class-email-preview.php:103
private static function apply_sample_substitutions( string $html, int $post_id = 0 ): string {The method is private static and only called from get_preview_html() (line 44), which always passes the actual $post_id. The = 0 default never fires. Tiny nit — either drop the default, or make the method public so the filter can be invoked standalone for ad-hoc previews. Whichever, it'd be more honest about intent.
B. test_contact_email_uses_reply_to_email cleans up via remove_all_filters at the end
tests/unit-tests/email-preview.php:111
remove_all_filters( 'newspack_reply_to_email' );If the assertions above it fail, the filter leaks to subsequent tests in the class — every later Emails::get_reply_to_email() call would still return reply@example.org. Belt-and-suspenders: move the removal into a tear_down() override, or wrap the test body in try { … } finally { remove_all_filters( … ); }. Minor — the test passes today.
C. Jest skip note for safety-timeout / iframe onError is the right call
src/wizards/newspack/views/settings/emails/email-preview.test.js:217-221
The comment at the bottom explains exactly why these two defensive paths aren't unit-tested:
jsdom automatically fires the iframe load event when srcDoc is set, which prevents us from simulating pending-asset scenarios.
That's correct — and it's the kind of transparency a future maintainer needs. If the project ever spins up Playwright/Cypress for the wizards screen, this is the place to add the two missing scenarios. No action for this PR.
D. README cherry-pick still present in the diff
includes/reader-activation/integrations/README.md
Same as last round — falls out when PR #4727 merges. No action.
Verdict
Ship-ready. The fixup is comprehensive: every behavioural fix lands cleanly, the race-condition test is genuinely good (catches the exact bug it's named for, not a proxy), and the security comment is the kind of inline rationale a future reader needs. The three deferred items are all reasonable for follow-up tickets.
Original review for the audit trail: #4730 (comment)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
tests/unit-tests/email-preview.php:112
remove_all_filters( 'newspack_reply_to_email' )is very broad and can break unrelated tests (or even plugin bootstrap behavior) that rely on that filter later in the same PHPUnit process. Prefer removing only the callback added in this test (e.g. keep a reference to the closure and call remove_filter with it).
$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' );
}
Backend:
- New Email_Preview class registers GET /wizard/newspack-settings/emails/{id}/preview
- Renders cached EMAIL_HTML_META with sample-value token substitution
- Template fallback for emails with no rendered HTML stored
- Reader/transaction tokens use stable fake values; site/branding tokens use
real publisher values; action URLs use # anchors
Frontend:
- React component with IntersectionObserver lazy-loading via isVisible bridge
- Iframe srcDoc rendering with sandbox="" for security
- Spinner loading state, envelope icon fallback on error
- Component exported; not yet integrated into emails.tsx
Tests:
- 6 PHPUnit tests
- 5 Jest tests
Also fixes a missing tab indentation in includes/class-newspack.php on the
new class-email-preview.php include line.
Replaces the envelope-icon preview placeholder with the live EmailPreview component built in 49b5bae1e. Each card now renders a lazy-loaded iframe thumbnail of the rendered email HTML with sample-value token substitution.
…-1525) CSS container query units (cqw) produce length values, but scale() requires a unitless number. The browser silently discarded the invalid transform, rendering the 600px iframe at full size with only the logo visible through overflow:hidden. Switch to ResizeObserver to measure the container width and set the scale as an inline style. Also removes the dead .newspack-emails__preview-placeholder rule replaced by the EmailPreview component, and restructures the SCSS to use BEM nesting with responsive container sizing (aspect-ratio 4/3). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
PHP lint: - class-email-preview.php line 139: auto-fix array double arrow alignment - tests/email-preview.php lines 93/109/123: capitalize docblock summaries Jest: - email-preview.tsx: fix IntersectionObserver TDZ crash. The newspack-scripts Jest mock invokes the observer callback synchronously inside the constructor, before the const observer assignment completes. Moved disconnect logic out of the callback and into the effect cleanup, gated by isVisible dependency. - email-preview.test.js, emails.test.js: add ResizeObserver mock for JSDOM. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Lint: - email-preview.tsx line 47: Prettier wants single arrow param without parens Test: - emails.test.js: mock EmailPreview to render null. The slice 1 test renders the full DataViews grid, which now includes EmailPreview in every card via the wire-in commit 5898082. EmailPreview's fetch effect hangs in the test because the slice 1 test mocks apiFetch only for the settings endpoint, not the preview endpoint. Mocking the component is the right pattern — its own behavior is covered by email-preview.test.js. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…r APIs (NPPD-1525) Address Copilot review feedback on PR #4730: - PHP: guard template include with is_readable() to avoid warnings if the registered template path is missing or unreadable. - JS: add typeof checks for IntersectionObserver and ResizeObserver so the component degrades gracefully in browsers without these APIs (falls back to eager loading and scale=1 respectively). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…act (NPPD-1525) Address thomasguillot review feedback: 848px source viewport, 1:1 aspect ratio, fade-in via is-ready class with opacity transition, iframe height measured from contentDocument after stylesheets/images load (8s safety timeout), state reset on postId change. Adds tests for is-ready class and reset-on-postId-change behaviour. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…is accessible (NPPD-1525) With sandbox="" the iframe is treated as a unique opaque origin, making contentDocument null. The onLoad handler returned early and never set isReady, leaving the spinner visible indefinitely. allow-same-origin is safe here because allow-scripts is not set. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- 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>
kmwilkerson
force-pushed
the
nppd-1525-email-preview-component
branch
from
8cac5e6 to
38c03ce
Compare
…D-1525) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…email-preview-component
dkoo
left a comment
dkoo
left a comment
There was a problem hiding this comment.
@kmwilkerson Great work—tests well and looks great in-browser! Commented inline with some minor blockers and a handful of non-blocking suggestions. The ones with a red 🔴 are the most important comments; others are less important and could be deferred if necessary.
Will review #4727 separately.
| * | ||
| * @return string Formatted site address, or empty string. | ||
| */ | ||
| private static function get_site_address(): string { |
There was a problem hiding this comment.
🟡 Non-blocking suggestion: extract formatted site address string to a shared helper method
This method replicates some logic in the Emails::get_email_payload() method almost exactly (the docblock even says so!). This creates some redundancy and could result in mismatching output if the Emails::get_email_payload() logic ever changes. If we move the logic into a new helper method (e.g. Emails::get_site_address()) that both Email_Preview and Emails classes can use, this simplifies things and prevents future drift.
| ? sprintf( '<strong>%s</strong> — %s', $site_title, $site_address ) | ||
| : $site_title; | ||
|
|
||
| return [ |
There was a problem hiding this comment.
🟡 Non-blocking: static token map could get out-of-sync
This is probably okay for the initial implementation due to how we've added placeholder tokens sort of ad hoc for each email template as needed, but the static map here has potential for easily getting out-of-sync if we add/remove placeholders or make any changes to existing ones in the future.
To properly address this, we'd likely need to make some refactors to how we're adding placeholder tokens in each email template's settings object. We'd want some kind of API to register placeholders in a single shared map that would include config such as the sample value and maybe a data replacement type + sanitization callback. Then, email template configs would reference placeholders from this shared map so we could be sure all email templates are using the same placeholders in the same way.
I'd say this is non-blocking because it would balloon the scope of this PR, but I'd strongly consider a separate follow-up PR to revisit and standardize how we handle all the different placeholders.
| '*BILLING_FIRST_NAME*' => 'Sample', | ||
| '*BILLING_LAST_NAME*' => 'Reader', | ||
| '*BILLING_NAME*' => 'Sample Reader', | ||
| '*PENDING_EMAIL_ADDRESS*' => 'sample.reader@example.com', | ||
|
|
||
| // Transaction / subscription details — stable sample values. | ||
| '*AMOUNT*' => '$25.00', | ||
| '*PAYMENT_METHOD*' => 'Visa ending in 4242', | ||
| '*PRODUCT_NAME*' => 'Monthly Membership', | ||
| '*BILLING_FREQUENCY*' => 'monthly', | ||
| '*DATE*' => wp_date( get_option( 'date_format', 'F j, Y' ) ), | ||
| '*CANCELLATION_TITLE*' => __( 'Subscription Cancelled', 'newspack-plugin' ), | ||
| '*CANCELLATION_TYPE*' => __( 'subscription', 'newspack-plugin' ), |
There was a problem hiding this comment.
🔴 Make all sample strings translatable
Some sample strings here are translatable while others aren't. Let's put them all through __() for consistency.
| '*BILLING_FIRST_NAME*' => 'Sample', | |
| '*BILLING_LAST_NAME*' => 'Reader', | |
| '*BILLING_NAME*' => 'Sample Reader', | |
| '*PENDING_EMAIL_ADDRESS*' => 'sample.reader@example.com', | |
| // Transaction / subscription details — stable sample values. | |
| '*AMOUNT*' => '$25.00', | |
| '*PAYMENT_METHOD*' => 'Visa ending in 4242', | |
| '*PRODUCT_NAME*' => 'Monthly Membership', | |
| '*BILLING_FREQUENCY*' => 'monthly', | |
| '*DATE*' => wp_date( get_option( 'date_format', 'F j, Y' ) ), | |
| '*CANCELLATION_TITLE*' => __( 'Subscription Cancelled', 'newspack-plugin' ), | |
| '*CANCELLATION_TYPE*' => __( 'subscription', 'newspack-plugin' ), | |
| '*BILLING_FIRST_NAME*' => __( 'Sample', 'newspack-plugin' ), | |
| '*BILLING_LAST_NAME*' => __( 'Reader', 'newspack-plugin' ), | |
| '*BILLING_NAME*' => __( 'Sample Reader', 'newspack-plugin' ), | |
| '*PENDING_EMAIL_ADDRESS*' => __( 'sample.reader@example.com', 'newspack-plugin' ), | |
| // Transaction / subscription details — stable sample values. | |
| '*AMOUNT*' => __( '$25.00', 'newspack-plugin' ), | |
| '*PAYMENT_METHOD*' => __( 'Visa ending in 4242', 'newspack-plugin' ), | |
| '*PRODUCT_NAME*' => __( 'Monthly Membership', 'newspack-plugin' ), | |
| '*BILLING_FREQUENCY*' => __( 'monthly', 'newspack-plugin' ), | |
| '*DATE*' => wp_date( get_option( 'date_format', 'F j, Y' ) ), | |
| '*CANCELLATION_TITLE*' => __( 'Subscription Cancelled', 'newspack-plugin' ), | |
| '*CANCELLATION_TYPE*' => __( 'subscription', 'newspack-plugin' ), |
| }, [ isVisible, postId ] ); | ||
|
|
||
| // Handle iframe load: wait for stylesheets and images, then measure height and reveal. | ||
| const handleIframeLoad = useCallback( () => { |
There was a problem hiding this comment.
🟡 Stale iframe load handler (flagged by 2/6)
The fetch effect cancels stale network responses, but handleIframeLoad() has no generation guard. If postId changes mid-load, the previous iframe's Promise.all([...linkPromises, ...imgPromises]) resolves later and finalize() sets isReady/iframeHeight against the new preview, resulting in visual flash and wrong dimensions.
Suggested fix: Add a per-load ref and bail in finalize if it doesn't match the current load.
| '*SITE_TITLE*' => $site_title, | ||
| '*SITE_URL*' => $site_url, | ||
| '*SITE_LOGO*' => $site_logo_url ? esc_url( $site_logo_url ) : '', | ||
| '*SITE_ADDRESS*' => $site_address, | ||
| '*SITE_CONTACT*' => $site_contact, |
There was a problem hiding this comment.
🔴 Escape raw text tokens
These sample strings come from options or theme mods and are interpolated raw into HTML before reaching srcDoc. Escape all output for tighter security:
| '*SITE_TITLE*' => $site_title, | |
| '*SITE_URL*' => $site_url, | |
| '*SITE_LOGO*' => $site_logo_url ? esc_url( $site_logo_url ) : '', | |
| '*SITE_ADDRESS*' => $site_address, | |
| '*SITE_CONTACT*' => $site_contact, | |
| '*SITE_TITLE*' => esc_html( $site_title ), | |
| '*SITE_URL*' => esc_url( $site_url ), | |
| '*SITE_LOGO*' => $site_logo_url ? esc_url( $site_logo_url ) : '', | |
| '*SITE_ADDRESS*' => esc_html( $site_address ), | |
| '*SITE_CONTACT*' => esc_html( $site_contact ), |
| 'label' => __( 'Test preview config', 'newspack' ), | ||
| 'description' => __( 'Email for testing preview.', 'newspack' ), |
There was a problem hiding this comment.
🔴 Wrong text domain
| 'label' => __( 'Test preview config', 'newspack' ), | |
| 'description' => __( 'Email for testing preview.', 'newspack' ), | |
| 'label' => __( 'Test preview config', 'newspack-plugin' ), | |
| 'description' => __( 'Email for testing preview.', 'newspack-plugin' ), |
| setIframeHeight( null ); | ||
| setHasError( false ); | ||
| setHtml( null ); | ||
| apiFetch< { html: string; post_id: number } >( { |
There was a problem hiding this comment.
🟡 Performance: redundant fetches on DataView re-renders
Currently, most DataViews remounts (pagination/sort/search) will unnecessarily re-fetch the same postIds. Also, changing the view style to "Table" instead of "Grid" will trigger preview re-fetches even though this view doesn't show any thumbnails.
Suggested fix: Cache fetched preview HTML in a map by postId (maybe in the parent component so it persists without a memo/ref) and load from the cache if available to avoid redundant REST calls.
There was a problem hiding this comment.
🟡 Non-blocking: consider useWizardApiFetch instead
Newer wizard views use this shared hook instead of calling apiFetch directly; consider updating to match.
| const fetchData = useCallback( () => { | ||
| setIsLoading( true ); | ||
| setError( null ); | ||
| apiFetch< EmailSettings >( { |
There was a problem hiding this comment.
🟡 Non-blocking: consider useWizardApiFetch instead
Same as above: newer wizard views use this shared hook instead of calling apiFetch directly; consider updating to match.
| * publisher-controlled post meta. */ | ||
| sandbox="allow-same-origin" | ||
| tabIndex={ -1 } | ||
| title="Email preview" |
There was a problem hiding this comment.
🟡 Untranslated string
Let's make sure raw strings are translatable. Note: will need to import __ from @wordpress/i18n
| title="Email preview" | |
| title={ __( 'Email preview', 'newspack-plugin' ) } |
dkoo
added
the
[Status] Needs Changes or Feedback
What this PR does
Adds an inline email preview component (NPPD-1525) that renders a live thumbnail of each email's HTML inside the DataViews grid built in #4727. Replaces the envelope-icon placeholder with real previews — readers see the actual rendered template at a glance.
Base branch is
nppd-945-unified-emails-newspack-slice, nottrunk.The diff in this PR shows only the NPPD-1525 work. The slice 1 changes (
#4727) are excluded from the diff by virtue of the base branch.Code changes
Backend (
includes/wizards/newspack/class-email-preview.php)Email_Previewclass registersGET /wizard/newspack-settings/emails/{post_id}/previewEMAIL_HTML_METApost meta with sample-value token substitution appliedget_option; action URLs use#anchorsFrontend (
src/wizards/newspack/views/settings/emails/)EmailPreviewReact componentisVisiblestate bridge — cards only fetch when scrolled into view, avoiding 24 simultaneous requests on initial rendersrcDocrendering with emptysandbox=""for security (scripts in email HTML cannot execute)transformto match container width while preserving 4:3 aspect ratioemails.tsxto replace the envelope-icon placeholder in the Preview columnLoader fix (
includes/class-newspack.php)class-email-preview.phpinclude lineTests
tests/unit-tests/email-preview.php): substitution map, preview HTML with stored meta, template fallback, nonexistent post, wrong post type returns 404, successful API responsesrc/wizards/newspack/views/settings/emails/email-preview.test.js): loading state, successful iframe render, error fallback, lazy-load gating, correct endpoint pathManual testing
Tested locally on katie.local (Homebrew-served Apache + PHP, symlinked plugin) with the slice 1 grid view. Verified:
All Submissions: