Skip to content

Premium Analytics: add Stats proxy data hooks - #49773

Closed
kangzj wants to merge 7 commits into
trunkfrom
add/stats-proxy-data-hooks
Closed

Premium Analytics: add Stats proxy data hooks#49773
kangzj wants to merge 7 commits into
trunkfrom
add/stats-proxy-data-hooks

Conversation

@kangzj

@kangzj kangzj commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Proposed changes

  • Add a shared Premium Analytics data fetcher for the unified Stats proxy route.
  • Add consistently named useStats... hooks and mutations for the WPCOM-proxied Stats endpoint families from Premium Analytics: extend the API proxy to cover Stats-Admin endpoints #49571.
  • Add lightweight Stats response normalization for widget/report-style data, including fixture-backed normalizer tests.
  • Export Stats proxy helpers, query utilities, normalized types, and hook APIs from the data package.

Related product discussion/links

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

No. This adds frontend data-layer access to existing proxied Stats endpoints and does not add or change tracking.

Testing instructions

  • pnpm --dir projects/packages/premium-analytics test
  • pnpm --dir projects/packages/premium-analytics typecheck
  • pnpm --dir projects/packages/premium-analytics build

@kangzj kangzj added Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Stats Data Feature that enables users to track their site's traffic and gain insights on popular content. [Status] In Progress labels Jun 19, 2026
@kangzj kangzj self-assigned this Jun 19, 2026
@kangzj

kangzj commented Jun 19, 2026

Copy link
Copy Markdown
Contributor Author

@claude please review this PR.

@claude

claude Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kangzj's task in 2m 45s —— View job


PR Review: Premium Analytics Stats proxy data hooks

  • Read changed files & gather context
  • Review API/fetch layer
  • Review hooks & queries
  • Review normalization/processing & tests
  • Post review feedback

Nice, well-structured PR. The layering (fetch → query factory → hook) is clean and consistent with the existing Woo report hooks, and the useStats* naming maps cleanly onto the proxy prefixes documented in AGENTS.md. Tests + fixtures for the normalizers and param mapping are a good touch. A few correctness items worth addressing before merge.


🐛 Object spread overrides sanitized label/value

In two normalizers the raw item is spread after the computed label/value, so the raw (often string) fields clobber the values you just sanitized.

processing/stats/index.ts:401-416sanitizeStatsDevicesResponse (array branch):

data: response.map( item => ( {
    label: asRecord( item ).label ?? asRecord( item ).name ?? '',
    value: safeParseFloat( asRecord( item ).value ?? asRecord( item ).views ),
    ...asRecord( item ),   // ← if item.value is "1234" (string), it overwrites the parsed number
} ) ),

processing/stats/index.ts:442-446sanitizeStatsGenericListResponse has the same pattern (...item after label/value).

When the source item carries a value (or label) key, the spread reverts your normalization back to the raw payload value, defeating safeParseFloat. The fix is to spread first, then override:

data: response.map( item => ( {
    ...asRecord( item ),
    label: asRecord( item ).label ?? asRecord( item ).name ?? '',
    value: safeParseFloat( asRecord( item ).value ?? asRecord( item ).views ),
} ) ),

Worth noting the existing normalizer tests don't catch this because the device/generic-list paths aren't fixture-covered. Fix this →


⚠️ Path-only params leak into the query string

queries/stats-queries.tsgeoMode, utmParams, and deviceProperty are used to build the endpoint path, but they're never stripped from params before being passed through reportParamsToStatsQueryParams (which only deletes a fixed allowlist in utils/stats-params.ts:67-78). They end up spread into the final query string:

  • statsLocationsQuery?geoMode=country
  • statsUtmQuery?utmParams=utm_source,utm_medium
  • statsDevicesQuery?deviceProperty=screensize

Probably harmless (WPCOM should ignore unknown args), but it pollutes the request and varies the React Query cache key unnecessarily. Consider deleting these in reportParamsToStatsQueryParams or stripping them in the query factories.


