Skip to content

Premium Analytics: add Stats app dashboard modules endpoint#49889

Draft
kangzj wants to merge 4 commits into
trunkfrom
endpoint/stats-app-dashboard-modules-endpoint
Draft

Premium Analytics: add Stats app dashboard modules endpoint#49889
kangzj wants to merge 4 commits into
trunkfrom
endpoint/stats-app-dashboard-modules-endpoint

Conversation

@kangzj

@kangzj kangzj commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Fixes #

Proposed changes

  • Add Premium Analytics data package support for the Stats app dashboard modules endpoint, following the Stats endpoint patterns established in Premium Analytics: add Stats endpoint foundation #49886.
  • Wire the endpoint-specific query, hook, public exports, and sanitizer/normalizer registration where applicable.
  • Keep endpoint typing tied to sanitizer keys or passthrough query inference, with focused fixtures/tests for the raw endpoint payload shape where normalization is introduced.

Related product discussion/links

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

No. This only adds typed client data/query integration for an existing Stats API endpoint.

Testing instructions

  • pnpm --dir projects/packages/premium-analytics test -- packages/data/src/queries/tests/stats-queries.test.ts packages/data/src/hooks/tests/stats-exports.test.ts --runInBand
  • pnpm exec eslint --max-warnings=0
  • pnpm --dir projects/packages/premium-analytics run typecheck
  • git diff --check

@kangzj kangzj requested review from a team as code owners June 24, 2026 07:53
@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 0s —— View job


PR Review: Stats app dashboard modules endpoint

I reviewed the wiring against the foundation in stats-app-query.ts / use-stats-app-query.ts and the proxy contract in AGENTS.md. This is clean, small, and consistent with the established pattern. A few minor suggestions below — nothing blocking.

✅ What looks good

  • Query factory correctly delegates to statsAppProxyQuery with name: 'dashboard-modules', matching the sibling query convention.
  • The POST mutation targets the jetpack-stats-dashboard/modules endpoint — per the proxy PREFIX_CONFIG table in AGENTS.md, the whole jetpack-stats-dashboard prefix is write-allowlisted and busts the read cache server-side, so the POST won't 405. 👍
  • Cache invalidation key [ 'stats-app', 'dashboard-modules' ] is a correct prefix of the query key [ 'stats-app', name, version, endpoint, method, params, body ] (statsAppProxyQuery). TanStack Query does partial prefix matching, so this invalidates every param/body variant of the read. Correct.
  • Exports wired through both hooks/index.ts and data/src/index.ts, and the export-name test was updated for both new symbols. Changelog included.

🟡 Minor suggestions (non-blocking)

  1. Duplicated endpoint identityversion: '2' and endpoint: 'jetpack-stats-dashboard/modules' are repeated in both stats-app-dashboard-modules-query.ts and use-stats-app-dashboard-modules.ts. If the endpoint/version ever changes, these can silently drift apart (the AGENTS.md note "a wrong version silently hits a different endpoint" applies here). Consider hoisting them into shared constants reused by both the query and the mutation. Fix this →

  2. Untyped mutation bodymutationFn: ( body: unknown ) accepts anything. If the modules write payload has a known shape, a typed body (and a typed response generic on fetchStatsProxy/useStatsAppQuery) would give callers compile-time safety. Acceptable as scaffolding if the shape isn't settled yet.

  3. Import-grouping comments — sibling hook files (e.g. use-stats-video-plays.ts) lead with /** Internal dependencies */ (and external where applicable) comment blocks. The new use-stats-app-dashboard-modules.ts omits them. Trivial style consistency nit.

❓ Question

The PR description mentions keeping "endpoint-owned processing, sanitizer, fixtures, and tests … where this endpoint introduces them." This endpoint is pure pass-through wiring with no processing/sanitizer and only the export-name test — that reads as intentional (raw unknown data, no normalization). Just confirming that's the intent for this endpoint and not an omission.


Note: I couldn't run pnpm --dir projects/packages/premium-analytics typecheck in this environment (command requires approval), so the type checks above are from static reading only. The wiring types line up (StatsQueryParams flows through consistently), but please rely on CI/the testing instructions to confirm.
· endpoint/stats-app-dashboard-modules-endpoint

@kangzj kangzj force-pushed the endpoint/stats-app-dashboard-modules-endpoint branch from 421139f to 8115b4f Compare June 24, 2026 08:25
@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 5s —— View job


Re-review: Stats app dashboard modules endpoint (post-rebase)

