Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion client/my-sites/stats/controller.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,13 @@ import AsyncLoad from 'calypso/components/async-load';
import { bumpStat } from 'calypso/lib/analytics/mc';
import { getSiteFragment, getStatsDefaultSitePage } from 'calypso/lib/route';
import { getMomentSiteZone } from 'calypso/my-sites/stats/hooks/use-moment-site-zone';
import { getSite } from 'calypso/state/sites/selectors';
import isJetpackModuleActive from 'calypso/state/selectors/is-jetpack-module-active';
import { getSite, isSimpleSite } from 'calypso/state/sites/selectors';
import getEnvStatsFeatureSupportChecks from 'calypso/state/sites/selectors/get-env-stats-feature-supports';
import { setNextLayoutFocus } from 'calypso/state/ui/layout-focus/actions';
import { getCurrentLayoutFocus } from 'calypso/state/ui/layout-focus/selectors';
import { getSelectedSiteId } from 'calypso/state/ui/selectors';
import { postEmailStatsAvailabilityQueryOptions } from './hooks/use-post-email-stats-availability-query';
import { rangeOfPeriod, getSiteFilters } from './pages/shared/helpers';
import PageLoading from './pages/shared/page-loading';
import StatsSite from './site';
Expand Down Expand Up @@ -416,6 +419,21 @@ export function post( context, next ) {
return next();
}

// Resolve email tab availability in parallel with the chunk download, so the
// tab strip doesn't pop in after the page renders.
const state = context.store.getState();
const { supportsEmailStats } = getEnvStatsFeatureSupportChecks( state, siteId );
// Module state is often not loaded yet at route time (QueryJetpackModules runs
// after mount, and /me/sites does not carry active_modules), so only a definite
// "inactive" skips the warm-up; the page's own `enabled` check stays strict.
const canHaveEmailStats =
!! supportsEmailStats &&
( isSimpleSite( state, siteId ) ||
isJetpackModuleActive( state, siteId, 'subscriptions', true ) !== false );
Comment thread
dognose24 marked this conversation as resolved.
Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment slightly overpromises here I think - with useFallback: true the selector discards a definite active: false from the modules store and falls through to active_modules, which is null when the site options don't carry it (and /me/sites doesn't request it). So a site with subscriptions provably off can still prefetch. It's only a wasted request since the page's enabled stays strict - same null-vs-false footgun as in #113843. Maybe check the module store result directly before falling back, or just soften the comment. Not a blocker.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c264fc4 — the gate now reads the module store directly first and only falls back when that is null, so a definite inactive skips the prefetch; comment updated to match.

if ( canHaveEmailStats && postId > 0 ) {
context.queryClient.prefetchQuery( postEmailStatsAvailabilityQueryOptions( siteId, postId ) );
}

context.primary = (
<StatsPageLoader>
<AsyncLoad
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { useQuery } from '@tanstack/react-query';
import { queryOptions, useQuery, type QueryClient } from '@tanstack/react-query';
import wpcom from 'calypso/lib/wp';
import getDefaultQueryParams from './default-query-params';

Expand All @@ -15,20 +15,11 @@ function queryEmailRate( siteId: number | null, postId: number ): Promise< Email
return wpcom.req.get( `/sites/${ siteId }/stats/opens/emails/${ postId }/rate` );
}

