Skip to content

Premium Analytics: add Stats app notices endpoint - #49890

Merged
kangzj merged 1 commit into
trunkfrom
endpoint/stats-app-notices-endpoint
Jul 1, 2026
Merged

Premium Analytics: add Stats app notices endpoint#49890
kangzj merged 1 commit into
trunkfrom
endpoint/stats-app-notices-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 notices 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

@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 8s —— View job


PR Review: Stats app notices endpoint

  • Gather context (diff, foundation files, conventions)
  • Analyze query/hook/mutation wiring
  • Check against package conventions (AGENTS.md)
  • Post review

Reviewed against the foundation branch (origin/endpoint/stats-endpoint-foundation). Overall this is a clean, well-scoped addition that correctly follows the package's architecture. The decision to bypass the WPCOM proxy and hit the local /jetpack-premium-analytics/v1/notices route is correct and matches AGENTS.md ("Not proxied because GET merges WPCOM dismissal state with local flags"), and the inline comment documents the why nicely. ✅

A few suggestions, all minor/non-blocking:

1. Mutation uses the imported queryClient singleton instead of useQueryClient()

use-stats-app-notices.ts:21

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

This works today because AnalyticsQueryClientProvider shares the same exported singleton. But the idiomatic react-query pattern inside a hook is to pull the client from context via useQueryClient(), which keeps the hook correct if it's ever mounted under a different client (e.g. an isolated QueryClient in a unit test). Since this is the package's first mutation, it's worth setting the convention here.

import { useMutation, useQueryClient } from '@tanstack/react-query';

export function useStatsAppNoticeMutation() {
	const queryClient = useQueryClient();
	return useMutation( {
		mutationFn: ( data: StatsAppNoticeMutationParams ) => updateStatsAppNotice( data ),
		onSuccess: () => {
			queryClient.invalidateQueries( { queryKey: [ 'stats-app', 'notices' ] } );
		},
	} );
}

(The invalidateQueries prefix match itself is correct — [ 'stats-app', 'notices' ] partial-matches the full key [ 'stats-app', 'notices', {…} ] by default.)

Fix this →

2. statsAppNoticesQuery hardcodes UseQueryOptions< unknown >

stats-app-notices-query.ts:10

The foundation's statsAppProxyQuery< TData = unknown > is generic so callers can type the response. Making this query generic too (<TData = unknown>) would let useStatsAppNotices flow a real notices type through later without changing the signature, and keeps it consistent with the rest of the family. Non-blocking — fine to defer until the notices response shape lands.

3. StatsQueryParams is broad for a notices GET

stats-app-notices-query.ts:9 / use-stats-app-notices.ts:12

StatsQueryParams carries period/end_date/days/etc., which a notices listing endpoint likely doesn't accept. Reusing the shared type is harmless (extra params are just dropped server-side), but a narrower param type would document the actual contract better. Worth a quick check on what the /notices GET actually consumes.

4. Mutation params are loosely typed

stats-app-notices-query.ts:21-25

export type StatsAppNoticeMutationParams = {
	id: string;
	status: string;
	postponed_for?: number;
};

If status has a known finite set of values (e.g. 'dismissed' | 'postponed'), a string-literal union would catch typos at the call site. Optional polish.

Notes

  • Exports/test wiring look consistent: stats-app-notices-queryqueries/index.tshooks/index.tsdata/src/index.ts, and both hooks are added to stats-exports.test.ts. ✅
  • The export-presence test only asserts the hooks are functions — there's no test asserting the GET path/query-arg building or the POST method. That matches the rest of the family (no per-query path tests), so it's consistent, but a small test for the path construction wouldn't hurt given this endpoint is special-cased off the proxy.
  • Changelog entry is present and correctly formatted (Significance: patch, Type: added). ✅

