Premium Analytics: Align Locations widget guidelines

[dognose24]

Fixes #

Proposed changes

• Add comparison-period support to the Locations widget leaderboard.
• Pass widget attributes into WidgetRoot and keep the Locations header visible through loading, error, and empty states.
• Add Locations Storybook coverage with Default, WithComparison, and dashboard harness stories using Stats proxy mocks for location views.
• Keep the existing country-to-region drill-down behavior unchanged.

Related product discussion/links

• N/A

Does this pull request change what data or activity we track or use?

No. It only changes how existing Stats Locations data is displayed and mocked in Storybook.

Testing instructions

• Run pnpm --dir projects/packages/premium-analytics run typecheck
• Run pnpm --dir projects/packages/premium-analytics run build:wp-build
• In Storybook, open Packages/Premium Analytics/Widgets/Locations and verify Default, WithComparison, and WidgetDashboardWithWidget.
• In the widget, click a country row and verify it still drills into regions, then use the breadcrumb to return to Top Locations.

[github-actions]

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

• ✅ Include a description of your PR changes.
  
• ✅ Add a "[Status]" label (In Progress, Needs Review, ...).
  
• ✅ Add testing instructions.
  
• ✅ Specify whether this PR includes any changes to data or privacy.
  
• ✅ Add changelog entries to affected projects
  

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:

1. Ensure all required checks appearing at the bottom of this PR are passing.
2. Make sure to test your changes on all platforms that it applies to. You're responsible for the quality of the code you ship.
3. You can use GitHub's Reviewers functionality to request a review.
4. When it's reviewed and merged, you will be pinged in Slack to deploy the changes to WordPress.com simple once the build is done.

If you have questions about anything, reach out in #jetpack-developers for guidance!

[claude]

Claude finished @dognose24's task in 3m 31s —— View job

---

Claude PR Review — Premium Analytics: Align Locations widget guidelines

• Gather context (read changed files & diff)
• Review render.tsx changes
• Review use-location-views.ts changes
• Review widget.ts changes
• Review Storybook + mocks
• Post review feedback

Overall this is a clean, well-scoped change that aligns the Locations widget with the Stats-widget conventions in AGENTS.md. The comparison wiring mirrors the established top-posts pattern, presentation is correctly moved out of widget.ts into widget.json, the attribute type is declared once in widget.ts and composed in render.tsx, and attributes are now correctly passed into <WidgetRoot>. Nice work. A few notes below — mostly minor.

---

Things that look good ✅

• Comparison mapping follows the contract. comparisonMap keyed by a stable key (countryCode:label), previousValue matched by key rather than by index, and previousShare/delta derived from it — exactly what the "Comparison data" section in AGENTS.md prescribes, and consistent with top-posts/render.tsx. calculateDelta already handles the previous === 0 ("new") edge case, so brand-new locations show +100% rather than Infinity. 👍
• <WidgetRoot attributes={ attributes }> — host reportParams/comparison state is no longer dropped at the boundary, so the Storybook withComparison control is actually meaningful.
• presentation removed from widget.ts and kept only in widget.json — matches the guideline.
• Mocks mirror the structure and idempotency guard of the existing register-report-mocks.ts, and return the raw WPCOM proxy shape so the normalizer runs on top, matching the real data flow.

---

Suggestions / questions

1. Empty-state branch vs. emptyStateText guideline (deliberate deviation worth a maintainer's eye)
The Stats-widgets guideline says: "Empty state: pass emptyStateText to LeaderboardChart — do not add a separate data.length === 0 render branch in the widget." This PR keeps the explicit empty branch (render.tsx:195-211). That appears intentional — it's how you keep the header (breadcrumb + "View by" selector) visible in the empty state, which is one of the PR's stated goals. So this is a reasonable, justified deviation, but it's worth calling out explicitly since it runs counter to the documented rule. If you agree the header should stay, consider a one-line code comment noting why the branch exists, so a future reader doesn't "fix" it back to emptyStateText.

2. No in-place loading indicator on background refetch
The guideline also says: "When stale data exists, pass loading={ isLoading } to the chart component so an in-place spinner appears without hiding the rows." Here the only loading affordance is <WidgetLoadingOverlay />, gated on isLoading && data.length === 0 (render.tsx:171). When the user switches mode (Countries↔Cities) or drills into a country, that's a fresh query — if React Query keeps placeholder rows, isLoading is true but data.length > 0, so there's no spinner at all during the refetch; if it doesn't, the rows blank to the overlay. Consider passing loading={ isLoading } to <LeaderboardChart> (if it supports it) for the stale-data case. Minor UX polish, not a blocker.

3. Orphaned comment in use-location-views.ts
The comment block kept at use-location-views.ts:88-90 still describes isFetching ("isFetching covers background refetches where stale data is already available; those should not blank the widget"), but isFetching is no longer destructured or used after the refactor — the hook now returns the combined isLoading from useStatsLocations. The comment now describes behavior the widget doesn't implement (see #2). Either re-wire isFetching/loading, or trim the comment so it doesn't mislead.

4. JSDoc param name mismatch on LocationsInner
render.tsx:39-40 documents @param props / @param props.max, but the function destructures its argument (function LocationsInner( { max }: { max: number } )) and has no props identifier. The repo's convention for destructured params is the root0 / root0.max form (as the outer Locations JSDoc still uses). Depending on the jsdoc/check-param-names config this may trip the linter — worth running pnpm lint / the eslint task on the package to confirm.

5. Storybook comparison detection is coupled to the default preset
register-stats-mocks.ts:isComparisonRequest() decides primary-vs-comparison purely by daysFromToday > 1 on the request's date param. That works for the shipped stories (default last-30-days, so the comparison window's date lands ~30 days back while the primary date is today). But it's fragile: for a today preset the comparison request's date would be yesterday (daysFromToday === 1), which the > 1 check would misclassify as primary. Since the stories only use the default preset this is fine in practice — just flagging that the heuristic is preset-dependent. A short code comment stating the assumption would help. (The sibling report mock uses request-signature tracking instead, which is more robust but heavier.)

