Skip to content

Premium Analytics: add Stats app dashboard module settings endpoint - #49888

Merged
kangzj merged 4 commits into
trunkfrom
endpoint/stats-app-dashboard-module-settings-endpoint
Jun 24, 2026
Merged

Premium Analytics: add Stats app dashboard module settings endpoint#49888
kangzj merged 4 commits into
trunkfrom
endpoint/stats-app-dashboard-module-settings-endpoint

Conversation

@kangzj

@kangzj kangzj commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Fixes #

Proposed changes

  • Add the Stats app-dashboard-module-settings 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: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 1m 26s —— View job


Code Review: Stats app dashboard module settings endpoint

  • Read PR diff vs origin/endpoint/stats-endpoint-foundation
  • Review query factory, hook, and mutation against package conventions
  • Check exports wiring, test, and changelog

This is a clean, small wiring PR that follows the foundation's patterns well. The query/hook/export plumbing matches the surrounding endpoints, the changelog entry is present, and the exports test is updated for both new hooks. A few observations, none blocking:

1. Query-key drift between the query and its invalidation (maintainability nit)

use-stats-app-dashboard-module-settings.ts:23-25 invalidates with:

queryKey: [ 'stats-app', 'dashboard-module-settings' ]

The query factory produces a key of [ 'stats-app', name, version, endpoint, method, params, body ] (stats-app-query.ts:28-36), so the invalidation works today via TanStack's prefix matching — ['stats-app', 'dashboard-module-settings'] is a prefix of the full read key. That's correct, but the 'dashboard-module-settings' string is duplicated in two files. If the name in stats-app-dashboard-module-settings-query.ts:9 is ever changed, invalidation silently stops matching. Consider exporting the name (or a queryKeyPrefix) from the query module and reusing it in the mutation so the two can't drift. Fix this →

2. Mutation body typed as unknown

use-stats-app-dashboard-module-settings.ts:16 types the mutation input as ( body: unknown ). That compiles, but callers get no shape checking on the settings payload. If the module-settings POST body has a known shape, a typed param (e.g. a ModuleSettingsPayload) would be more useful at call sites. Acceptable to defer if this endpoint is intentionally kept untyped at the foundation layer like the read side (TData = unknown).

3. No onError on the mutation

Only onSuccess (cache invalidation) is handled. The package has a globalErrorManager/useGlobalError (providers/index.ts:5) — worth confirming whether mutation errors are expected to surface through that or are left entirely to the call site. Not necessarily a change for this PR, just flagging the convention.

Things that look correct

  • POST to jetpack-stats-dashboard/module-settings is allowed — per the package AGENTS.md, the whole jetpack-stats-dashboard prefix permits writes (and busts the read cache), so the mutation won't hit 405 rest_read_only.
  • version: '2' is consistent between the query and the mutation, so both resolve to the same WPCOM v2 base.
  • Exports are wired in both hooks/index.ts and the package index.ts, and the satisfies ReadonlyArray<keyof typeof dataPackage> constraint in stats-exports.test.ts will keep the test honest.

