Premium Analytics: add a Popular post widget and share the post highlight card - #50931
Conversation
louwie17
added
Enhancement
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
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! |
Code Coverage SummaryCoverage changed in 3 files.
5 files are newly checked for coverage.
Full summary · PHP report · JS report Coverage check overridden by
Covered by non-unit tests
|
|
Pushed 88f8723 fixing the two rendering bugs found in local testing. Root cause. Confirmed, with one correction to the initial diagnosis.
That second row is exactly the reported "labels with no values". What the prototype does. Its card carries Fix. Height-awareness via container queries on a card-owned size container (the shared Verified in Storybook against the real dashboard host at 8 geometries (widths 331/736/1546 x bodies 86/186/510). Every one now reports zero overflow with all three labels and all three values fully visible, checked against every clipping ancestor rather than just the card root. New Typecheck, eslint and stylelint are clean; Jest is 1457/1458, the one failure being the pre-existing |
There was a problem hiding this comment.
Took this for a spin on a local docker env (site timezone UTC-12) — the shared PostHighlightCard extraction is clean and the widget structure follows the package conventions nicely. Five findings, two confirmed locally; recordings/screenshots to follow in a comment.
1. Winner change briefly renders the previous post's likes/comments. On the first range change that produces a new winner, the new post's title rendered with the previous post's engagement numbers before updating (reproduced reliably by delaying the stats/post proxy request with an apiFetch middleware; later switches show a full skeleton instead, so the window is the cross-hook transition — the top-posts winner updates before the dependent stats-post query resets — possibly compounded by placeholder data). Fix suggestion either way: only consume metrics confirmed to belong to the current postId (skeleton otherwise), plus a test that changes the range to a new winner and asserts the old engagement never renders.
2026-07-31.9.36.18.mov
2. Mixed tile windows. views is range-scoped (top-posts row) while likes/comments are lifetime (the stats post endpoint takes no range) — three tiles side by side under the date pills with no hint they differ. Suggestion: make all three all-time — read views from the same stats-post response, let the range only pick the winner. That's consistent with Latest post (the sibling sharing this card is already all-time), drops the top-posts views dependency, and needs no backend work — and it's exactly what the legacy Calypso card already does: client/my-sites/stats/sections/all-time-highlights-section/post-cards-group.tsx deliberately reads lifetime views from stats/post ("Align the most popular post views count with the Post Details page … to avoid confusion") and puts the window in the card copy ("in the past year"). Longer term this should follow WOOA7S-1787's still-open date-model question (all-time + yearly; custom ranges TBD) — if custom ranges win, range-scoping engagement needs backend support on the stats post endpoint.
3. (FYI, fine as-is) POPULAR_POST_REQUEST_MAX = 20: on page-heavy sites the top 20 ranked rows can filter to zero post-type rows and show the empty state even though a qualifying post ranks lower. Current behavior is acceptable — worth a code comment documenting the trade-off. For what it's worth the legacy card shares this latent edge (its topPost comes from the same mixed-type leaderboard) and it has never surfaced there, thanks to the fixed year-long window. If more single-winner widgets are coming, a post_type-filtered top-posts endpoint would remove the root of both this point and point 2.
4. Not in the Insights default layout. After a dashboard reset the widget doesn't appear (dashboard-layout.php untouched) — picker-only. Is that deliberately left to WOOA7S-1786, or should this PR add it? As-is, beta interviewees on the default board won't see the new widget.
5. Unconsumed comparison request. With Compare enabled, the top-posts query fires twice (primary + comparison window) but the widget renders no comparison anywhere. Strip the comparison params the way video-detail-highlights does, with its "no extra comparison request" test pattern.
|
Thanks for working on this! @louwie17 just left a few inline notes. 🙂 Additionally, should we align the default layout with demo/design (could be a follow-up)? Current Design: |
|
@dognose24 & @chihsuan thanks for the reviews, the items should be addressed, this should be good for a re-review.
Related to Dognose's item 4, this will be part of a separate issue WOOA7S-1786 |
chihsuan
left a comment
chihsuan
left a comment
There was a problem hiding this comment.
Thanks for addressing feedback! @louwie17
I just found the default story currently sits in the loading skeleton forever in Storybook. Also, going Empty → ErrorRetryable shows the empty state. There might be collide with each other. Also left a optional comment, otherwise, this PR should be good to go. 🙂
Screen.Recording.2026-08-03.at.5.17.55.PM.mov
|
Dug into the Storybook regression @chihsuan recorded — the two symptoms have different root causes, worth separating because they need different fixes: 1. Default story stuck on the skeleton — fixture/guard mismatch, not state pollution. The Suggested fix — make the mock echo the requested ID: if ( subPath.startsWith( '/post/' ) ) {
const id = Number( subPath.match( /^\/post\/(\d+)/ )?.[ 1 ] );
return {
...mockStatsPostData,
post: { ...mockStatsPostData.post, ID: id || mockStatsPostData.post.ID },
};
}That also keeps the fixture honest for any future post-scoped widget that checks identity. 2. One more note on the red coverage check: the bulk of it is |
bf69c4c to
a473e60
Compare
|
Thanks @chihsuan & @dognose24 I addressed the storybook issue, this is good for re-review. |
dognose24
left a comment
dognose24
left a comment
There was a problem hiding this comment.
Re-reviewed the latest push — the two Storybook problems are both fixed properly, and the earlier feedback landed well:
- The
stats/postmock now echoes the requested ID, and the hook's identity guard has the right escape hatch (trusting a response with no identifier instead of skeletoning forever). Thenever attributes the previous winner's metrics to a new onetest locks this in. - The per-story presets with cleanup are carefully done — nice catch making
Emptyuselast-yearso it can't share a query key withlast-12-months. - Unknown metrics render as a dash with visually hidden "Not available" text rather than a fake zero, and the
comment_countsanitize is deduped down intonormalizeStatsPostMeta(including the third copy). PostHighlightCardextraction is a net win, and the detail link goes throughPostTitleLink's existingsafeHttpUrlguard with object-basedsearch.
Two things I'd like to see before approving:
-
popular-post/package.jsonis missing a direct dependency.render.tsximportspickReportDateParamsfrom@jetpack-premium-analytics/routing, but the manifest doesn't declare it — it currently builds only because hoisting happens to provide it.latest-post/package.jsondeclares the same dependency, so this is a gap specific to the new widget:"@jetpack-premium-analytics/routing": "link:../../packages/routing"
-
A few docs still describe the previous behavior. The implementation now uses the date range only to pick the winner, with Views/Likes/Comments all being all-time totals from
stats/post— but the Storybook component description ("the views it collected in that period", "Changing the date range changes … its view count"), thewidget.tsdocblock ("changes which post wins and the view count shown for it"), and the PR description's data-source table still say Views is period-scoped. Worth syncing so the docs don't outlive the behavior they describe.
Two optional notes, fine as follow-ups:
use-latest-posthas the sameplaceholderDatacross-post exposure the popular-post identity guard fixes (itsuseStatsPostgoes through the samestatsProxyQuery), just with a much rarer trigger — the latest post changing while the dashboard is mounted. Could share the guard later.- After the
PostHighlightCardextraction,latest-post/render.tsxno longer usesdate-fnsor@wordpress/ui, butlatest-post/package.jsonstill declares them — the mirror image of item 1, worth a quick cleanup.
On CI: the only red check is code coverage, and the uncovered lines are the two thin render entry points already exercised by the Storybook tests — adding the Covered by non-unit tests label seems reasonable here. Also [Status] In Progress can probably come off now that this is in review.
chihsuan
left a comment
chihsuan
left a comment
There was a problem hiding this comment.
Thanks for the follow-up !@louwie17 This looks good to me. 👍
Happy for this to merge once @dognose24's two items land.
…ight card
Adds the Insights "Most popular post" module as `jpa/popular-post`: the
site's most-viewed post for the dashboard's selected date range, with its
publish date and the same three metric tiles as Latest post. Unlike Latest
post it is period-scoped, reading `reportParams` from the widget root
context, so changing the date range changes both the winning post and its
view count. Views come from `stats/top-posts` for the period; likes and
comments come from `stats/post/{id}`, which takes no date range, so those
two tiles carry an all-time aggregation note instead of implying a period
number.
Latest post's presentational card moves to `PostHighlightCard` in
widgets-toolkit so both widgets render one card shape, and the card now
adapts to the dashboard cell with a single container-query breakpoint:
width-2 and wider cells get the two-column layout from the design (text
left, metric row anchored to the bottom, rounded near-square featured
image right), while width-1 cells drop the image and wrap the metric row.
The publish line copy now matches the prototype ("Post published on
<date>").
WOOA7S-1787
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HAX3YkQxDLLXhKXjft6qU2
The card's size ladder branched on width only, so a cell that was wide enough for the two-column layout but short could not fit its content. The metric row was pinned to the end of an over-tall column and `overflow: hidden` on the root silently clipped it: at a 736x86 body the row's values sat 38px below the clip boundary, leaving the labels cut mid-line, and at 331x86 the labels rendered cleanly with no values under them at all. Rebuilt against the design prototype, which resolves `wide = inline-size >= 520px` and `tall = block-size >= 300px` and ships `--wide-tall` / `--wide-short` / `--narrow-short` treatments. The card now declares its own named size container -- the shared `widget` container is `container-type: inline-size` and cannot answer block-axis queries -- and queries both axes, following the precedent in `widget-state.module.scss`. The clipping is fixed structurally rather than by moving a breakpoint: `.metrics` is `flex-shrink: 0` and `.header` is the flexible, clipping box, so the headline gives up space while the metric row keeps its full size. A label can no longer be shown without its value at any geometry. Below the height where the publish line also fits, it is dropped outright instead of being clipped to a sliver. Both widgets gain ShortCell and ShortNarrowCell dashboard stories so a height regression is visible in review. WOOA7S-1787 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HAX3YkQxDLLXhKXjft6qU2
Read every displayed metric from `stats/post`, so the three tiles share one all-time window instead of pairing range-scoped views with lifetime likes and comments. The date range's only job is now picking the winner, which also matches the sibling Latest post widget that shares this card. Consume metrics only from a response that identifies the current post. The Stats query carries the previous key's payload over through `placeholderData` while the content query deliberately does not, so a winner change could render the new post's title beside the previous post's engagement. Drop the comparison fields from the top-posts request: they triggered a second report fetch for a window this widget never renders. Document the `max` trade-off that can hide a qualifying post on a page-heavy site. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HAX3YkQxDLLXhKXjft6qU2
Show an unread metric as a dash rather than as zero. The Stats post endpoint can fail on its own — it 403s on a private site — and zeroing there rendered a post with 12 likes as "Likes 0", which is a wrong number rather than a missing one. Applies to Latest post too, which shares the card and had the same gap. Stop hiding a report failure behind stale rows. `placeholderData` only applies while a query is pending, so rows that survive an error are the last successful data — a failed background refetch. Gating on `! topRow` / `! latestPost` hid exactly that: stale numbers, no error, no Retry. Correct the story docblocks that still described a 432px breakpoint; the card resolves wide at 520px and tall at 300px. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HAX3YkQxDLLXhKXjft6qU2
Compress the comments added over the review rounds: keep the non-obvious rationale, drop the restated-code prose and the multi-paragraph explanations. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HAX3YkQxDLLXhKXjft6qU2
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HAX3YkQxDLLXhKXjft6qU2
The stats/post mock returned a fixture with a hardcoded post ID for every requested post, so the card could never attribute the metrics to the current winner and sat in its skeleton forever. The mock now echoes the ID it was asked for, which is what the real endpoint does. Move the Empty story off `last-365-days`: it resolves to the same dates as ErrorRetryable's `last-12-months` most years, so the two shared a query key and Empty's cached result won. Normalize `comment_count` in `sanitizeStatsPostResponse` instead of in each consumer, removing three copies of the same coercion. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HAX3YkQxDLLXhKXjft6qU2
Matches Latest post, which trunk moved onto the internal detail route. Both widgets now carry the dashboard's window into the detail page, and the card still falls back to the published post when there is no post ID. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HAX3YkQxDLLXhKXjft6qU2
The card's title now renders a router-backed Link when it has a post ID, so the close-up stories need the same `withStoryRouter` decorator the sibling post widgets already use. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HAX3YkQxDLLXhKXjft6qU2
Popular post imports `pickReportDateParams` and `useMemo` but declared neither, building only because hoisting happened to provide them. Latest post gained the same `useMemo` import, and no longer uses `date-fns` or `externals` since the shared card took over its rendering. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HAX3YkQxDLLXhKXjft6qU2
The widget reads every metric from `stats/post`, so the date range picks the winner rather than scoping the view count. The widget definition and the Storybook descriptions still described the earlier period-scoped views. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HAX3YkQxDLLXhKXjft6qU2
…kens Trunk's `@wordpress/*` update renamed the font-weight tokens (medium -> emphasis, regular -> default). The shared card still used the old names, which resolve to nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HAX3YkQxDLLXhKXjft6qU2
0241089 to
eb3ff66
Compare
louwie17
added
the
Covered by non-unit tests
github-actions
Bot
added
[Status] UI Changes
3 participants
Fixes #
Proposed changes
Part of the Insights tab work in WOOA7S-1787: "Create a new type of widget to highlight one post for Latest post and Popular post, including posting timestamp and basic stats."
jpa/popular-postwidget — the Insights "Most popular post" module. Shows the site's most-viewed post for the dashboard's selected date range, with its publish date and the same three metric tiles as Latest post. Unlike Latest post it is period-scoped: it readsreportParamsfromuseWidgetRootContext(), so changing the date range changes both the winning post and its view count.PostHighlightCardinwidgets-toolkit, so both widgets render one card shape instead of duplicating it. Each widget'srender.tsxstays thin: fetch through its own hook, hand the data to the card, wrap the states in<WidgetState>.postContentQuery( postId )inpackages/data(a post's headline content and featured image by ID, reusing the existing latest-post sanitizer), and a Storybook mock for the core/wp/v2/posts?include=<id>request.Data sources, and which numbers are period-scoped
Popular post composes three requests:
stats/top-postswith the dashboard date range/wp/v2/posts?include=<id>stats/post/{id}The Stats post endpoint takes no date range, so likes and comments cannot be period-scoped. Rather than showing them silently next to a period number, those two tiles carry an aggregation note (a
titletooltip plus visually hidden text for assistive technology), and the widget'sdescription/helpsay so explicitly. If we want genuinely period-scoped engagement here, that needs a different endpoint and should be a follow-up.Only a
stats/top-postsfailure surfaces as an error — the ranking is the widget. A failing content or metrics request degrades to no image and zeroed engagement counts rather than blanking the card, the same way Latest post already behaves.Sizing
The card adapts on both axes. An earlier revision of this PR branched on width only, which clipped the metric row on short cells — see the fix commit — so the ladder was rebuilt against the design prototype rather than invented. The prototype resolves
wide = inline-size >= 520pxandtall = block-size >= 300pxand ships--wide-tall/--wide-short/--narrow-shorttreatments; those thresholds and treatments are what is implemented here.Two further steps keep the smallest cells honest: below 130px the headline clamps to one line, and below 100px — where a one-line headline, the publish line and the metric row can no longer all fit — the publish line is dropped outright rather than clipped to a sliver.
Because the shared
widgetcontainer iscontainer-type: inline-sizeand cannot answer block-axis queries, the card declares its own named size container, following the precedent already set bywidget-state.module.scss.The metric row can never be partially rendered.
.metricsisflex-shrink: 0and the headline block is the flexible, clipping box, so under vertical pressure the headline gives up space and the metric row keeps its full size. A label is never left visible with its value pushed outside the card.@ederrengifo — two things worth a look, since the prototype's grid only ever renders these cards at one height (416px) and so does not demonstrate the short modes end to end: (1) in
wide + short, the 116px square sits block-centred while the metric row is bottom-aligned, which leaves it floating slightly high — is that the intent? (2) dropping the publish line below 100px is my call, not the design's.Deliberately out of scope
jpa/popular-postis not added to the default Insights layout insrc/dashboard-layout.phpyet (that file has a standing TODO naming "Most popular post" as unported). Other worktrees are touching the Insights layout for this same issue, so registering it there is left as a small follow-up to avoid conflicting edits. The widget is discoverable and addable from the dashboard in the meantime.Related product discussion/links
Does this pull request change what data or activity we track or use?
No. Popular post reads existing Jetpack Stats endpoints (
stats/top-posts,stats/post/{id}) through the existing data proxy, plus the local core posts endpoint. No new endpoints, no new tracking.Testing instructions
Storybook is the quickest path:
pnpm run storybookfromprojects/packages/premium-analytics(orcd projects/js-packages/storybook && pnpm run storybook:dev).Default— the close-up canvas is width-1 sized, so it shows the narrow tier: no featured image, wrapped metric row.WidgetDashboardWithWidget— the real dashboard at width 2. This is the default placement: two columns, image right. DropwidgetWidthto 1 and back to walk the breakpoint. Comparison report params are passed here, so the widget is covered against inventing deltas.Loading/Error/ErrorRetryable/Empty—Errorhas no Retry (permission gate),ErrorRetryabledoes.On a real site, add both widgets to the Insights tab:
Automated checks I ran (from
projects/packages/premium-analytics):pnpm run typecheck— clean.npx jest --config=tests/jest.config.cjs— 209 suites / 1458 tests pass.packages/ui/src/date-range-filterhas one arrow-key focus test that fails only under parallel load; it passes in isolation both with and without this branch, so it is pre-existing flake, not this change.eslinton every touched path, andstylelinton the new stylesheet — clean.