Skip to content

Premium Analytics: add Stats endpoint foundation - #49886

Merged
kangzj merged 3 commits into
trunkfrom
endpoint/stats-endpoint-foundation
Jun 24, 2026
Merged

Premium Analytics: add Stats endpoint foundation#49886
kangzj merged 3 commits into
trunkfrom
endpoint/stats-endpoint-foundation

Conversation

@kangzj

@kangzj kangzj commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Fixes #

Proposed changes

  • Add shared Stats endpoint query helpers used by the endpoint-level PRs.
  • Add shared app-query helpers for app/admin resources.
  • Add shared processing helpers used by the endpoint normalizers.

Related product discussion/links

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

No. This adds shared client-side helpers only.

Testing instructions

  • Run pnpm --dir projects/packages/premium-analytics typecheck.
  • Run pnpm --dir projects/packages/premium-analytics build.

@kangzj
kangzj requested review from a team as code owners June 24, 2026 07:51
@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
@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

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

This comment has been minimized.

@claude

claude Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

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


PR Review: Premium Analytics — Stats endpoint foundation

  • Read PR diff against trunk
  • Review the new query/app-query/processing helpers
  • Check integration points & existing patterns
  • Post review

This is a clean, well-scoped foundation PR. The helpers mirror existing patterns (statsProxyQuery, useReport) and the changelog/types look right. Typecheck and build are the right gates given there are no runtime consumers yet. A few things worth addressing before the endpoint-level PRs build on top of these:

🟠 Inconsistent (and lossy) enabled handling between the two new hooks

useStatsQuery correctly ANDs the caller flag with the query's own flag:

// use-stats-query.ts
const enabled =
  options?.enabled === undefined
    ? queryOptions.enabled
    : options.enabled && queryOptions.enabled !== false;

But useStatsAppQuery spreads enabled last, so it overrides any enabled the query options carried:

// use-stats-app-query.ts
return useQuery( {
  ...queryOptions,
  enabled: options?.enabled ?? true,   // discards queryOptions.enabled === false
} );

If a future caller passes a query factory that sets enabled: false (e.g. "don't fetch until a date range is selected", which is exactly what statsReportQuery does today) and doesn't also pass options.enabled, the app-query hook will silently force-enable it. Since these two hooks are meant to be the shared entry points, having them disagree on enabled semantics is a footgun. Recommend making useStatsAppQuery respect queryOptions.enabled the same way useStatsQuery does. Fix this →

🟡 Type duplication: UseStatsAppOptions vs UseStatsOptions

UseStatsAppOptions = { enabled?: boolean } is identical to UseStatsOptions in use-stats-report.ts. Consider importing/reusing the existing type (as use-stats-query.ts already does) rather than redeclaring it, so they can't drift.

🟡 statsAppProxyQuery overlaps heavily with statsProxyQuery

queries/stats-app-query.ts is close to the existing statsProxyQuery in stats-query.ts. The meaningful differences are: 'stats-app' key prefix, no param transformation (statsQueryParamsToApiParams), no sanitizer, and the ?? {} key normalization. That's a legitimate split (app/admin resources are un-sanitized passthrough), but it's worth a short comment at the top of the new function explaining why it exists separately from statsProxyQuery, so the next person doesn't try to merge them or pick the wrong one.

Minor: statsAppQueryKeyPart( value ) => value ?? {} only normalizes null/undefined. React Query hashes keys deterministically, so undefined would otherwise be dropped from the serialized key — coalescing to {} is reasonable, just confirm that's the intent (vs. e.g. wanting two calls with params: undefined and params: {} to share a cache entry, which this makes them do).

🟡 reportSlugOrDisabledComparisonKey — single param doing double duty

reportSlugOrDisabledComparisonKey: string | string[],

This is backward-compatible (all current callers pass a slug string ✅) and the array-vs-string branch is clear, but the overloaded parameter + long name is a bit awkward as a public API. An options object or a separate optional disabledComparisonKey param would read better for the endpoint PRs that need the array form. Non-blocking — flagging for API ergonomics.

🟢 Looks good

  • getStatsLabel correctly guards decodeURIComponent with try/catch (malformed % sequences throw) and falls back to the raw string. 👍
  • emptyStatsReport / generics on useStatsReport are reasonable.
  • Changelog entry is present and correctly typed (minor / added).

Note: no consumers or tests yet

None of the new exports are referenced or re-exported from src/index.ts / queries/index.ts yet — expected for a "foundation" PR, but it means typecheck/build can't really exercise the enabled logic or the key-building. Given the subtle enabled semantics above, a couple of small unit tests (the two enabled truth tables + getStatsLabel decode/fallback + statsAppProxyQuery key shape) would be cheap insurance and there's already a __tests__ convention in the package to follow.
· branch endpoint/stats-endpoint-foundation

@kangzj

This comment has been minimized.

@kangzj

This comment has been minimized.

@claude

claude Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

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


Re-review: Premium Analytics — Stats endpoint foundation

  • Read the latest diff and the "Address review" commit (a5054087)
  • Re-check the previous review items
  • Post updated review