The rebase onto the clean foundation (#49886) is clean — the diff against origin/endpoint/stats-endpoint-foundation is identical to before (56 additions, no processing/sanitizer). I re-verified the wiring against the now-rebased foundation files and everything still lines up.

✅ Confirmed against the rebased foundation

  • Query factory delegates to statsAppProxyQuery with name: 'dashboard-modules', matching the sibling convention. version: '2' and method flow through the typed StatsProxyVersion/StatsProxyMethod (api/stats-proxy-fetch.ts'2' and 'POST' are both valid members), so no type drift from the rebase.
  • POST mutation targets jetpack-stats-dashboard/modules. I confirmed in src/REST/class-api-proxy-controller.php:121-123 that PREFIX_CONFIG['jetpack-stats-dashboard'] sets 'writes' => array( 'jetpack-stats-dashboard/' ) — the whole prefix is write-allowlisted and busts the read cache server-side, so the POST won't 405. The PHP test matrix (Api_Proxy_Controller_Test.php:312, 571, 641) backs this up, including that only POST (not PUT/PATCH) is allowed.
  • Cache invalidation key [ 'stats-app', 'dashboard-modules' ] is a correct prefix of the read query key [ 'stats-app', name, version, endpoint, method, params, body ], so TanStack's partial matching invalidates every param/body variant. Correct.
  • Exports wired through both hooks/index.ts and data/src/index.ts, with the export-name test updated for both new symbols. Changelog present.

🟡 Prior minor suggestions still stand (all non-blocking)

  1. Duplicated endpoint identityversion: '2' + endpoint: 'jetpack-stats-dashboard/modules' are repeated in both the query and the mutation; they can silently drift. Consider a shared constant. Fix this →
  2. Untyped mutation bodymutationFn: ( body: unknown ). If the write payload shape is known, a typed body would give callers compile-time safety. Fine as scaffolding otherwise.
  3. Import-grouping commentsuse-stats-app-dashboard-modules.ts omits the /** Internal dependencies */ block that sibling hooks use. The query file already has it. Trivial style nit.

🟢 Verdict

LGTM. The rebase preserved correctness and the endpoint matches the established pattern. The three notes above are all optional polish.

Note: I read the code statically and cross-checked the PHP proxy contract, but couldn't run pnpm typecheck/test in this environment — please rely on CI per the testing instructions.
· endpoint/stats-app-dashboard-modules-endpoint

@kangzj

kangzj commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

Addressing claude[bot]'s re-review (comment #4787361603): shared the dashboard modules query name/version/endpoint constants between the read query and mutation, and switched mutation invalidation to useQueryClient(). 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 52s —— View job


Re-review: Stats app dashboard modules endpoint (post-fixes)

  • Verify shared query identity constants between read query & mutation
  • Verify mutation switched to useQueryClient()
  • Re-check cache invalidation key & proxy contract
  • Post verdict

Thanks @kangzj — the two follow-ups from the prior re-review are both in and correct.

✅ Confirmed fixes

  1. Shared endpoint identitySTATS_APP_DASHBOARD_MODULES_NAME / _VERSION / _ENDPOINT now live in queries/stats-app-dashboard-modules-query.ts and are imported by the mutation in use-stats-app-dashboard-modules.ts. The read query and the POST mutation can no longer drift apart on version/endpoint — exactly what the "wrong version silently hits a different endpoint" note warned about. The query factory consumes the same constants, so there's a single source of truth.
  2. useQueryClient() — the mutation now resolves the client via the hook (const queryClient = useQueryClient()) inside useStatsAppDashboardModulesMutation, instead of reaching for a module-level/imported client. Correct for React context.

✅ Re-verified still-correct after the refactor

  • Invalidation key remains a valid prefix. onSuccess invalidates [ 'stats-app', STATS_APP_DASHBOARD_MODULES_NAME ][ 'stats-app', 'dashboard-modules' ]. The read key built by statsAppProxyQuery is [ 'stats-app', name, version, endpoint, method, params, body ] (stats-app-query.ts:30-38) with name = 'dashboard-modules', so the invalidation prefix-matches every param/body variant of the read. Using the shared _NAME constant for the key (rather than a hard-coded string) is a nice bonus — the key can't drift from the query name either.
  • Proxy write contract. POST jetpack-stats-dashboard/modules is still covered by the 'jetpack-stats-dashboard' write-allowlist prefix (class-api-proxy-controller.php PREFIX_CONFIG), which busts the read cache server-side, so the write won't 405.
  • Exports & test. Both new symbols remain wired through hooks/index.ts, queries/index.ts, and data/src/index.ts, with stats-exports.test.ts updated. Changelog present.

🟡 Remaining (optional, non-blocking)

  • Untyped mutation bodymutationFn: ( body: unknown ) still accepts anything. Fine as scaffolding; tighten to the real write-payload shape (plus a typed response generic on fetchStatsProxy) once it's settled.
  • Import-grouping comment — the query file leads with /** Internal dependencies */; the hook file still omits the block that some sibling hooks use. Pure style nit.

🟢 Verdict

LGTM. The query-identity sharing and useQueryClient() change are both correct and resolve the actionable feedback. Remaining items are optional polish.

Note: verified by static reading + cross-checking the PHP proxy contract; I couldn't run pnpm typecheck/test here, but you reported local typecheck passing — please continue to rely on CI per the testing instructions.
· endpoint/stats-app-dashboard-modules-endpoint

Base automatically changed from endpoint/stats-endpoint-foundation to trunk June 24, 2026 21:33
@kangzj kangzj force-pushed the endpoint/stats-app-dashboard-modules-endpoint branch 2 times, most recently from baf626c to dfd685f Compare June 24, 2026 22:43
@kangzj kangzj force-pushed the endpoint/stats-app-dashboard-modules-endpoint branch from dfd685f to b4fb591 Compare June 25, 2026 00:55
@chihsuan

Copy link
Copy Markdown
Member

Hi @kangzj Are the Stats endpoint PRs ready for review? They all appear to have conflicts with trunk. Just to confirm before starting the review process. 🙂

@kangzj

kangzj commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

@claude this is already supported on trunk, isn't it?

@claude

claude Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

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


Is the dashboard modules endpoint already on trunk?

Short answer: No — not the one this PR adds. What's on trunk is the sibling endpoint, and they're easy to mix up because the names differ by one word.

I checked origin/trunk:

On trunk? name endpoint
dashboard-module-settings ✅ yes dashboard-module-settings jetpack-stats-dashboard/module-settings
dashboard-modules (this PR) ❌ no dashboard-modules jetpack-stats-dashboard/modules

On trunk, git grep dashboard under packages/data/src/ only turns up the module-settings query/hook (stats-app-dashboard-module-settings-query.ts, use-stats-app-dashboard-module-settings.ts, plus its exports in hooks/index.ts, queries/index.ts, data/src/index.ts). There is no stats-app-dashboard-modules-query.ts / use-stats-app-dashboard-modules.ts on trunk.

The only place jetpack-stats-dashboard/modules appears on trunk is as a generic example string in test fixtures:

  • packages/data/src/api/__tests__/stats-proxy-fetch.test.ts (asserting proxy URL building)
  • tests/php/REST/Api_Proxy_Controller_Test.php (proxy write-allowlist matrix)

Neither registers a typed query/hook for it — they just exercise the proxy plumbing. So the actual client wiring for /modules (query factory + mutation hook + exports) that this PR adds is not redundant with trunk.

⚠️ But there's a real overlap to watch

This branch has no merge base with trunk (git diff origin/trunk...HEADfatal: no merge base), and a direct git diff origin/trunk HEAD shows the branch is stale: trunk has since moved on (refactored insights / streak / visits / processing, and added dashboard-module-settings), none of which this branch has. That matches @chihsuan's note about conflicts.

So while the modules endpoint itself isn't on trunk, the branch needs a rebase onto current trunk before review/merge — otherwise it would regress the newer trunk work. After rebasing, the modules files should slot in cleanly next to the existing module-settings ones (they follow the identical pattern, which is good).

TL;DR: trunk has dashboard-**module-settings**, not dashboard-**modules** — this PR's endpoint is still net-new. But please rebase onto trunk to clear the divergence before merging.
· endpoint/stats-app-dashboard-modules-endpoint

@kangzj

kangzj commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

@chihsuan thanks for taking a look - not really. I failed miserably shepherding the many PRs at the same time, and now I'm working on them one by one - a lot of conflicts because they share the same exports and lower level code 😢

@kangzj kangzj removed the request for review from a team June 25, 2026 05:09
@kangzj kangzj removed the request for review from a team June 25, 2026 05:09
@kangzj kangzj marked this pull request as draft June 25, 2026 09:01
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 [Status] Needs Team Review Obsolete. Use Needs Review instead.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants