Premium Analytics: align the video detail composition with the design mocks - #50970
Conversation
|
@claude please review this PR. |
|
Claude finished @dognose24's task in 3m 41s —— View job Code reviewI read through the data layer, the new Files reviewed
✅ Verified: the "single shared request" claim holdsThe highlights widget passes the raw ✅ Security / correctness spot-checks
🟡 Minor — worth addressing
🟢 Non-blocking / questions
Tests & docsTest coverage looks solid — new/updated suites for the processing sanitizer, the query builder (incl. the I couldn't independently run Overall: clean, well-scoped, and matches the stated design intent. Only #1 (DRY on the scope helper) and #2 (logical property) are worth a quick touch-up; the rest are optional. |
|
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! |
github-actions
Bot
added
the
[Status] Needs Author Reply
Code Coverage SummaryCoverage changed in 7 files. Only the first 5 are listed here.
3 files are newly checked for coverage.
Full summary · PHP report · JS report Coverage check overridden by
Covered by non-unit tests
|
dognose24
added
[Tests] Includes Tests
[Status] Needs Team Review
dognose24
removed
[Status] Needs Author Reply
dognose24
added
[Status] Needs Review
|
Addressing claude[bot]'s review: both items fixed in ecc8881 — highlights now uses the shared toPostId (0 sentinel, hasVideoScope adjusted) and the chart widget uses block-size per the logical-properties convention. Note for a possible follow-up: widgets/post-views/style.module.css on trunk has the same height: 100% pattern. |
| // A long unbroken video title in the breadcrumb refuses to shrink: admin-ui's | ||
| // Page header renders the breadcrumbs slot inside a flex chain (inner header | ||
| // Stack → the Breadcrumbs `nav`) whose items keep the flexbox default | ||
| // `min-inline-size: auto`, so the crumb's nowrap min-content propagates and | ||
| // drags the whole page into horizontal scrolling before the crumb's own | ||
| // ellipsis can engage. Let both links shrink until admin-ui fixes the slot; | ||
| // the `nav`'s parent has no stable class, hence the structural `:has()`. | ||
| .page :has(> nav[aria-label]) { | ||
| min-inline-size: 0; | ||
| } | ||
|
|
||
| .page nav[aria-label] { | ||
| min-inline-size: 0; | ||
| } |
There was a problem hiding this comment.
Is this issue documented somewhere in Gutenberg?
I'm thinking these likely cover it WordPress/gutenberg#77628 and WordPress/gutenberg#77039 but asking in case.
There was a problem hiding this comment.
Good question — I checked both: neither documents this exact min-content propagation as its own bug. #77039 (Breadcrumb component extraction) and #77628 (the admin-ui 2.0 proposal) are the tracks under which the slot gets reworked, so I've linked both from the workaround comment in 73b093a and noted no dedicated issue exists. Happy to file one upstream if you think it's worth pinning.
There was a problem hiding this comment.
Linked the tracking issues from the workaround comment in 73b093a: WordPress/gutenberg#77039 (Breadcrumbs slot extraction) and WordPress/gutenberg#77628 (admin-ui 2.0 proposal).
| display: flex; | ||
| align-items: center; | ||
| flex-wrap: wrap; | ||
| gap: var(--wpds-dimension-gap-lg); | ||
| padding-block: 40px; | ||
| padding-inline: var(--wpds-dimension-padding-2xl); |
There was a problem hiding this comment.
This is starting to look like you just need Stack component there ;-)
There was a problem hiding this comment.
|
Housekeeping: the one-line jest coverage-glob fix moved out to #50997 — touching |
dognose24
changed the title
chihsuan
left a comment
chihsuan
left a comment
There was a problem hiding this comment.
Thanks for the update! Left a few more comments.
| // comparison, so drop them before they reach the URL and the widgets. | ||
| for ( const param of COMPARISON_SEARCH_PARAMS ) { | ||
| delete seeded[ param ]; | ||
| } |
There was a problem hiding this comment.
Could we keep the comparison params in the URL and let these widgets continue to ignore them instead?
pickReportDateParams() intentionally carries comparison state into the video detail route, while useDashboardLink() and the “Back to Videos” link read that state back when navigating away. Removing it here means Dashboard → Video → Dashboard silently loses the user’s comparison settings.
There was a problem hiding this comment.
Good call — done in 65e3e3e. The route now leaves comparison params untouched: the page's widgets already ignore them, and preserving them means the dashboard link and "Back to Videos" carry the user's comparison settings back out instead of silently dropping them on a Dashboard → Video → Dashboard round trip. Replaced the strip test with two pass-through tests (settled URL and seeded URL).
There was a problem hiding this comment.
Update after more live testing: going with your suggestion outright. The route-level stripping turned out to fight the round-trip state (and my serialization fix effectively disabled it anyway), so the contract is now: comparison params pass through the video URL untouched and every widget on the page ignores them (test-locked); the page still renders no comparison UI per the mock. The buildDashboardLink serialization fix stays — it was independently broken. Thanks for pushing on this one.
| icon: video, | ||
| value: sumVideoMetric( impressions.data ), | ||
| icon: seen, | ||
| value: total?.impressions ?? 0, |
There was a problem hiding this comment.
Could we preserve missing metrics instead of coercing them to zero?
The available statType=all columns are described by fields, so a missing impressions, watch_time, or retention_rate value does not necessarily mean a measured zero. With ?? 0, a partial response displays 0, 0.0, or 0.0% as real data.
There was a problem hiding this comment.
Fixed in 65e3e3e. The tiles now pass null for metrics missing from the response instead of coercing to 0 — MetricTileGrid already renders its — placeholder for any non-finite value, so a partial response shows placeholders rather than fake zeros. Added a test covering a fields: [period, plays]-only response.
There was a problem hiding this comment.
Done in 65e3e3e — metrics missing from the response pass null through, so MetricTileGrid renders its placeholder instead of a fabricated 0 / 0.0 / 0.0%.
| if ( fields.length >= 2 && Array.isArray( payload.data ) ) { | ||
| const metricNames = fields.slice( 1 ); | ||
| metrics = metricNames; | ||
| rows = tuples.map( ( [ period, ...cells ] ) => ( { |
There was a problem hiding this comment.
Would it make sense to omit rows for now and add the per-metric series shape when its first consumer lands? It appears that rows and StatsSingleVideoMetricRow currently have no consumers and add normalization, exports, and tests across several layers.
There was a problem hiding this comment.
Agreed — removed in 65e3e3e. rows and StatsSingleVideoMetricRow are gone from the normalizer and the export surface; metrics stays since it still marks range mode. The per-metric series shape can come back with its first consumer.
There was a problem hiding this comment.
Done in 65e3e3e — rows / StatsSingleVideoMetricRow are gone from the normalizer; the widgets read data/total/post directly.
…etrics, drop unused rows - The video-detail route no longer strips comparison params: the page ignores them, but the dashboard link and Back to Videos carry the URL state back out, so stripping lost the user's comparison settings on a round trip. - Highlights tiles render the MetricTileGrid placeholder for metrics missing from the response instead of coercing them to 0/0.0/0.0%. - Remove the unconsumed rows/StatsSingleVideoMetricRow from the single-video normalizer; widgets read data/total/post directly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Mirror the video-detail review outcome (#50970): the page renders no comparison and its widgets ignore the params, but the breadcrumb's dashboard link reads the URL state back out, so stripping them lost the user's comparison settings on a Dashboard → Post → Dashboard round trip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The breadcrumb's dashboard link serialized search values with bare URLSearchParams, but the router JSON-parses every search value on read — so comp: '1' came back as the number 1 and every strict comp === '1' check treated comparison as disabled. From that dashboard, detail links then carried no comparison at all. - buildDashboardLink now JSON-quotes string values that would re-parse as a different type, matching the router's own stringifier. - hasComparisonEnabled, deriveComparisonRange, and the range patch accept a numeric comp flag from URLs already written unquoted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ment panel Conflicts were the #50964 externals refactor against the video-detail stage/summary-card imports (resolved to the externals convention) and the date-filters panel, taken wholly from trunk — this branch's net change to packages/ui is zero since 'Drop the header date-filter half', and trunk's WOOA7S-1817 content-measurement rework is what that deferral was waiting for. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
chihsuan
left a comment
chihsuan
left a comment
There was a problem hiding this comment.
Thanks @dognose24 Overall this looks good
I left two notes worth addressing, then feel free to merge. See inline comments. Thanks!
| safeParseFloat( value ), | ||
| ] ) | ||
| ) | ||
| : null; |
There was a problem hiding this comment.
Could we drop non-numeric cells here instead of coercing them?
A video with no retention data renders Retention rate 0.0% — a fabricated statistic that reads as a real finding, which on an analytics surface costs more than a visible blank. Same fake-zero we removed from the tiles last round: safeParseFloat() falls back to 0, so ?? null never sees it. normalizeStatsSummary() already guards this way (processing/stats/utils.ts:276).
There was a problem hiding this comment.
Fixed in cefbbc0 — non-numeric cells are now dropped with the same guard normalizeStatsSummary uses (isStatsNumericSummaryValue, now exported from processing/stats/utils.ts), so a missing metric stays undefined and the tile renders its placeholder instead of a fabricated 0.0%. Added a normalizer test pinning the dropped-cell behavior.
| onClick: () => { | ||
| void Promise.all( queries.map( query => query.refetch() ) ); | ||
| }, | ||
| onClick: () => void refetch(), |
There was a problem hiding this comment.
Should this error state go through describeError() too, now that its sibling does?
Without Stats access the page contradicts itself — both cards share one cache entry, so Views performance says You don't have access to this data. while this one offers a Retry that can never succeed, so the user keeps pressing it.
There was a problem hiding this comment.
Fixed in cefbbc0 — the highlights card now routes its error through describeError() with the same shape as the Views performance card, so a plain 403 shows the shared no-access copy with no Retry, and only the healable no_connection 403 keeps the Retry action. Tests updated: the retry test now uses the no_connection envelope, and a new case pins the permission 403 rendering without a Retry button.
…escribeError Per review: a non-numeric totals cell coerced to 0 renders a fabricated statistic (e.g. "Retention rate 0.0%"), so drop it with the same guard normalizeStatsSummary uses and let the tile show its placeholder. The highlights card's error state now goes through describeError like its sibling, so a plain 403 shows the shared no-access copy instead of a Retry that can never succeed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ideo-detail-composition # Conflicts: # projects/packages/premium-analytics/routes/video-detail/stage.tsx
github-actions
Bot
added
[Status] UI Changes
…tents trail (#51085) * Premium Analytics: fix the breadcrumb overflow behind the display:contents trail The StatsBreadcrumbs wrapper is display:contents, so the route-level ':has(> nav)' workaround from #50970 lands min-inline-size on a box-less element and the crumb's nowrap min-content propagates again, widening the whole page into horizontal scrolling (regressed when #51022 swapped the raw Breadcrumbs for StatsBreadcrumbs). Own the constraint in the component: the wrapper's DOM parent and the nav — the actual flex item in the box tree — both get min-inline-size: 0, fixing every page that renders StatsBreadcrumbs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Drop the superseded route-level breadcrumb workaround The ':has(> nav)' rule targets the display:contents trail wrapper (no box, so min-inline-size has nothing to act on) and the nav rule is covered by the component-owned fix — StatsBreadcrumbs' stylesheet is now the single source of the constraint. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Cite the upstream issue for the breadcrumbs slot propagation Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: retrigger checks — workflow runs were never created for the previous push --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…resets, no comparison (#50971) * Premium Analytics: post detail parity — overflow fixes, timezone, no comparison Applies to the post/email detail page the fixes proven on the video detail page: the breadcrumb/summary overflow bugs (contain: inline-size plus the breadcrumbs-slot shrink shim), aspect-ratio: 1 so the boot shell's img reset cannot squash the featured image, post-views bucket keys parsed as site-local calendar dates (parseSiteDateTime, UTC-12 regression test), and block-size nits. The page's design has no period-over-period comparison: the post-views and email-time-series widgets drop their comparison series, the route normalizes comparison params away, and DateFiltersPanel gains a minimal showComparison prop so the Compare control hides at the existing fixed-bar call site. Moving the panel onto the summary's title row (per the mock) is deferred until the preset measurement rework lands (WOOA7S-1816) — it would collide head-on, the same reason the video-detail PR dropped its filters row. Rebased onto trunk accordingly: nothing here depends on the video-detail branch anymore. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Pass comparison params through the post-detail route Mirror the video-detail review outcome (#50970): the page renders no comparison and its widgets ignore the params, but the breadcrumb's dashboard link reads the URL state back out, so stripping them lost the user's comparison settings on a Dashboard → Post → Dashboard round trip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Premium Analytics: align comparison pass-through comments with the route contract * Strip comparison params from injected widget reportParams and drop the parseISO fallback Per review: the page-wide no-comparison invariant now holds by construction — usePostDetailTabs injects comparison-stripped reportParams (via the new omitComparisonReportParams helper) into every layout entry, so comparison-capable widgets like UTM insights and highlights can no longer render deltas from URL state. The URL keeps the comparison params for the breadcrumb round trip. Also remove the parseISO fallback in use-post-views: bucket dates come from format(start, 'yyyy-MM-dd') so parseSiteDateTime cannot fail, and the fallback would silently reintroduce the browser-local day shift; an unparseable bucket now drops the point instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Record the crypto-js deprecation in pnpm-lock.yaml The npm registry marked crypto-js@4.2.0 deprecated after this lock was written, so the lock check's fresh resolution now expects the deprecation note. Registry-metadata drift only; no dependency change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Put the date filter presets on the summary title row per the mocks One header row per the design mocks: title left, presets right, vertically centered. The summary grows into free space and absorbs the squeeze down to a 400px floor (title ellipsis); its inline-size containment keeps a long title from wrapping the row. Known rough edge, deferred to #51088: the panel self-measures its root, which in this shrink-to-fit slot always sees its own content width — so the presets keep their full layout and narrow rows degrade poorly. The external-measurement wiring (containerElement / reservedInlineSize) ships there to keep this PR free of shared-component changes beyond the already-reviewed showComparison prop. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Drop the route-level breadcrumb shim in favor of the component-owned fix Superseded by #51085: the ':has(> nav)' rule targets StatsBreadcrumbs' display:contents trail wrapper (no box for min-inline-size to act on) and the nav rule is covered by the component stylesheet. Tracked upstream as WordPress/gutenberg#81297. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: retrigger checks — workflow runs were never created for the previous push --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…mparison from its widgets (#51082) * Premium Analytics: post detail parity — overflow fixes, timezone, no comparison Applies to the post/email detail page the fixes proven on the video detail page: the breadcrumb/summary overflow bugs (contain: inline-size plus the breadcrumbs-slot shrink shim), aspect-ratio: 1 so the boot shell's img reset cannot squash the featured image, post-views bucket keys parsed as site-local calendar dates (parseSiteDateTime, UTC-12 regression test), and block-size nits. The page's design has no period-over-period comparison: the post-views and email-time-series widgets drop their comparison series, the route normalizes comparison params away, and DateFiltersPanel gains a minimal showComparison prop so the Compare control hides at the existing fixed-bar call site. Moving the panel onto the summary's title row (per the mock) is deferred until the preset measurement rework lands (WOOA7S-1816) — it would collide head-on, the same reason the video-detail PR dropped its filters row. Rebased onto trunk accordingly: nothing here depends on the video-detail branch anymore. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Pass comparison params through the post-detail route Mirror the video-detail review outcome (#50970): the page renders no comparison and its widgets ignore the params, but the breadcrumb's dashboard link reads the URL state back out, so stripping them lost the user's comparison settings on a Dashboard → Post → Dashboard round trip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Premium Analytics: align comparison pass-through comments with the route contract * Strip comparison params from injected widget reportParams and drop the parseISO fallback Per review: the page-wide no-comparison invariant now holds by construction — usePostDetailTabs injects comparison-stripped reportParams (via the new omitComparisonReportParams helper) into every layout entry, so comparison-capable widgets like UTM insights and highlights can no longer render deltas from URL state. The URL keeps the comparison params for the breadcrumb round trip. Also remove the parseISO fallback in use-post-views: bucket dates come from format(start, 'yyyy-MM-dd') so parseSiteDateTime cannot fail, and the fallback would silently reintroduce the browser-local day shift; an unparseable bucket now drops the point instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Premium Analytics: restore the video detail date filters and strip comparison from its widgets WOOA7S-1816: the date filter presets were dropped from the video detail page in #50970 because they collided with the preset-measurement rework; now that #50906 has landed, restore them using the same construction as post detail (DateFiltersPanel with showComparison=false, fixed above the scroll container). Also port the comparison-strip construction from #50971: inject omitComparisonReportParams()-stripped reportParams into every layout entry so comparison-capable widgets (highlights) cannot render deltas from URL state, while the URL keeps the params for the breadcrumb round trip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Record the crypto-js deprecation in pnpm-lock.yaml The npm registry marked crypto-js@4.2.0 deprecated after this lock was written, so the lock check's fresh resolution now expects the deprecation note. Registry-metadata drift only; no dependency change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Clarify that the video detail comparison strip is defensive All three current video-detail widgets already ignore comparison params in their own query mapping; the injection exists so the page-wide invariant holds by construction (matching post detail) rather than relying on each widget. Adjust the code comment and changelog wording accordingly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Put the date filter presets on the summary title row per the mocks One header row per the design mocks: title left, presets right, vertically centered. The summary grows into free space and absorbs the squeeze down to a 400px floor (title ellipsis); its inline-size containment keeps a long title from wrapping the row. Known rough edge, deferred to #51088: the panel self-measures its root, which in this shrink-to-fit slot always sees its own content width — so the presets keep their full layout and narrow rows degrade poorly. The external-measurement wiring (containerElement / reservedInlineSize) ships there to keep this PR free of shared-component changes beyond the already-reviewed showComparison prop. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Put the video detail date filter presets on the summary title row Same composition as post detail: one header row, title left and presets right, 400px summary floor with inline-size containment. The panel's step-down on narrow rows needs external measurement and ships separately (#51088). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Drop the route-level breadcrumb shim in favor of the component-owned fix Superseded by #51085: the ':has(> nav)' rule targets StatsBreadcrumbs' display:contents trail wrapper (no box for min-inline-size to act on) and the nav rule is covered by the component stylesheet. Tracked upstream as WordPress/gutenberg#81297. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Drop the video detail route-level breadcrumb shim in favor of #51085 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: retrigger checks — workflow runs were never created for the previous push * ci: retrigger checks — workflow runs were never created for the previous push --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
3 participants
Part of WOOA7S-1785
Why
The video detail page should match the design mocks so beta interviewees see the intended composition: a full-width highlights row (Impressions / Hours watched / Retention rate), a Views performance chart beside the "Used on posts & pages" list, and a 72×72 thumbnail header.
Proposed changes
stats/video/:idrequests tostatType=allwith the page's date range (start_date/date; range-scoped servertotalincluding play-weightedretention_rate, as confirmed against wpcom #229903 / Calypso #112969). One shared cache entry feeds the highlights tiles and chart.jpa/video-detail-views-performancewidget: add a single-series views line chart with day/week/month granularity. Bucket keys are parsed as site-local calendar dates (parseSiteDateTime) so labels match the legacy chart in any site timezone.post.posterpassthrough guarded bysafeHttpUrl, with anonErrorvideo-icon fallback), a single-line ellipsized title, and a line stating the applied performance range.Testing instructions
?page=jetpack-premium-analytics-wp-admin&p=%2Fvideo%2F<id>). Highlights show Impressions / Hours watched / Retention rate, the Views performance chart renders beside "Used on posts & pages", and Network shows one range-scopedstats/video/<id>request withstatType=allshared by the highlights and chart.comp=1to the URL and confirm the route strips them and renders only the current Views series.pnpm testandpnpm run typecheckfromprojects/packages/premium-analytics.Notes for reviewers
localTZDateday shift in post views).@wordpress/admin-uias a focused admin-page framework (2.0) WordPress/gutenberg#77628.Does this pull request change what data or activity we track or use?
No.