Nothing here is blocking — the core wiring is correct. The useQueryClient() change (#1) is the one I'd most recommend before this becomes the template for future mutations.
· endpoint/stats-app-notices-endpoint

@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 force-pushed the endpoint/stats-app-notices-endpoint branch from aec8889 to 1c32960 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 18s —— View job


PR Re-Review: Stats app notices endpoint

  • Gather context (diff vs rebased foundation)
  • Re-check query/hook/mutation wiring
  • Verify previous feedback status
  • Post review

Re-reviewed against the rebased base (origin/endpoint/stats-endpoint-foundation). The rebase is clean — the diff is content-identical to the prior review (7 files, +77/−0), now correctly stacked on the clean foundation rather than the old layered stack. Wiring still verified end to end:

  • stats-app-notices-queryqueries/index.ts
  • use-stats-app-noticeshooks/index.tsdata/src/index.ts
  • both hooks in stats-exports.test.ts
  • changelog present and well-formed (Significance: patch, Type: added) ✅

The local-route decision is still correct and matches AGENTS.md/notices is deliberately not proxied because the GET merges WPCOM dismissal state with local flags. The invalidateQueries({ queryKey: [ 'stats-app', 'notices' ] }) prefix correctly partial-matches the full key [ 'stats-app', 'notices', {…} ]. ✅

Status of prior feedback (all still open, all non-blocking)

None of the four earlier suggestions were addressed in the rebase — relisting with updated notes:

1. Mutation uses the queryClient singleton instead of useQueryClient()use-stats-app-notices.ts:21

Worth noting: there is existing precedent for importing the singleton — prefetch/prefetch-report.ts does the same. But that's a plain module function, not a hook. Inside a hook the idiomatic react-query pattern is useQueryClient(), which keeps the hook correct under an isolated client (e.g. a unit-test QueryClient). Since this is the package's first mutation it sets the template, so I'd still nudge toward the context-based client here.

import { useMutation, useQueryClient } from '@tanstack/react-query';

export function useStatsAppNoticeMutation() {
	const queryClient = useQueryClient();
	return useMutation( {
		mutationFn: ( data: StatsAppNoticeMutationParams ) => updateStatsAppNotice( data ),
		onSuccess: () => {
			queryClient.invalidateQueries( { queryKey: [ 'stats-app', 'notices' ] } );
		},
	} );
}

Fix this →

2. statsAppNoticesQuery hardcodes UseQueryOptions< unknown >stats-app-notices-query.ts:10

The foundation's statsAppProxyQuery< TData = unknown > is generic (confirmed at stats-app-query.ts:21). Making this query < TData = unknown > too lets a real notices type flow through useStatsAppNotices later without a signature change. Fine to defer until the response shape lands.

3. StatsQueryParams is broad for a notices GETstats-app-notices-query.ts:9 / use-stats-app-notices.ts:12

StatsQueryParams carries period/end_date/days/etc. that the /notices GET likely doesn't accept. Harmless (extra params are dropped server-side), but a narrower param type would document the real contract.

4. Mutation params loosely typedstats-app-notices-query.ts:21-25

If status has a finite value set (e.g. 'dismissed' | 'postponed'), a string-literal union would catch typos at call sites. Optional polish.

Verdict

Nothing blocking — the rebase is clean and the wiring is correct. The useQueryClient() change (#1) remains the one I'd most recommend before this becomes the mutation template, but it's a judgment call you can reasonably defer.
· endpoint/stats-app-notices-endpoint

@kangzj

kangzj commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

Addressing claude[bot]'s re-review (comment #4787361853): switched the notice 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 42s —— View job


PR Re-Review: Stats app notices endpoint


Re-reviewed against origin/endpoint/stats-endpoint-foundation. Diff unchanged in shape (7 files, +78/−0).

✅ Prior feedback #1 is resolved

use-stats-app-notices.ts:15-24 — the mutation now pulls the client from context instead of the singleton:

export function useStatsAppNoticeMutation() {
	const queryClient = useQueryClient();
	return useMutation( {
		mutationFn: ( data: StatsAppNoticeMutationParams ) => updateStatsAppNotice( data ),
		onSuccess: () => {
			queryClient.invalidateQueries( { queryKey: [ 'stats-app', 'notices' ] } );
		},
	} );
}

This is exactly the idiomatic react-query pattern and sets a good template for future mutations in the package. The invalidateQueries prefix [ 'stats-app', 'notices' ] still correctly partial-matches the full key [ 'stats-app', 'notices', statsAppQueryKeyPart(params) ]. 👍

Wiring re-verified

  • statsAppQueryKeyPart / useStatsAppQuery / UseStatsAppOptions all resolve against the foundation (stats-app-query.ts:8, use-stats-app-query.ts:5-7). ✅
  • Export chain intact: stats-app-notices-queryqueries/index.tshooks/index.tsdata/src/index.ts; both hooks present in stats-exports.test.ts. ✅
  • Local /jetpack-premium-analytics/v1/notices route (not proxied) still matches AGENTS.md rationale, with the // Notices are served by the local plugin REST route comment documenting the why. ✅
  • Changelog present and well-formed. ✅

Remaining items (all non-blocking, fine to defer)

  • Allow plugins to inject additional template-specific open graph tags #2 Generic query typestatsAppNoticesQuery still hardcodes UseQueryOptions< unknown >; the foundation's statsAppProxyQuery< TData = unknown > (stats-app-query.ts:21) is generic. Making this < TData = unknown > later lets a real notices type flow through without a signature change. Fine to defer until the response shape lands.
  • Edit and rename the readme for GitHub #3 Broad param typeStatsQueryParams carries period/end_date/days the /notices GET likely ignores. Harmless (extra args dropped server-side), but a narrower type would document the real contract.
  • Merge the changes in 2.7.1 upstream to master. #4 Loosely typed status — if status has a finite set (e.g. 'dismissed' | 'postponed'), a string-literal union would catch call-site typos. Optional polish.

Verdict

The one change I'd most recommended (#1) is done and done cleanly. Nothing blocking remains — the rest are optional refinements that can reasonably wait for the notices response/request contract to firm up. LGTM. ✅
· endpoint/stats-app-notices-endpoint

Base automatically changed from endpoint/stats-endpoint-foundation to trunk June 24, 2026 21:33
@kangzj
kangzj force-pushed the endpoint/stats-app-notices-endpoint branch 3 times, most recently from 217f371 to 88e6a47 Compare June 25, 2026 00:55
@kangzj
kangzj marked this pull request as draft June 25, 2026 09:01
@kangzj
kangzj force-pushed the endpoint/stats-app-notices-endpoint branch from 88e6a47 to cd99f8b Compare June 30, 2026 22:29
@kangzj
kangzj marked this pull request as ready for review June 30, 2026 22:47
@kangzj
kangzj merged commit 738f5f1 into trunk Jul 1, 2026
73 checks passed
@kangzj
kangzj deleted the endpoint/stats-app-notices-endpoint branch July 1, 2026 00:05
@github-actions github-actions Bot removed the [Status] Needs Team Review Obsolete. Use Needs Review instead. label Jul 1, 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