Skip to content

Premium Analytics: add Stats app commercial classification endpoint - #49887

Merged
kangzj merged 4 commits into
trunkfrom
endpoint/stats-app-commercial-classification-endpoint
Jun 24, 2026
Merged

Premium Analytics: add Stats app commercial classification endpoint#49887
kangzj merged 4 commits into
trunkfrom
endpoint/stats-app-commercial-classification-endpoint

Conversation

@kangzj

@kangzj kangzj commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Fixes #

Proposed changes

  • Add the Stats app-commercial-classification endpoint query/hook/export wiring.
  • Keep endpoint-owned processing, sanitizer, fixtures, and tests in this PR where this endpoint introduces them.
  • Build on the shared foundation from Premium Analytics: add Stats endpoint foundation #49886, now merged to trunk.

Related product discussion/links

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

No. This adds client-side wrappers and normalization for existing Stats API responses.

Testing instructions

  • Run pnpm --dir projects/packages/premium-analytics typecheck.
  • For endpoints with processing changes, run pnpm --dir projects/packages/premium-analytics test --runInBand.
  • Build coverage was checked on the foundation plus representative resource/app endpoint branches with pnpm --dir projects/packages/premium-analytics build.

@kangzj
kangzj requested review from a team as code owners June 24, 2026 07:52
@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] Needs Team Review Obsolete. Use Needs Review instead. [Package] Premium Analytics labels Jun 24, 2026
@kangzj kangzj self-assigned this Jun 24, 2026
@jp-launch-control

jp-launch-control Bot commented Jun 24, 2026

Copy link
Copy Markdown

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

@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!

@kangzj

kangzj commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

@claude please review this PR.

@claude

claude Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

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


Claude's PR Review

  • Gather context (diff, sibling hooks, proxy fetch, query conventions)
  • Review the new mutation hook + wiring
  • Post review

I reviewed the diff against origin/endpoint/stats-endpoint-foundation. This is a clean, small wrapper PR and the wiring (hook → hooks/index.tsdata/src/index.ts → export test) is consistent with the existing Stats hooks. version: '2' and the commercial-classification prefix match the proxy allowlist documented in AGENTS.md (cap view_stats, POST allowed at the exact path). A few things worth confirming before merge:

1. POST sends params as query string, with an empty body (worth confirming) 🔶

projects/packages/premium-analytics/packages/data/src/hooks/use-stats-app-commercial-classification.ts:8-14

mutationFn: ( params?: StatsQueryParams ) =>
    fetchStatsProxy( {
        version: '2',
        endpoint: 'commercial-classification',
        method: 'POST',
        params,
    } ),

fetchStatsProxy only forwards body as the request payload for POST (data: body); params are appended to the query string (stats-proxy-fetch.ts:62-68). So this mutation issues a POST with the args in the URL and an empty body. If the WPCOM commercial-classification endpoint expects a JSON body (e.g. the classification value), it should be passed via body: instead of params:. If it genuinely takes query params (or no input), this is fine — just want to confirm it's intentional. Fix this →

2. invalidateQueries targets a query key that nothing registers yet 🔶

use-stats-app-commercial-classification.ts:16

queryClient.invalidateQueries( { queryKey: [ 'stats-app', 'plan-usage' ] } );

No query in the package currently uses the plan-usage name (statsAppProxyQuery keys are ['stats-app', name, version, endpoint, …]), so this invalidation is a no-op today. That's expected if plan-usage lands in a later stacked PR — just flagging so it isn't forgotten. Worth a short comment noting the dependency, and double-checking the string matches the future query's name exactly (prefix matching means ['stats-app','plan-usage'] will catch it).

3. Response type is unknown (minor)

The mutation resolves to unknown since fetchStatsProxy's TResponse defaults to unknown. If callers will read the response, consider parameterizing it (fetchStatsProxy< TResponse >) so consumers get a typed result.

4. Import-grouping consistency (nit)

Every sibling hook uses the /** External dependencies */ / /** Internal dependencies */ comment blocks; this file uses plain imports. I confirmed @wordpress/dependency-group is not enabled for this package, so it won't fail lint — purely a consistency nit.