Verdict: Looks good to merge. The query-key duplication (#1) is the only item I'd genuinely consider tightening; the rest are optional/foundation-level decisions.

· endpoint/stats-app-dashboard-module-settings-endpoint

@kangzj
kangzj force-pushed the endpoint/stats-app-dashboard-module-settings-endpoint branch from 7397898 to 2984f9c 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 41s —— View job


Re-review: Stats app dashboard module settings endpoint

  • Re-fetch and diff vs rebased origin/endpoint/stats-endpoint-foundation
  • Confirm query/hook/mutation/export wiring against foundation patterns
  • Re-check the query-key observation from the prior review

Rebase looks clean — the diff against the now-clean foundation (#49886) is identical to what I reviewed before (58 additions, 7 files, no drift introduced by the rebase). The query factory, hook, mutation, and export wiring still match the surrounding endpoints, and the exports test is updated for both new hooks.

Carry-over note (optional, non-blocking)

The one item still worth tightening is the duplicated 'dashboard-module-settings' string:

  • Read key (stats-app-dashboard-module-settings-query.ts:9) → name: 'dashboard-module-settings', which the factory expands to [ 'stats-app', name, version, endpoint, method, params, body ] (stats-app-query.ts:28-36).
  • Invalidation (use-stats-app-dashboard-module-settings.ts:23-25) → queryKey: [ 'stats-app', 'dashboard-module-settings' ].

This works today via TanStack's prefix matching (['stats-app','dashboard-module-settings'] is a prefix of the full key), but the name lives in two files. If name is ever renamed, invalidation silently stops matching. Exporting the name as a shared constant and reusing it in both places would make that impossible. Fix this →

The other two prior notes remain foundation-level decisions, fine to defer:

  • Mutation body typed as unknown (use-stats-app-dashboard-module-settings.ts:16) — matches the read side's untyped TData = unknown.
  • No onError on the mutation — confirm whether errors are meant to surface via the call site vs. globalErrorManager.

Looks correct

  • version: '2' is consistent between query and mutation.
  • POST to jetpack-stats-dashboard/module-settings is write-allowed under the jetpack-stats-dashboard prefix, so no 405 rest_read_only.
  • Exports wired in both hooks/index.ts and the package index.ts; the satisfies ReadonlyArray<keyof typeof dataPackage> constraint keeps stats-exports.test.ts honest.

Verdict: Still looks good to merge. Rebase introduced no regressions; the query-key dedup (#1) is the only item I'd consider tightening, and it's optional.
· endpoint/stats-app-dashboard-module-settings-endpoint

@kangzj

kangzj commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

Addressing claude[bot]'s re-review (comment #4787361272): shared the dashboard module-settings query name/version/endpoint constants between read and mutation code, 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 56s —— View job


Re-review: Stats app dashboard module settings endpoint

  • Diff vs origin/endpoint/stats-endpoint-foundation (69 additions, 7 files)
  • Verify the query-key dedup fix from the prior review
  • Confirm query/hook/mutation/export wiring still matches foundation patterns

The carry-over item from the last review is fully resolved.

✅ Query-key drift fixed

stats-app-dashboard-module-settings-query.ts:7-9 now exports shared constants:

export const STATS_APP_DASHBOARD_MODULE_SETTINGS_NAME = 'dashboard-module-settings';
export const STATS_APP_DASHBOARD_MODULE_SETTINGS_VERSION = '2';
export const STATS_APP_DASHBOARD_MODULE_SETTINGS_ENDPOINT = 'jetpack-stats-dashboard/module-settings';

The mutation (use-stats-app-dashboard-module-settings.ts) now imports and reuses all three — VERSION/ENDPOINT in the fetchStatsProxy call and NAME in the invalidation key (['stats-app', STATS_APP_DASHBOARD_MODULE_SETTINGS_NAME]). The string can no longer drift between the read query and the invalidation, since both resolve from the same source.

I confirmed the prefix-match still holds: the factory builds ['stats-app', name, version, endpoint, method, params, body] (stats-app-query.ts:30-38), so ['stats-app', NAME] remains a valid prefix and invalidation continues to match the read key.

Also good: the mutation now uses useQueryClient() rather than a module-level client, which is the correct React Query pattern (respects the provider tree, plays well with tests/SSR).

Remaining notes (optional, foundation-level — fine to defer)

  • Mutation body typed as unknown (use-stats-app-dashboard-module-settings.ts:16) — mirrors the read side's untyped TData = unknown. Consistent with the foundation; can be tightened later if the settings payload gets a known shape.
  • No onError — only onSuccess (invalidation) is handled. Confirm whether mutation errors are meant to surface via the call site or globalErrorManager. Not a change for this PR.

Looks correct

  • POST to jetpack-stats-dashboard/module-settings is write-allowed (whole prefix permits writes per the package AGENTS.md) — no 405 rest_read_only.
  • version: '2' consistent between query and mutation → same WPCOM v2 base.
  • Exports wired in both hooks/index.ts and the package index.ts; query exported in queries/index.ts; the satisfies ReadonlyArray<keyof typeof dataPackage> constraint in stats-exports.test.ts keeps the export test honest for both new hooks.
  • Changelog entry present (patch / added).

Verdict: :shipit: Looks good to merge. The one item I flagged previously is resolved cleanly, and nothing new was introduced.
· endpoint/stats-app-dashboard-module-settings-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-module-settings-endpoint branch from 2323ba8 to 1ee00c8 Compare June 24, 2026 21:44
@kangzj
kangzj force-pushed the endpoint/stats-app-dashboard-module-settings-endpoint branch from 1ee00c8 to 3a8e313 Compare June 24, 2026 22:43
@kangzj
kangzj merged commit 189c92f into trunk Jun 24, 2026
71 of 73 checks passed
@kangzj
kangzj deleted the endpoint/stats-app-dashboard-module-settings-endpoint branch June 24, 2026 23:32
@github-actions github-actions Bot removed the [Status] Needs Team Review Obsolete. Use Needs Review instead. label Jun 25, 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