/**
* Whether a post was ever sent as a newsletter email, based on the email stats themselves
* rather than post metadata, which does not always reflect what was actually sent.
*/
export default function usePostEmailStatsAvailabilityQuery(
siteId: number | null,
postId: number,
enabled = true
) {
return useQuery( {
export function postEmailStatsAvailabilityQueryOptions( siteId: number | null, postId: number ) {
return queryOptions( {
...getDefaultQueryParams(),
queryKey: [ 'stats', 'emails', 'opens', 'rate', siteId, postId ],
queryFn: () => queryEmailRate( siteId, postId ),
enabled: !! enabled && !! siteId && postId > 0,
// A "no email stats" answer can be transient while a newsletter is still being sent,
// so only a positive result is kept for a while.
staleTime: ( query ) => ( hasEmailStats( query.state.data ) ? 1000 * 60 * 5 : 1000 * 30 ),
Expand All @@ -37,6 +28,37 @@ export default function usePostEmailStatsAvailabilityQuery(
// (the shared defaults set retryOnMount: false).
retryOnMount: true,
meta: { persist: false },
} );
}

/**
* Mark a post as having email stats without waiting for the request, for navigations
* from a page that already proves they exist (e.g. the email detail tabs). The counts
* are a placeholder; nothing reads them besides hasEmailStats. Real responses win.
*/
export function seedPostEmailStatsAvailability(
queryClient: QueryClient,
siteId: number | null,
postId: number
) {
const { queryKey } = postEmailStatsAvailabilityQueryOptions( siteId, postId );
if ( queryClient.getQueryData( queryKey ) === undefined ) {
queryClient.setQueryData( queryKey, { total_sends: 1 } );
}
Comment thread
dognose24 marked this conversation as resolved.
Comment on lines +54 to +57

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I managed to seed a false positive here - open the Email opens URL directly for a post that was never emailed (that page always renders the tabs), click Post traffic, and the post detail shows the email tabs for a post with no email stats. And because the seed is written as fresh data, its 5-min staleTime suppresses the /rate refetch that would correct it, so "Real responses win" doesn't quite hold - screenshots in the review body.

I think seeding as already-stale data fixes both this and the cached-negative case Copilot flagged:

Suggested change
const { queryKey } = postEmailStatsAvailabilityQueryOptions( siteId, postId );
if ( queryClient.getQueryData( queryKey ) === undefined ) {
queryClient.setQueryData( queryKey, { total_sends: 1 } );
}
const { queryKey } = postEmailStatsAvailabilityQueryOptions( siteId, postId );
queryClient.setQueryData( queryKey, { total_sends: 1 }, { updatedAt: 0 } );

The strip still renders instantly from the cached value, but the query is stale so the mount refetches and the truth wins within one round trip - and that makes it safe to overwrite a stale negative too. It does bring the /rate request back on the Emails → Post traffic hop, tho it runs in parallel and blocks nothing, so probably a fair trade? If you'd rather keep the zero-request behavior I'm happy with a different gate, just not a silent 5-min false positive.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch, and the stale-seed idea is exactly right — took it in c264fc4 with one refinement: a cached entry that already satisfies hasEmailStats is left untouched, so the normal Emails → Post traffic hop keeps its zero-request behavior when a real positive is cached, while everything else (empty cache, stale negative, wrong seed) gets the already-stale placeholder and self-corrects within one round trip. Covered by three new unit tests.

}
Comment thread
Copilot marked this conversation as resolved.

/**
* Whether a post was ever sent as a newsletter email, based on the email stats themselves
* rather than post metadata, which does not always reflect what was actually sent.
*/
export default function usePostEmailStatsAvailabilityQuery(
siteId: number | null,
postId: number,
enabled = true
) {
return useQuery( {
...postEmailStatsAvailabilityQueryOptions( siteId, postId ),
enabled: !! enabled && !! siteId && postId > 0,
select: hasEmailStats,
} );
}
12 changes: 12 additions & 0 deletions client/my-sites/stats/stats-details-navigation/index.tsx
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
import page from '@automattic/calypso-router';
import { useQueryClient } from '@tanstack/react-query';
import { TabPanel } from '@wordpress/components';
import { useTranslate } from 'i18n-calypso';
import PropTypes from 'prop-types';
import { useMemo } from 'react';
import { useSelector } from 'calypso/state';
import { getSelectedSiteId } from 'calypso/state/ui/selectors';
import { seedPostEmailStatsAvailability } from '../hooks/use-post-email-stats-availability-query';

interface StatsDetailsNavigationProps {
postId: number;
Expand All @@ -18,6 +22,8 @@ function StatsDetailsNavigationImproved( {
givenSiteId,
}: StatsDetailsNavigationProps ) {
const translate = useTranslate();
const queryClient = useQueryClient();
const selectedSiteId = useSelector( getSelectedSiteId );
const tabs = useMemo(
() => ( {
highlights: translate( 'Post traffic' ),
Expand Down Expand Up @@ -55,6 +61,12 @@ function StatsDetailsNavigationImproved( {
if ( tabName !== selectedTab ) {
const tab = tabPanelTabs.find( ( tab ) => tab.name === tabName );
if ( tab?.path ) {
if ( tabName === 'highlights' && [ 'opens', 'clicks' ].includes( selectedTab ) ) {
// Being on an email tab proves this post has email stats; seed the
// availability cache so the post detail page shows the tabs
// without waiting for the /rate request.
seedPostEmailStatsAvailability( queryClient, selectedSiteId, postId );
}
Comment thread
dognose24 marked this conversation as resolved.
page( tab.path );
}
}
Expand Down
Loading