💬 Minor / nits

  • sanitize in the query key (stats-queries.ts:61-70): the sanitize function is included in queryKey. React Query's default hash serializes functions to null, so it adds nothing to uniqueness (the name/version/endpoint already disambiguate). It's dead weight and a little misleading — safe to drop.
  • Shared EMPTY_REPORT constant (processing/stats/index.ts:40-43, returned from sanitizeStatsGenericListResponse): returning a shared mutable object means a consumer mutating .data/.summary would corrupt it for everyone. Returning a fresh { summary: {}, data: [] } is cheap and safer.
  • StatsProxyVersion = ... | ( string & {} ) (stats-proxy-fetch.ts:11): the string & {} trick (preserve literal autocomplete while allowing any string) is fine, but {} can trip @typescript-eslint/ban-types — worth confirming lint passes.
  • num/max defaults (stats-params.ts:94-95): every report query gets num: 1, max: 10 by default. Just double-check these match the legacy Odyssey defaults per endpoint — num (period count) semantics differ from the days you also compute, and a wrong default could silently truncate results.
  • getStatsBucket prefers start_date (processing/stats/index.ts:80-86): in the report flow both date (=to) and start_date (=from) are set, so the bucket lookup keys on from. If the API returns a single bucket keyed by the end date, this falls back to getFirstDayBucket — usually fine, but worth a sanity check against a real multi-day response.

These are all small relative to the surface area; the core design is solid. The spread-override bug is the one I'd consider blocking since it silently undoes normalization for the devices/generic-list endpoints.
add/stats-proxy-data-hooks

@github-actions

Copy link
Copy Markdown
Contributor

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!

@jp-launch-control

jp-launch-control Bot commented Jun 19, 2026

Copy link
Copy Markdown

Code Coverage Summary

No summary data is available for parent commit 0d686fc, so cannot calculate coverage changes. 😴

If that commit is a feature branch rather than a trunk commit, this is expected. Otherwise, this should be updated once coverage for 0d686fc is available.

Full summary · PHP report

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a Premium Analytics data-layer surface for the unified Stats proxy route, including fetch utilities, React Query query factories, useStats… hooks/mutations, and response normalization for common widget/report-style payloads.

Changes:

  • Introduces fetchStatsProxy / getStatsProxyPath and Stats query factories for many Stats endpoint families.
  • Adds Stats response normalizers plus fixture-backed unit tests.
  • Exposes useStats… hooks and re-exports Stats APIs/types from the data package entrypoints.

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
projects/packages/premium-analytics/packages/data/src/utils/stats-params.ts Adds helpers to convert report params into Stats query params and build stable query-key parts.
projects/packages/premium-analytics/packages/data/src/utils/tests/stats-params.test.ts Tests interval→period mapping and report→stats param conversion.
projects/packages/premium-analytics/packages/data/src/queries/stats-queries.ts Adds React Query option factories for many Stats endpoints via the proxy fetcher.
projects/packages/premium-analytics/packages/data/src/queries/index.ts Re-exports Stats queries.
projects/packages/premium-analytics/packages/data/src/processing/stats/index.ts Adds Stats response normalization utilities/types.
projects/packages/premium-analytics/packages/data/src/processing/stats/tests/stats.test.ts Tests key normalizers using fixtures.
projects/packages/premium-analytics/packages/data/src/processing/stats/fixtures/stats.ts Provides fixture payloads for normalizer tests.
projects/packages/premium-analytics/packages/data/src/processing/index.ts Re-exports Stats processing.
projects/packages/premium-analytics/packages/data/src/index.ts Exports Stats hooks, proxy helpers, query/utils, and normalized types.
projects/packages/premium-analytics/packages/data/src/hooks/use-stats.ts Adds useStats… hooks and mutations for Stats proxy endpoints.
projects/packages/premium-analytics/packages/data/src/hooks/use-report.ts Generalizes useReport params typing and broadens primary/comparison param forwarding.
projects/packages/premium-analytics/packages/data/src/hooks/index.ts Re-exports Stats hooks.
projects/packages/premium-analytics/packages/data/src/hooks/tests/stats-exports.test.ts Ensures useStats… hooks are exported from the package entry.
projects/packages/premium-analytics/packages/data/src/api/stats-proxy-fetch.ts Adds Stats proxy fetcher and URL builder for /jetpack-premium-analytics/v1/proxy/v<version>/….
projects/packages/premium-analytics/packages/data/src/api/index.ts Re-exports Stats proxy fetch APIs/types.
projects/packages/premium-analytics/packages/data/src/api/constants.ts Adds statsProxyPath constant.
projects/packages/premium-analytics/packages/data/src/api/tests/stats-proxy-fetch.test.ts Tests Stats proxy path construction (including comma segments).
projects/packages/premium-analytics/changelog/add-stats-proxy-data-hooks Adds changelog entry for the new data hooks.

Comment thread projects/packages/premium-analytics/packages/data/src/processing/stats/index.ts Outdated
Comment thread projects/packages/premium-analytics/packages/data/src/processing/stats/index.ts Outdated
@kangzj
kangzj requested review from a team June 19, 2026 03:10
@kangzj

kangzj commented Jun 19, 2026