Looks good

  • Naming (…Mutation suffix) cleanly distinguishes it from the query hooks.
  • Export test update is correct; for a thin wrapper with no processing/sanitizer, the absence of a dedicated fixture/test is consistent with the PR description.
  • onSuccess-based invalidation via the shared queryClient is the right pattern.

Overall: no blocking bugs in the wiring. The one substantive item is #1 (params vs. body for the POST) — confirming the WPCOM endpoint's expected request shape would settle it.
· branch endpoint/stats-app-commercial-classification-endpoint

@kangzj
kangzj force-pushed the endpoint/stats-app-commercial-classification-endpoint branch from ff9dc33 to d8ca54f Compare June 24, 2026 08:24
@kangzj

kangzj commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto the clean foundation PR (#49886). @claude please re-review.\n\n

@claude

claude Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

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


Claude's PR Re-review

  • Gather context (rebased diff, foundation base, proxy fetch, sibling hooks)
  • Re-review the new mutation hook + wiring
  • Post review

Re-reviewed against origin/endpoint/stats-endpoint-foundation after the rebase onto #49886. The rebase is clean: the diff content is byte-identical to the previous review (git diff origin/endpoint/stats-endpoint-foundation...HEAD shows the same +26/-0 across the same 5 files), so the foundation swap didn't alter this endpoint's changes. No new issues introduced. The wiring (hook → hooks/index.tsdata/src/index.ts → export test) remains consistent with the sibling Stats hooks, and version: '2' + the commercial-classification endpoint match the proxy allowlist in AGENTS.md (cap view_stats, POST allowed at the exact path).

Carrying forward the prior items, with one now better-substantiated by the foundation:

1. POST sends args as query string with an empty body — confirm intent 🔶

packages/data/src/hooks/use-stats-app-commercial-classification.ts:8-14

mutationFn: ( params?: StatsQueryParams ) =>
    fetchStatsProxy( { version: '2', endpoint: 'commercial-classification', method: 'POST', params } ),

fetchStatsProxy only forwards body as the POST payload (data: body); params go to the query string via addQueryArgs (api/stats-proxy-fetch.ts:55-68). So this issues a POST with args in the URL and an empty body. The proxy does pass query params through to WPCOM (per AGENTS.md), so this works if commercial-classification reads its input from the query string or takes no input. If it expects a JSON body, switch params:body:. Just confirming the WPCOM endpoint's expected request shape. Fix this →

2. invalidateQueries(['stats-app','plan-usage']) is a no-op today 🔶

use-stats-app-commercial-classification.ts:16

No hook in the package registers a plan-usage query yet, so this invalidation matches nothing at runtime. I did confirm the key shape is correct for the future: queries/__tests__/stats-queries.test.ts:73-88 shows statsAppProxyQuery({ name: 'plan-usage', … }) produces ['stats-app', 'plan-usage', '2', …], so the prefix ['stats-app','plan-usage'] will correctly catch it once a plan-usage query lands in a later stacked PR. Worth a one-line comment noting that cross-PR dependency so it isn't forgotten.

3. Response type is unknown (minor)

fetchStatsProxy's TResponse defaults to unknown. If callers read the result, parameterize it (fetchStatsProxy< TResponse >) for a typed return.

4. Import-grouping consistency (nit)

Sibling hooks use the /** External dependencies */ / /** Internal dependencies */ blocks; this file uses plain imports. @wordpress/dependency-group isn't enabled for this package, so it won't fail lint — purely a consistency nit.

Looks good

  • Clean rebase, no content drift from the prior review.
  • …Mutation suffix cleanly distinguishes it from the query hooks.
  • Export-test + barrel wiring are correct; for a thin wrapper with no processing/sanitizer, the absence of a dedicated fixture/test matches the PR description.
  • onSuccess invalidation via the shared queryClient is the right pattern.

Overall: no blocking bugs. The only substantive item is #1 — confirming whether the WPCOM commercial-classification POST wants its input in the body vs. query string would settle it.
· branch endpoint/stats-app-commercial-classification-endpoint

@kangzj

kangzj commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

Addressing claude[bot]'s re-review (comment #4787360143): confirmed the commercial-classification POST intentionally sends inputs as query params, matching the existing Stats Admin controller behavior, and documented that plus the cross-PR plan-usage invalidation dependency in code.\n\n

@kangzj

kangzj commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up for the mutation-hook pattern: switched the commercial-classification mutation to useQueryClient() so invalidation uses the active React Query client. Local pnpm --dir projects/packages/premium-analytics typecheck passed.\n\n

@kangzj

kangzj commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

@claude please re-review.\n\n

@claude

claude Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

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


Claude's PR Re-review

  • Gather context (latest diff, updated hook, proxy fetch)
  • Verify the two follow-up commits address prior feedback
  • Post review

Re-reviewed against origin/endpoint/stats-endpoint-foundation after the two follow-up commits (document commercial classification mutation, use contextual query client). The diff is still a clean +29/-0 across the same 5 files, and the wiring (hook → hooks/index.tsdata/src/index.ts → export test) remains consistent with the sibling Stats hooks. Both substantive items from the prior review are now resolved.

Resolved since last review ✅

1. POST args via query string — now documented & confirmed intentional. use-stats-app-commercial-classification.ts:14 adds an inline comment ("The WPCOM endpoint reads POST inputs from the query string"), and the author confirmed this matches the existing Stats Admin controller behavior. This is consistent with AGENTS.md: commercial-classification is an allowlisted POST-at-exact-path prefix (cap view_stats), and the proxy passes query params through to WPCOM. fetchStatsProxy only forwards body as the POST payload (data: body, stats-proxy-fetch.ts:67), so sending input via params + empty body is the correct shape here. Settled.

2. invalidateQueries(['stats-app','plan-usage']) — now documented as a cross-PR dependency. use-stats-app-commercial-classification.ts:18 adds a comment ("Plan usage lands in a sibling endpoint PR and shares this app query prefix"). It's still a no-op until the plan-usage query lands, but the dependency is now recorded and the prefix key shape is correct (statsAppProxyQuery({ name: 'plan-usage' })['stats-app','plan-usage','2',…], so the prefix will catch it). Good.

Follow-up: useQueryClient() switch is correct. Moving from a shared imported client to useQueryClient() (:6) is the idiomatic React Query pattern — invalidation now targets the active provider's client rather than a module-level singleton. Nice catch on your own follow-up.

Remaining (non-blocking)

3. Response type defaults to unknown (minor). fetchStatsProxy< TResponse > defaults TResponse to unknown, so the mutation resolves to unknown. For a fire-and-invalidate mutation that doesn't read the response this is fine; only worth parameterizing if a caller needs the classification result back. Fix this →

4. Import-grouping consistency (nit). Sibling hooks use the /** External dependencies */ / /** Internal dependencies */ comment blocks; this file uses plain imports. @wordpress/dependency-group isn't enabled for this package so it won't fail lint — purely a consistency nit.

Looks good

  • Both substantive review items addressed with in-code documentation.
  • …Mutation suffix cleanly distinguishes it from the query hooks; export-test + barrel wiring are correct and the changelog entry is present.
  • For a thin wrapper with no processing/sanitizer, the absence of a dedicated fixture/test matches the PR description.

Overall: no blocking issues. The prior substantive concerns are resolved; only the two minor/nit items (#3, #4) remain, neither of which needs to block merge.
· branch endpoint/stats-app-commercial-classification-endpoint

Base automatically changed from endpoint/stats-endpoint-foundation to trunk June 24, 2026 21:33
@kangzj
kangzj force-pushed the endpoint/stats-app-commercial-classification-endpoint branch from d2ec97d to 7f4f82b Compare June 24, 2026 21:39
@kangzj
kangzj merged commit 2f39750 into trunk Jun 24, 2026
71 of 72 checks passed
@kangzj
kangzj deleted the endpoint/stats-app-commercial-classification-endpoint branch June 24, 2026 22:13
@github-actions github-actions Bot removed the [Status] Needs Team Review Obsolete. Use Needs Review instead. label Jun 24, 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.

1 participant