Premium Analytics: Add email post detail views - #50545
Conversation
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
# Conflicts: # projects/packages/premium-analytics/routes/post-detail/config/tab-layouts.test.ts
The email tabs only show for posts actually sent to subscribers: total_sends from the per-post opens rate summary is the send signal (the same query the Email top row reads, so React Query shares the result). Calypso infers availability from subscription settings and post metadata because it decides before fetching; the summary is the direct source. Fails closed while loading or errored, and deep links to a gated tab fall back through the existing hidden-tab path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Code Coverage SummaryThis PR did not change code coverage! That could be good or bad, depending on the situation. Everything covered before, and still is? Great! Nothing was covered before? Not so great. 🤷 |
- Fix the rate tiles: the endpoint returns 0-1 fractions (Calypso multiplies by 100), so stop dividing by 100 - 2 sends / 2 opens now reads 100%, not 1%. - Design strings: the top row becomes 'Newsletter performance' with Sent / Total unique opens / Total opens|clicks / Open|Click rate. - Drop the header dropdowns: the top row's metric switch duplicates the tab split and the breakdown's view selector fights the fixed composition, so both attributes return to default (low) relevance. - Per-card design titles: the host titles a card by widget type, so the page clones jpa/email-breakdown into page-local aliases (Location/ Platforms/Clients opens|clicks, Top links) that reuse the resolved render module. - Email clicks grid becomes 3/1 + 2/2. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The non-map row wrapper sat inside WidgetState's plain-block ready container, so its flex sizing was inert and the row grew to content height - overflow was clipped by the root instead of scrolling. Give it the same definite block-size the map variant already had, so the leaderboard scrolls internally in every view. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The filters now sit directly under the tabs, mirroring the main dashboard's placement, with the summary header on its own row below. Restores SectionTabs' default bottom margin (the header row no longer owns that spacing) and drops the header's since-obsolete side-by-side responsive layout. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
16px top padding stacks with the filters row's 16px bottom padding to 32px above the heading; 32px below mirrors it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
chihsuan
left a comment
There was a problem hiding this comment.
Thanks for this @dognose24
This PR seems cover a few separate concerns: the rate fix, reusable Email widget enhancements, Post Details layout changes, and the final tab integration.
Would you be open to splitting these into smaller PRs? I think it would make the changes easier to review and reduce the risk of unrelated regressions. 🙏
| export function usePostDetailTabs() { | ||
| export function usePostDetailTabs( postId: number ) { | ||
| const opens = useStatsEmailOpensBreakdown( postId, 'rate', { enabled: postId > 0 } ); | ||
| const summary = ( opens.data as StatsEmailBreakdown | undefined )?.summary; |
There was a problem hiding this comment.
While this query is loading, data is undefined, so an email-opens deep link gets replaced with post-traffic before we know whether Email stats exist. Should we wait for the query to settle before normalizing the URL?
There was a problem hiding this comment.
Good catch — this was real: the normalization effect fired during the first load and rewrote a legitimate email-opens deep link before the gate knew whether email stats exist.
Fixed in d2e94a2: the hidden-tab URL normalization now waits for the send summary to settle (isLoading is false while the query is disabled and once it resolves or errors, so only the first load holds it off). The visible fallback still renders immediately — only the URL write waits. Added a regression test for the loading + deep-link case.
This reverts commit cac0788.
This reverts commit 90b1022.
- Revert the email rate fraction fix here; it moves to its own PR (#50563) per review. - Hold the hidden-tab URL normalization until the email gate's send summary settles: rewriting while it loaded bounced a legitimate email-tab deep link to post-traffic before we knew whether email stats exist. The visible fallback still renders immediately. The date-filters relocation was likewise split out to #50562 (reverted in the two commits before this one). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks for the review @chihsuan! Split done where the pieces were genuinely independent:
I kept the email widget enhancements and the tab integration together: the fixed compositions directly reference the new widget surface (the Also fixed the deep-link race you flagged inline — good catch. The URL normalization now waits for the send summary to settle. |
kangzj
left a comment
There was a problem hiding this comment.
Thorough review of the package (full-file reads, JS tests, typecheck). Findings from the review of cac0788:
- [blocker — already fixed in d2e94a2] Deep links / refreshes on an email tab bounced to Post traffic: the hidden-tab URL normalization raced the send-summary gate query and always won on a cold load. The
gateSettled = ! opens.isLoadingguard resolves it, and I verified the disabled-query edge: on @tanstack/react-query v5, a disabled query (no post scope) reportsisLoading: false, so normalization still runs there. The added regression test covers the loading deep-link case. ✅ - [moot] A stale test name (“converts the 0–100 rate”) flagged against the 0–1 fraction change no longer applies after the rate handling was split out and reverted in d2e94a2.
- [suggestion] One minor note posted inline: the hidden country map still mounts Google Charts below 720px.
Also verified: changelog entry valid; i18n domains correct; new CSS uses logical properties with a proper container-type ancestor; jpa/email-breakdown / jpa/email-top-row have no consumers outside the post detail page, so the relevance removals and layout wrapper are safe; package tests (32 PR-scoped) and typecheck pass locally. The one unrelated local test failure (WordAds window clamp in packages/data) is byte-identical to trunk and green in CI.
LGTM once the inline note is considered — nothing blocking remains.
| showLegend={ false } | ||
| dataFormat={ DATA_FORMAT } | ||
| /> | ||
| { renderMap && ( |
There was a problem hiding this comment.
[suggestion] Below a 720px container the map is display: none (container query in style.module.css), but GeoChart still mounts here, so narrow layouts pay the Google Charts load for a chart that's never visible. Fine to defer — a JS-side width check or lazy-mount on visibility would avoid the dead weight.
There was a problem hiding this comment.
Went ahead and implemented it in 84b8a01 rather than deferring — GeoChart now mounts only once the measured container is ≥ the same 720px the CSS query needs to show it (ResizeObserver on the widget root, first paint defaults to unmounted), so a narrow layout never loads Google Charts at all. CSS still owns the visual fallback.
kangzj
left a comment
There was a problem hiding this comment.
Approving — the deep-link race flagged earlier is fixed in d2e94a2 and nothing blocking remains (full findings in my review above). The PR is on the larger side, but given the deadline I'm happy for it to merge as is; the one remaining inline note (lazy-mounting the hidden map) can land as a follow-up.
chihsuan
left a comment
There was a problem hiding this comment.
Thanks for the thoughtful update! @dognose24
I left one concern about the email-tab gate treating a failed request as a completed availability check but it's minor.
The rest of the data merging and shared-query approach looks sensible to me. 🙏
Pre-approving.
| const hasEmailStats = Number( summary?.total_sends ?? 0 ) > 0; | ||
| // `isLoading` is false while the query is disabled (no post scope) and once | ||
| // it settles (data or error), so this only holds off during the first load. | ||
| const gateSettled = ! opens.isLoading; |
There was a problem hiding this comment.
One small concern: if this request fails or times out, isLoading becomes false while the data is still unavailable. We would then hide the Email tabs and rewrite an Email deep link to Post traffic, even though we don’t know whether the post has email stats yet.
Could we preserve the requested section on error and only normalize the URL after a successful response?
There was a problem hiding this comment.
Fixed in 84b8a01 — normalization now requires the query to have succeeded (opens.isSuccess), not merely settled: on a failed/timed-out request the URL keeps the requested email section (the tabs stay hidden, fail-closed, and a later successful refetch settles it). Non-email deep links don't depend on the gate and normalize right away. Added a regression test for the errored deep-link case.
…ages (#50563) The per-post stats/<opens|clicks>/emails/<id>/rate summary reports rates as 0-1 fractions (wp-calypso renders rate * 100), but the widget divided by 100 again - 2 sends / 2 opens rendered as 1% instead of 100%. Pass the fraction straight to the percentage formatter, and align the test and Storybook fixtures with the real payload shape. Split out of #50545 per review. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
- An email deep link is only normalized after the send-summary query succeeds: a failed/timed-out request can't tell us whether the post has email stats, so the URL keeps the requested section (tabs stay hidden, a later successful refetch settles it). Non-email deep links don't depend on the gate and normalize right away. - Mount GeoChart only when the measured container is at least the 720px the CSS container query needs to show it, so narrow layouts never pay the Google Charts load for an invisible map. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ail-detail # Conflicts: # projects/packages/premium-analytics/widgets/email-top-row/__tests__/email-top-row.test.tsx
#50562) * Post detail: move the date filters below the tab bar The filters now sit directly under the tabs, mirroring the main dashboard's placement, with the summary header on its own row below (16px + 16px stacking to a symmetric 32px around the heading). Restores SectionTabs' default bottom margin and drops the header's since-obsolete side-by-side responsive layout. Split out of #50545 per review — this affects the whole post detail page, not only the email tabs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Address review: use the gap-2xl token for the header's bottom padding Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Part of WOOA7S-1623.
Proposed changes
total_sendsfrom the per-post opens rate summary is positive — the same query the Email top row reads, so React Query shares the result and the gate costs no extra request. It fails closed (hidden while loading/errored), and deep links to a gated tab fall back through the existing hidden-tab path. Calypso infers the same availability from subscription settings, post metadata, and a 2023-05-30 date cutoff because it decides before fetching; the send summary is the direct signal.jpa/email-breakdown— the host titles a card by widget type, so the fixed page clones the resolved type (render module and all) under variant names that can never leak into a widget gallery (this page has none).Related product discussion/links
Does this pull request change what data or activity we track or use?
No. It presents existing email Stats API data in the post detail page.
Testing instructions
pnpm test -- --runInBandandpnpm run typecheckfromprojects/packages/premium-analytics.pnpm run buildfromprojects/packages/premium-analytics.Screenshots
Email Opens
Email Clicks