Premium Analytics: add Regions to the Top locations "View by" control - #51267
Premium Analytics: add Regions to the Top locations "View by" control#51267kangzj wants to merge 5 commits into
Conversation
The widget's dropdown offered only Countries and Cities, so a site owner could reach region data only by drilling into one country at a time — while the Locations report has had a Regions tab all along. Regions mode is worldwide, like Cities: the leaderboard lists regions and the map sums them back up to their country, since Google GeoChart cannot place sub-country rows on the world map.
|
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! |
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.
Code Coverage SummaryCoverage changed in 1 file.
|
…reset - REPORT_SECTIONS values are typed to the report's own tab ids so a slug typo fails the build; ReportLink takes a bare string and would not. - Fall back to the Countries tab when a persisted layout carries a granularity outside the union. - Test the round trip the reset actually guards: drill into a country, switch to Regions, and come back to Countries listing every country again. - Fix an it.each title whose placeholders read in the opposite order to its tuple.
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 report-link fallback only covered one of the two consumers: the same unvalidated value became the geoMode path segment, so a stale layout would still request stats/location-views/<unknown> and render an error under a link that had quietly fallen back to Countries. Normalizing once covers both and lets the fallback go. Also type the test's hook fixture off the hook itself, so a new LocationView field breaks the test rather than being cast away, and assert the map still renders in Regions mode.
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.
`in` also accepts inherited keys, so `toString` would have passed normalization and reached the endpoint as a path segment — the same failure the normalization was added to close. Also correct a fixture comment: the typing protects the row literals passed to mockReturnValue, not the empty-data constant it sat above.
This comment has been minimized.
This comment has been minimized.
Review-cycle summary —
|
| Round | Source | Finding | Resolution |
|---|---|---|---|
| 1 | claude | it.each title placeholders read in the opposite order to its tuple |
Reworded — 04566ecdac |
| 1 | claude | No test for the behaviour the drill-down reset guards | Added the round-trip test (drill in → Regions → back to Countries); verified it fails against the old guard. The originally-suggested assertion passes either way, since activeSelectedCountry masking already blanks the filter — 04566ecdac |
| 1 | claude | REPORT_SECTIONS lookup not total at runtime |
Typed the values as the report's tab ids so a slug typo fails the build; added a fallback — 04566ecdac |
| 2 | claude | The fallback covered only one of two consumers | Correct: the same value became the stats/location-views/{geoMode} path segment, so a stale layout rendered an error state under a link that had silently fallen back. Normalized once at the boundary; fallback dropped as redundant — 2c0155afe1 |
| 2 | claude | as unknown as cast opted the test fixture out of type checking |
Typed off the hook's own return type — 2c0155afe1 |
| 2 | claude | geo-chart test id never queried |
Asserted in the drill-down test — 2c0155afe1 |
| 3 | claude | in also matches inherited keys |
Correct, and it reopened the same hole: 'toString' in REPORT_SECTIONS is true. Swapped for Object.prototype.hasOwnProperty.call — 7776c285ed |
| 3 | claude | Fixture comment described protection the empty-data constant can't provide | Reworded to name the row literals it actually guards — 7776c285ed |
Declined:
- Plugin changelog entries alongside the package entry. Every widget change in this package ships a package-only entry, and
plugins/premium-analyticsis pre-release. claude reached the same conclusion and raised it only to make the call deliberate. - Regions map can look sparse. Real, but Cities mode already makes the identical top-N-summarised-to-country tradeoff, and map and leaderboard describe the same visible rows. A design call across both modes rather than a divergence to introduce here.
copilot-swe-agent returned no actionable findings in either round.
Unaddressed (flagged for owner): none.
Verification: 7/7 locations tests, package typecheck and ESLint clean, and all three modes driven in a live dashboard after each rebuild — each renders and each "View all" opens its matching report tab.
| }; | ||
|
|
||
| function getGeoChartCountryId( countryCode: string ): string { | ||
| if ( countryCode.toUpperCase() === 'TW' ) { |
There was a problem hiding this comment.
hum why would we need this special handling here 🤔
There was a problem hiding this comment.
Looked at it. I think this line does nothing now. It came in with #50251 the "Avoid unsupported Taiwan map" commit.
The actual fix turned out to be runtimeUnsupportedProvinceMapCountries. Worth deleting in a follow-up rather than here, keep the .toUpperCase() though.
cc @dognose24 for confirming.
There was a problem hiding this comment.
Confirmed — that line came out of the TW "Requested map does not exist" fight, and you're right that the actual fix was runtimeUnsupportedProvinceMapCountries; the province-map error happens on the region: 'TW' + resolution: 'provinces' path, which this row-value mapping never touches. One nuance before deleting: it does change the world-map datatable value from 'TW' to 'Taiwan'. GeoChart's regions mode documents ISO alpha-2 as accepted, so the bare code should highlight the same — worth a quick check against the Cities-mode story (its mock has TW rows) when the follow-up removes it. Keeping .toUpperCase() 👍
| }; | ||
|
|
||
| function getGeoChartCountryId( countryCode: string ): string { | ||
| if ( countryCode.toUpperCase() === 'TW' ) { |
There was a problem hiding this comment.
Looked at it. I think this line does nothing now. It came in with #50251 the "Avoid unsupported Taiwan map" commit.
The actual fix turned out to be runtimeUnsupportedProvinceMapCountries. Worth deleting in a follow-up rather than here, keep the .toUpperCase() though.
cc @dognose24 for confirming.
| // reach the endpoint as a path segment. | ||
| const geoGranularity = Object.prototype.hasOwnProperty.call( REPORT_SECTIONS, storedGranularity ) | ||
| ? storedGranularity | ||
| : 'country'; |
There was a problem hiding this comment.
nit: worth naming the default once?
'country' appears twice here and the two have to agree. Also 'country' carries two meanings in this file. I think a DEFAULT_GEO_GRANULARITY const would improve it, but it's a small thing.
const DEFAULT_GEO_GRANULARITY: GeoGranularity = 'country';A package-only entry reaches the package CHANGELOG and nowhere else, so a user-visible control gaining an option stays invisible to the people who read the plugin changelog.
dognose24
left a comment
There was a problem hiding this comment.
Read through the widget and the report wiring — the regions tab is already registered, the unfiltered request shape is right, and the stale-attribute normalization is a nice touch. Three small notes inline, none blocking.
| // The "View by" control lives in the widget host header (the | ||
| // `relevance: 'high'` attribute). City mode disables country drill-down. | ||
| // `relevance: 'high'` attribute). Only Countries mode drills down, so leaving | ||
| // the other modes would strand a selected country the user can't clear. |
There was a problem hiding this comment.
nit: keying the filter off geoMode leaves the first render after switching a drilled-down Countries view → Regions in a self-contradictory state — the granularity says worldwide while the computed filter still says one country (the clearing effect only runs after render). In practice nothing visible happens: the stale params match the drill-down query already in cache so React Query dedupes the request, and the renderLocationState / isPlaceholderData machinery keeps the transition seamless. Still, deriving the filter from the granularity makes the render self-consistent and leaves the effect as pure UI-state cleanup:
countryFilter: geoGranularity === 'country' ? activeSelectedCountry?.code : undefined,| Significance: minor | ||
| Type: added | ||
|
|
||
| Locations: offer Regions in the widget's "View by" control. |
There was a problem hiding this comment.
nit: the package entry says Locations: while the plugin entry says Top locations: — the card's product name is Top locations, so worth aligning the two.
| } ); | ||
|
|
||
| it.each( [ | ||
| [ undefined, 'countries' ], |
There was a problem hiding this comment.
nit: this case prints as opens the undefined granularity on the countries report tab in the runner output — a literal like 'default' (mapped to “no attribute” in the render call) would read better.
Fixes WOOA7S-1940
Why
Site owners could see which countries their visitors came from, and which cities — but not which regions, unless they clicked into one country at a time and read its regions in isolation. There was no way to ask "which regions send me the most traffic overall". The full Locations report has had a Regions tab all along; the dashboard widget just never offered it. Now it does.
Proposed changes
Countries mode, its click-through drill-down into one country's regions, and the province map for the selected country are all unchanged.
Screenshots
The "View by" control, before and after — same widget, same data, same scroll position:
Regions selected — worldwide regions in the leaderboard with their country flags, summarised by country on the map:
Related product discussion/links
Does this pull request change what data or activity we track or use?
No. Regions mode reads the same
stats/location-views/{geoMode}endpoint the widget already used for the country drill-down, and the Locations report already queries it unfiltered for its Regions tab.Testing instructions
Acceptance criteria from the issue, as a checklist:
The screenshots above come from a local docker site whose
stats/location-viewsresponses were mocked, since an offline-mode site has no live Stats data. Storybook covers the same states without a WordPress install: Packages/Premium Analytics/Widgets/Locations now has a RegionsMode story alongside Default and CitiesMode.The second observation in the issue — "the Top platforms widget is missing platform distribution" — was checked separately and is not the same issue. Its dropdown offers Browser and OS and both render correctly (Windows / macOS / Android / iOS / Linux under OS). The Desktop / Mobile / Tablet split lives in the separate Devices widget. Nothing to fix there; if that split should also appear inside Top platforms, that is a product decision worth its own issue.