Repository navigation
Conversation
The widget exposed two of the three properties stats/devices/{property}
serves — browser and platform — so the screen-size breakdown was unreachable
from it.
Size needs its own formatting: WPCOM special-cases screensize and returns
percentage shares, while browser and platform return view counts. The
leaderboard prints the two units accordingly; bar widths stay relative to the
largest visible value either way.
Also drop client_type from StatsDeviceProperty. It is not one of the three
properties the endpoint accepts, so a request naming it is rejected with a 400
before any query runs. Nothing called it.
widget.json is the registered widget's catalog and help copy, and it still described only browsers and operating systems.
SelectField renders the first element when the attribute is unset, so listing Size first made an unconfigured widget label itself "Size" while still fetching browser data. Size moves to the end, matching the render default, and a test pins the first element to the default so the two cannot drift.
kangzj
added
Enhancement
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
This comment has been minimized.
This comment has been minimized.
|
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! Premium Analytics plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
This comment has been minimized.
This comment has been minimized.
Code Coverage SummaryCoverage changed in 5 files.
1 file is newly checked for coverage.
|
There was a problem hiding this comment.
Pull request overview
Adds Size (screensize) as a third “View by” dimension for the Premium Analytics Top platforms widget, including correct formatting for the endpoint’s mixed units (counts vs percentage shares), and tightens the allowed device-property set in the data layer.
Changes:
- Add Size as a selectable dimension and render it as percent shares while keeping Browser/OS as counts.
- Guard persisted
platformDimensionvalues to prevent unsupportedstats/devices/{property}requests. - Remove unsupported
client_typefromStatsDeviceProperty, and update Storybook mocks + add widget unit tests.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/packages/premium-analytics/widgets/top-platforms/widget.ts | Adds screensize to widget attributes and to the “View by” control options. |
| projects/packages/premium-analytics/widgets/top-platforms/widget.json | Updates widget description/help text to reflect Size support and mixed units. |
| projects/packages/premium-analytics/widgets/top-platforms/use-platform-views.ts | Adds screensize label mapping and supports screensize as a device property. |
| projects/packages/premium-analytics/widgets/top-platforms/stories/top-platforms-widget.stories.tsx | Expands Storybook controls and adds a BySize story. |
| projects/packages/premium-analytics/widgets/top-platforms/render.tsx | Formats screensize rows as percentages, keeps bars relative, and guards persisted dimensions. |
| projects/packages/premium-analytics/widgets/top-platforms/tests/top-platforms.test.tsx | Adds tests for requested device property, default behavior, invalid persisted values, and formatting. |
| projects/packages/premium-analytics/packages/widgets-toolkit/src/stories/mocks/register-stats-mocks.ts | Removes the invalid client_type devices mock branch; keeps platform-only routing. |
| projects/packages/premium-analytics/packages/data/src/queries/stats-devices-query.ts | Narrows StatsDeviceProperty to the endpoint-supported set. |
| projects/packages/premium-analytics/changelog/add-wooa7s-1940-top-platforms-size | Adds a package changelog entry for the new Size breakdown. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| Significance: minor | ||
| Type: added | ||
|
|
||
| Top platforms: offer Size alongside Browser and OS in the widget's "View by" control. |
| import type { ReportParams, StatsDevicesComparisonItem } from '@jetpack-premium-analytics/data'; | ||
|
|
||
| export type PlatformDimension = 'screensize' | 'browser' | 'platform'; | ||
|
|
- PlatformDimension aliases StatsDeviceProperty instead of restating it, so the label map stays exhaustive when the endpoint's property set changes. - Hoist the screen-size labels into widgets-toolkit; Devices and Top platforms were translating the same six strings separately. - Derive the supported-dimension guard from that label map rather than keeping a second copy of the list. - Cover the guard directly, drop the it.each row that duplicated the defaults test, and add a Size-with-comparison story for the one Size state nothing exercised. - Correct a docblock that still described two dimensions. - Add the plugins/premium-analytics changelog entry the package-only entry should have been paired with.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
- The attribute type was the one place still restating the endpoint's property set; it aliases StatsDeviceProperty like the hook does, so the guard and the attribute type cannot diverge. - Cover the per-dimension label mapping, which the render tests could not reach because they stub the hook wholesale. - formatDisplayLabel looked up plain-object keys unguarded, so a key named toString returned an inherited function where a string was expected. No API vocabulary triggers it, but this change points a second widget at that lookup.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
A failing assertion would have skipped the restore on the line below it and leaked the override into whatever test was appended next. The override is local to the mock now, and the fixture map is readonly so it cannot drift back.
This comment has been minimized.
This comment has been minimized.
Review-cycle summary —
|
| Round | Source | Finding | Resolution |
|---|---|---|---|
| 1 | claude | PlatformDimension was a third copy of StatsDeviceProperty |
Aliased the shared type, so the exhaustive label map breaks the build on an upstream change — d58012b7cf |
| 1 | claude | Screen-size labels duplicated the Devices widget's map | Hoisted to widgets-toolkit/helpers/device-labels.ts; both widgets import it — d58012b7cf |
| 1 | claude | SUPPORTED_DIMENSIONS was a fourth copy of the list |
Replaced with isPlatformDimension() keyed off the label map — d58012b7cf |
| 1 | claude | Test nits; hook had no coverage | Dropped the duplicated it.each row; the render mock now spreads requireActual so the guard test exercises shipped code — d58012b7cf |
| 1 | claude | Stale docblock | Updated — d58012b7cf |
| 1 | claude | Missing plugin changelog entry | Added plugins/premium-analytics — d58012b7cf |
| 1 | copilot | sharePercentage receives the raw value; comment placement |
Documented why the raw value is deliberate and moved the conversion comment — d58012b7cf |
| 2 | claude | widget.ts was the last hand-written copy of the union |
Aliased StatsDeviceProperty — 63779e2402 |
| 2 | claude | toPlatformView uncovered |
Hook-level tests through the real query path, one key per dimension plus the title-case fallback — 63779e2402 |
| 2 | claude | Drive-by: formatDisplayLabel prototype hole |
Own-property lookup plus tests. Taken rather than deferred: this PR points a second widget at that helper — 63779e2402 |
| 3 | claude | Fallback test mutated a shared fixture without a finally |
Override is local to the mock; fixture map is Readonly — 3f17a5d1f6 |
Declined:
plugins/jetpackchangelog entry. The dashboard is behindJetpack::is_premium_analytics_enabled()(class.jetpack.php:871-889), which defaults to false, so it isn't reachable by Jetpack users yet.wpcomshandmu-wpcom-pluginare sticker-gated similarly. Per the rule's own "not to every dependent by reflex".- Folding
build-sessions-by-device-data.ts's Desktop/Mobile/Tablet map intoSCREEN_SIZE_LABELS. It mapsdevice_typefrom the report layer rather thanstats/devices/screensize, so merging them would assert an unverified equivalence between two API vocabularies. Flagged for a follow-up instead.
Design question surfaced, not silently decided: calculateDelta on Size compares two shares, so 57.8% vs 52.3% renders as +10.5% while the share moved +5.5 points. Reachable with comparison on. A BySizeWithComparison story now puts that state in front of a reviewer.
Verification: 18 top-platforms tests across 3 suites, 5 for formatDisplayLabel, full package suite at 2041 passing (the single failure, packages/routing/.../build-range-patch.test.ts, is pre-existing on trunk), typecheck and ESLint clean. All three dimensions re-exercised in a live dashboard after each rebuild — the final pass against the site's real Stats data with no local mock.
kangzj
added
[Status] Needs Team Review
dognose24
left a comment
dognose24
left a comment
There was a problem hiding this comment.
LGTM! The unit-conversion chain is careful (display values converted, bar widths and calculateDelta deliberately on raw values — scale-invariant either way), the stale-attribute guard matches the SelectField first-element convention with a test that reads the definition directly, and the client_type removal is clean — I grepped both premium-analytics projects and found no remaining references.
One non-blocking product question to settle before or after merge: in Size + comparison mode, a share moving from 52.3% to 57.8% currently shows the relative change (+10.5%) rather than +5.5 pp. Two percentages with different meanings sit side by side on the same row — the BySizeWithComparison story you added shows it well. If design is happy with relative change, the implementation is already correct; just flagging so it's a decision rather than an accident.
Also left one tiny i18n suggestion inline.
| value: 'platform', | ||
| }, | ||
| { | ||
| label: __( 'Size', 'jetpack-premium-analytics-pkg' ), |
There was a problem hiding this comment.
nit: Size is an isolated string for translators and easily lands as "file size" / "dimensions" in some locales. A translator context would pin it down:
| label: __( 'Size', 'jetpack-premium-analytics-pkg' ), | |
| label: _x( 'Size', 'Screen size breakdown: Desktop / Mobile / Tablet', 'jetpack-premium-analytics-pkg' ), |
(with _x added to the @wordpress/i18n import). Browser/OS are less ambiguous, fine as they are.
|
Thanks for digging into the server side! One thing I want to double-check before this lands: the screen-size split already has its own widget on the same Traffic page — Devices (row 2) renders 
Is this addition based on an updated design or product direction I might have missed? If design does want Size inside Top platforms too, great — but if not, we'd be duplicating the Devices card two rows below it and diverging from the mock. Your first comment on WOOA7S-1940 read it the same way (a product decision worth its own issue), so I figured it's worth confirming which direction is current before merging. |
|
Thanks @dognose24! I didn't realized it's split into two widgets. Closing... |
github-actions
Bot
removed
[Status] In Progress
[Status] Needs Team Review
Follows WOOA7S-1940 (the second half of that report)
Why
WOOA7S-1940 said the Top platforms widget was "missing platform distribution". It was: the widget offered Browser and OS, which is two of the three breakdowns
stats/devices/{property}actually serves. The screen-size split — Desktop / Mobile / Tablet — was unreachable from it. This adds it as Size.Proposed changes
screensizeand returns percentage shares, whilebrowserandplatformreturn view counts, so the leaderboard prints the two units accordingly. Bar widths stay relative to the largest visible value either way.platformDimensionagainst a value outside the supported set, so a stale layout can't turn into astats/devices/<unknown>request.client_typefromStatsDeviceProperty. It is not one of the properties the endpoint accepts, so a request naming it is rejected before any query runs. Nothing called it; the Storybook mock that pretended it worked is gone too.Browser stays the first option deliberately: the shared
SelectFielddisplays the first element when the attribute is unset, so the first option has to match the render default. If the design wants Size first, the render default and theexampleattributes should move with it — say the word and I'll flip both together.Screenshots
Size selected — Desktop / Mobile / Tablet as shares of all views:
Related product discussion/links
Does this pull request change what data or activity we track or use?
No. Size reads
stats/devices/screensize, the same endpoint and property the existing Devices widget already requests.Testing instructions
57.8%), not view counts.Storybook covers the same states without a WordPress install: Packages/Premium Analytics/Widgets/TopPlatforms now has a BySize story alongside Default and ByOS.
Note on the screenshot
The dashboard site used for the screenshot is in offline mode, so its
stats/devices/*responses were mocked locally with the shapes WordPress.com actually returns (percentage shares forscreensize, counts for the other two). The endpoint's property set and response shapes were confirmed against the WordPress.com server code rather than assumed.