Copy link
Copy Markdown
Contributor Author

I'm adding everything in one-go and potentially split up in smaller chunks for reviews.

@kangzj
kangzj force-pushed the add/stats-proxy-data-hooks branch from e4efb58 to d9a2820 Compare June 19, 2026 03:26
@kangzj
kangzj marked this pull request as ready for review June 19, 2026 03:26
@kangzj kangzj added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Jun 19, 2026
@kangzj

kangzj commented Jun 19, 2026

Copy link
Copy Markdown
Contributor Author

@claude please review the latest changes.

Implemented the follow-up pass after checking live Stats endpoint shapes:

  • Separated report/stat hooks from app/admin resources: report hooks stay under useStats...; dashboard/settings/upgrades/notices/admin resources now use useStatsApp... and a separate stats-app query-key namespace.
  • Normalized additional live endpoint shapes for subscribers, publicize, followers, tags, comments, comment followers, email summary, and email breakdown matrix responses.
  • Ensured time-series endpoints normalize to Premium Analytics-style rows with time_interval, date_start, date_end, metric keys like views/visitors/subscribers, and numeric value. This now covers visits, subscribers, single-post, email opens/clicks timelines, and WordAds stats.
  • Addressed Copilot/Claude feedback: removed empty date/start_date query args, stripped path-only params, made query-key body serialization stable, and kept raw spreads from overriding normalized values.
  • Added fixture-backed normalizer tests while keeping the coverage light.

Validation:

  • pnpm --dir projects/packages/premium-analytics test
  • pnpm --dir projects/packages/premium-analytics typecheck
  • pnpm --dir projects/packages/premium-analytics build

@claude

claude Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kangzj's task in 2m 18s —— View job


PR Review: latest changes (follow-up pass)

  • Read changed files & gather context
  • Verify previous review feedback was addressed
  • Review new useStatsApp split & query namespaces
  • Review new normalizers & time-series rows
  • Post review feedback

Thanks for the thorough follow-up — this is a solid pass. I confirmed the earlier feedback is all addressed, and the new app/report split reads cleanly. One concrete server-contract bug stands out, plus a couple of small notes.


✅ Previous feedback — verified fixed

  • Spread-override...row / ...record / ...item now come before the computed label/value in sanitizeStatsTimeSeriesResponse (index.ts:336-343), sanitizeStatsDevicesResponse (index.ts:1013-1020), and sanitizeStatsGenericListResponse (index.ts:1061-1065). ✔️
  • Empty date/start_date — now conditionally spread (stats-params.ts:94-95), so missing dates are omitted rather than sent as date=. ✔️
  • Path-only paramsgeoMode/utmParams/deviceProperty are stripped in reportParamsToStatsQueryParams (stats-params.ts:79-81). ✔️
  • Query-key stabilitystatsQueryKeyPart does stable, sorted JSON serialization, and queryKey now carries the sanitizer string key rather than a function reference (stats-queries.ts:99-108). ✔️
  • Shared empty reportemptyReport() returns a fresh object now. ✔️

🐛 useStatsAppUserFeedbackMutation will be rejected with 405 rest_read_only

use-stats-app.ts:180-190 POSTs to jetpack-stats/user-feedback:

fetchStatsProxy( { version: '2', endpoint: 'jetpack-stats/user-feedback', method: 'POST', body } );

But the proxy's PREFIX_CONFIG entry for the jetpack-stats prefix has no writes (src/REST/class-api-proxy-controller.php:120):

'jetpack-stats' => array( 'capability' => 'view_stats' ),   // no 'writes'

is_write_allowed() (:390-405) iterates $config['writes'] ?? array(), so for this prefix it always returns false → any non-GET is rejected with 405 rest_read_only. As written, this mutation can never succeed against the proxy.

Two options:

  • If user-feedback should be writable, add a writes entry for it server-side (e.g. 'writes' => array( 'jetpack-stats/user-feedback' )) and extend data_endpoint_matrix() — but that's a security-boundary change and probably belongs in the proxy PR (Premium Analytics: extend the API proxy to cover Stats-Admin endpoints #49571), not here.
  • Otherwise drop/defer this mutation until the prefix allows the write.