6. Minor: repeated header + .content wrapper
The loading/error/empty/success branches each repeat { header } + <div className={ styles.content }>. Not a problem, but a small withChrome( children ) wrapper or a single shell with a switch on state would cut the duplication. Purely optional.

---

Nothing here is a correctness blocker. #3 and #4 are quick cleanups; #1 and #2 are guideline-alignment judgment calls worth a maintainer's confirmation. I didn't run the typecheck/build/Storybook steps from the testing instructions in this environment — please make sure pnpm --dir projects/packages/premium-analytics run typecheck and the package lint pass, particularly given #4.
· codex/locations-widget-guideline

[jp-launch-control]

Code Coverage Summary

This PR did not change code coverage!

That could be good or bad, depending on the situation. Everything covered before, and still is? Great! Nothing was covered before? Not so great. 🤷

Full summary · PHP report

[claude]

Claude finished @dognose24's task in 1m 7s —— View job

---

Claude PR Re-Review — Premium Analytics: Align Locations widget guidelines

• Review fixes for prior feedback (Use class_exists() guard for Featured_Content. #1, Edit and rename the readme for GitHub #3, Merge the changes in 2.7.1 upstream to master. #4, Create a readme.md for the Jetpack GitHub Repository. #5)
• Re-read changed files at current HEAD
• Post updated review

Re-reviewed at 11a596c8. Every actionable item from the previous review is resolved cleanly. 👍

#	Item	Status
1	Empty-state branch deviation	✅ Comment added at render.tsx:195-197 explaining why the explicit branch exists (keeps header/breadcrumb + "View by" interactive). Clear and accurate.
3	Orphaned isFetching comment	✅ Removed (use-location-views.ts).
4	JSDoc param name mismatch	✅ Now @param root0 / @param root0.max (render.tsx:39-40), matching the destructured-param convention the outer Locations already uses — satisfies jsdoc/check-param-names.
5	Storybook comparison heuristic	✅ Comment added above isComparisonRequest() documenting the daysFromToday > 1 assumption and its preset dependence.
2	In-place loading on refetch	⏭️ Deferred — reasonable; keeps scope tight.
6	Repeated chrome wrapper	⏭️ Left as-is — fine, purely optional.

The fixes are all correct and the deferrals are well-justified. Nice, tight follow-up.

---

One small new observation (optional)

Outer component doesn't default attributes. The widget contract (.agents/rules/widgets.md) says the render component receives { attributes } that "may arrive empty" and should be defaulted (attributes = {}), and the template uses WidgetRenderProps with attributes = {}. The outer Locations currently destructures without a default:

export default function Locations( { attributes }: LocationsWidgetProps ) {
	const max = attributes?.max ?? 10;
	return (
		<WidgetRoot attributes={ attributes }>

attributes?.max guards the max read, but <WidgetRoot attributes={ attributes }> forwards a possibly-undefined value rather than {}. In practice the host always injects attributes, so this is a cosmetic alignment nit, not a bug — worth a = {} default only if you want to match the documented template exactly. Fix this →

---

Note: I was not able to run pnpm --dir projects/packages/premium-analytics run typecheck in this environment (command not permitted), so please confirm the typecheck and package lint pass locally per the testing instructions — especially given the JSDoc change. Everything I could verify by reading looks correct.

No blockers. ✅
· codex/locations-widget-guideline

[dognose24]

🤖 Review-cycle summary — 24d0c04c66 → 9804c092cd

4 rounds; CI green; 2 claude[bot] review passes addressed; 0 inline threads; 0 unaddressed human comments.

What changed during the cycle

Commits added during the cycle:

• 11a596c8ea — Address review: fix JSDoc param tag, drop orphaned isFetching comment, add empty-state and mock heuristic comments
• 9804c092cd — Premium Analytics: Default Locations attributes

Diff summary: 3 files changed, 11 insertions(+), 6 deletions(-)

Review threads addressed:

Source	Comment	Resolution
claude[bot]	(#4834865182) 6 items: empty-state deviation, orphaned comment, JSDoc mismatch, mock heuristic, in-place loading, chrome wrapper	Fixed #1/#3/#4/#5; deferred #2/#6 as non-blocking
claude[bot]	(#4834984410) LGTM + nit: attributes = {} default missing	Already applied by owner before re-review; confirmed resolved

Unaddressed (flagged for owner): None.

CI: all required checks passing.

[chihsuan]

Nice work @dognose24

Nothing blocking. Two notes worth a look before merge:

1. No loading state on refetch. suggests a WidgetLoadingOverlay was intended
2. The in-body Top Locations heading feel duplicate the host's Locations title. Sibling widgets like Top Posts render body-only.