The Address review: align stats app query helpers commit cleanly resolves the items from the first pass. Walking each one:

enabled semantics now unified

The enabled resolution was extracted into a single getStatsQueryEnabled() helper in use-stats-query.ts, and both useStatsQuery and useStatsAppQuery now route through it. The previous footgun — useStatsAppQuery doing enabled: options?.enabled ?? true and silently force-enabling a query that set enabled: false — is gone. The two shared entry points now agree.

✅ Type duplication removed

UseStatsAppOptions = UseStatsOptions (imported from use-stats-report.ts) instead of a redeclared { enabled?: boolean }. They can no longer drift.

✅ Rationale comment added to statsAppProxyQuery

The new doc comment explains the separate query-key namespace ("App/admin resources use the Stats proxy transport but do not use report param coercion or response sanitizers"), so the next person won't try to merge it back into statsProxyQuery or grab the wrong one.

✅ Tests added — and they cover the subtle bits

Good targeted coverage of exactly the risk areas from the first review:

  • getStatsQueryEnabled truth tables — preserves query enabled with no override, won't let a caller force-enable a disabled query, allows a caller to disable an enabled one. This is the logic that was previously untestable.
  • getStatsLabel — decode success, malformed-% fallback, number→string, and object→''. I confirmed the implementation matches (typeof === 'string' try/catch, number/boolean via String(), else '').
  • statsAppProxyQuery key shape — asserts no report-param coercion, and that omitted vs {} params share a cache key (the statsAppQueryKeyPart( … ) => value ?? {} behavior, now intentional and pinned by a test).

🟡 Remaining (non-blocking) — reportSlugOrDisabledComparisonKey

Unchanged from last time, which is fine since it was flagged as non-blocking API ergonomics. The single string | string[] param still does double duty in useStatsReport. Backward-compatible and correct; just a bit awkward as a public signature for the endpoint PRs that need the array form. Worth revisiting (options object, or a separate optional param) if/when a second caller actually passes the array — no need to churn it now.


Verdict

All blocking and minor items from the previous review are addressed, with tests that lock in the behavior. The only open note is the cosmetic reportSlugOrDisabledComparisonKey ergonomics, which is explicitly non-blocking. LGTM for a foundation PR. 👍

Note: I wasn't able to run typecheck/build locally in this run (command approval unavailable), so I relied on static review — CI covers those gates.
· branch endpoint/stats-endpoint-foundation

@kangzj

kangzj commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

Review-cycle summary - f1a099a129 -> a5054087c2

Foundation review is clean: required CI is green and Claude's re-review is LGTM.

What changed during the cycle

Commits added:

  • a5054087c2 - Address review: align stats app query helpers

Diff summary: 6 files changed, 94 insertions(+), 11 deletions(-)

Review feedback addressed:

Source Comment Resolution
claude[bot] #4787142134 Unified enabled handling, reused UseStatsOptions, documented app query namespace, and added focused coverage.

Unaddressed human feedback: None.

CI: all required checks passing.

};
}

export function getStatsLabel( value: unknown ): string {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This function only seems to be used in tests, or is it supposed to be used elsewhere too?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is a foundation helper consumed by the endpoint-level normalizer PRs that build on this one — getStatsLabel is called from the archives, devices, and generic-list normalizers (it URL-decodes stats item labels with a safe fallback). No consumers live on this branch yet since this PR only lands the shared helpers; the tests exercise the decode/fallback behavior in the meantime.

@Nikschavan Nikschavan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you, this looks good. Added two questions

Comment on lines +8 to +10
return options?.enabled === undefined
? queryOptions.enabled
: options.enabled && queryOptions.enabled !== false;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

queryOptions.enabled can be a function predicate (TanStack Query v5 supports boolean | ((query) => boolean)), but options.enabled && queryOptions.enabled !== false always collapses to a boolean — so when a caller passes a function and options.enabled is truthy, the predicate gets silently replaced with true and the conditional gating is lost. We can keep the same "either side can disable" behavior while passing the original enabled through untouched:

Suggested change
return options?.enabled === undefined
? queryOptions.enabled
: options.enabled && queryOptions.enabled !== false;
return options?.enabled === false ? false : queryOptions.enabled;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch — fixed in f8a5765. Switched to return options?.enabled === false ? false : queryOptions.enabled; so a queryOptions.enabled function predicate is passed through untouched and only an explicit caller enabled: false forces a disable. The "either side can disable" truth table is unchanged (existing tests still pass) and I added a case pinning the predicate-preservation behavior.

queryOptions.enabled can be a TanStack predicate (boolean | (query) => boolean), but the AND-collapse turned a passed predicate into a plain true whenever the caller flag was truthy, dropping the gating. Pass queryOptions.enabled through untouched and only let an explicit caller enabled:false force a disable.
@kangzj
kangzj dismissed Nikschavan’s stale review June 24, 2026 21:29

Feedback addressed 👆

@kangzj
kangzj merged commit 5d8974a into trunk Jun 24, 2026
76 checks passed
@kangzj
kangzj deleted the endpoint/stats-endpoint-foundation branch June 24, 2026 21:33
@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.

2 participants