For contrast, the other mutations are covered by the config: stats/referrers/spam/new|delete match the stats/referrers/spam/ write prefix, jetpack-stats-dashboard/* match the jetpack-stats-dashboard/ prefix, and commercial-classification matches its exact-path write. Only user-feedback falls through. Fix this →


💬 Minor / nits

  • POST params vs. body for mutations (use-stats-app.ts:107-121, 192-202): markSpam/unmarkSpam send domain and commercial-classification sends params as query-string args (since fetchStatsProxy only forwards body as data for POST). The proxy passes query params through, so this should reach WPCOM, but worth a quick confirm against the legacy callers that these endpoints read the value from the query string rather than the request body.
  • num: 1 / max: 10 defaults still apply to every report query (stats-params.ts:97-98): carried over from the prior review — just re-flagging that a global num: 1 (period count) could truncate multi-period report responses if any endpoint expects more; double-check against the Odyssey per-endpoint defaults.
  • days-shaped time series fallback (index.ts:258-260): when response.days is a flat { date: number } map (value is a scalar, not an object), asRecord(value) returns {}, so the row has no metric and getPrimaryMetricValue yields 0. Fine if no live endpoint returns that shape, but it's the one time-series branch without fixture coverage — worth a guard or a test if any endpoint uses it.
  • StatsProxyVersion = ... | ( string & {} ) (stats-proxy-fetch.ts:11): still using the {} trick — just confirm @typescript-eslint/ban-types lint passes in CI (typecheck alone won't catch it).

Net: the normalization layering and the report/app split are well done, and the earlier correctness issues are genuinely fixed. The one I'd treat as blocking is the user-feedback mutation — it's wired to an endpoint the proxy will reject. Everything else is confirm-and-go.

Note: I couldn't run pnpm test/typecheck here (command not permitted in this job), so I relied on static review plus your reported validation results.
add/stats-proxy-data-hooks

@kangzj

kangzj commented Jun 19, 2026

Copy link
Copy Markdown
Contributor Author

Addressing Claude follow-up review: the unsupported useStatsAppUserFeedbackMutation was removed in b4ca2a4452 because the proxied jetpack-stats prefix is read-only for writes in the current server contract.

@kangzj
kangzj marked this pull request as draft June 19, 2026 03:44
@kangzj

kangzj commented Jun 19, 2026

Copy link
Copy Markdown
Contributor Author

@claude please re-review the latest changes.

Outstanding feedback addressed:

  • b4ca2a4452: removed useStatsAppUserFeedbackMutation because the current proxied jetpack-stats prefix does not allow writes.
  • 4798b56963: added a defensive scalar days time-series fallback and fixture coverage so { date: number } maps normalize to numeric value rows.
  • Confirmed the remaining POST query-string note against projects/packages/stats-admin/src/class-rest-controller.php: referrer spam and commercial classification legacy callers also forward request query params to WPCOM for those POSTs.
  • Confirmed lint/typecheck/build locally after the {} Stats proxy version note; local validation remains green.

Validation after latest changes:

  • pnpm --dir projects/packages/premium-analytics test
  • pnpm --dir projects/packages/premium-analytics typecheck
  • pnpm --dir projects/packages/premium-analytics build

All inline review threads that were already addressed have been replied to with the relevant hash and resolved.

@claude

claude Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kangzj's task in 1m 13s —— View job


PR Re-review: Premium Analytics Stats proxy data hooks

  • Verify b4ca2a4452 — removed unsupported user-feedback mutation
  • Verify 4798b56963 — scalar days time-series fallback + fixtures
  • Verify POST query-string note against legacy callers
  • Confirm {} proxy version / lint concern
  • Final scan of latest diff & post review

Thanks for the thorough follow-up — everything I flagged as blocking or worth-a-guard is now resolved. This looks ready to me.


✅ Outstanding feedback — verified fixed

  • user-feedback mutation (the blocking one)useStatsAppUserFeedbackMutation is fully removed in b4ca2a4452, and a repo-wide grep confirms no dangling UserFeedback / user-feedback references remain in the package (no orphaned export from index.ts, no test reference). Correct call given the jetpack-stats prefix is read-only in the current proxy contract — adding a writes entry is a security-boundary change that belongs in the proxy PR (Premium Analytics: extend the API proxy to cover Stats-Admin endpoints #49571). ✔️

  • Scalar days time-series fallback4798b56963 handles it cleanly (processing/stats/index.ts:258-264):

    if ( typeof value === 'number' || typeof value === 'string' ) {
        return numericTimeSeriesRow( { period, value } );
    }
    return numericTimeSeriesRow( { period, ...asRecord( value ) } );

    The string case ('7') routes through numericTimeSeriesRowsafeParseFloat → numeric 7, so getPrimaryMetricValue no longer yields 0 for flat { date: number } maps. The new fixture (scalarDaysTimeSeriesFixture mixes '7' and 3) plus the assertion that summary.value === 10 exercises both the string-parse and the cross-row summation paths. Good coverage. ✔️

  • POST query-string params — confirmed against stats-admin's class-rest-controller.php: the legacy referrer-spam and commercial-classification callers also forward request params via the query string for those POSTs, so the proxy behavior matches. Consistent with existing contract. ✔️

  • StatsProxyVersion = … | ( string & {} ) lint — taking your local pnpm lint/build green as the source of truth here (the CI job can't run pnpm). No further action. ✔️


💬 Non-blocking, for whenever the consuming screens land

These are carried-over notes, not regressions — fine to defer:

  • num: 1 / max: 10 report defaults (utils/stats-params.ts): still worth a real-data sanity check per endpoint when the dashboards wire up, in case any report expects more than one period. Static review can't confirm the Odyssey per-endpoint defaults.
  • getStatsBucket end-date fallback (processing/stats/index.ts:181-197): keys on query.date ?? query.start_date then falls back to the first day bucket — behaviorally fine; just the kind of thing best confirmed against a live multi-day response.

Net: the blocking user-feedback issue is gone, the last time-series gap is closed with a test, and the rest is confirm-and-go. Nice work splitting the report/app surfaces — the layering held up well across the review passes. 👍

Note: as before, I reviewed statically — pnpm test/typecheck/build aren't runnable in this job, so I'm relying on your reported green validation for those.
add/stats-proxy-data-hooks

@kangzj

kangzj commented Jun 19, 2026

Copy link
Copy Markdown
Contributor Author

Additional minor/nit follow-up addressed in 3dd586508e:

  • Removed implicit num: 1 / max: 10 from Stats report param conversion so the data layer does not silently truncate responses. Callers can still pass explicit num / max, now covered by tests.
  • Narrowed StatsProxyVersion to the currently supported proxy versions instead of using the string & {} autocomplete trick.

Validation after this commit:

  • pnpm --dir projects/packages/premium-analytics test
  • pnpm --dir projects/packages/premium-analytics typecheck
  • pnpm --dir projects/packages/premium-analytics build

@kangzj
kangzj marked this pull request as ready for review June 19, 2026 03:46
@kangzj

kangzj commented Jun 19, 2026

Copy link
Copy Markdown
Contributor Author

Moved subscriber counts and WordAds earnings back into the stats hook/query surface in c2b9cdff08:

  • statsSubscribersCountsQuery / useStatsSubscribersCounts now live with the stats hooks.
  • statsWordAdsEarningsQuery / useStatsWordAdsEarnings now live with the stats hooks.
  • Removed the statsApp... variants for those two resources so app/admin hooks only cover dashboard/settings/upgrades/notices/admin-style resources.

Validation before commit:

  • pnpm --dir projects/packages/premium-analytics test
  • pnpm --dir projects/packages/premium-analytics typecheck
  • pnpm --dir projects/packages/premium-analytics build

@kangzj

kangzj commented Jun 19, 2026

Copy link
Copy Markdown
Contributor Author

I split this PR into a smaller stacked series. Please review the replacement PRs instead:

  1. Premium Analytics: add Stats proxy foundation #49775 — Stats proxy foundation, against trunk.
  2. Premium Analytics: add Stats traffic normalizers #49776 — Traffic normalizers, stacked on Premium Analytics: add Stats proxy foundation #49775.
  3. Premium Analytics: add Stats traffic queries #49777 — Traffic query factories, stacked on Premium Analytics: add Stats traffic normalizers #49776.
  4. Premium Analytics: add Stats traffic hooks #49778 — Traffic hooks, stacked on Premium Analytics: add Stats traffic queries #49777.
  5. Premium Analytics: normalize Stats time-series reports #49779 — Time-series and email normalizers, stacked on Premium Analytics: add Stats traffic hooks #49778.
  6. Premium Analytics: normalize secondary Stats reports #49780 — Secondary report normalizers, stacked on Premium Analytics: normalize Stats time-series reports #49779.
  7. Premium Analytics: add remaining Stats hooks #49781 — Remaining Stats query factories/hooks, stacked on Premium Analytics: normalize secondary Stats reports #49780.
  8. Premium Analytics: add Stats app hooks #49782 — Stats app/admin hooks, stacked on Premium Analytics: add remaining Stats hooks #49781.

Each split PR follows the PR template and includes a "What changed and why" section plus its non-test/support line count. Review-loop kickoff comments have been posted on the split PRs.

@kangzj kangzj closed this Jun 19, 2026
@github-actions github-actions Bot removed the [Status] Needs Review This PR is ready for review. label Jun 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Stats Data Feature that enables users to track their site's traffic and gain insights on popular content. [Package] Premium Analytics

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants