Premium Analytics: add Stats traffic hooks - #49778
Conversation
Code Coverage SummaryThis 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. 🤷 |
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
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:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
e10f0b5 to
0d72822
Compare
87426da to
ab39774
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
734f111 to
b192b40
Compare
ab39774 to
92cbcd5
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Review loop clear for current head |
| @@ -14,6 +14,7 @@ export { useReportVisitors } from './hooks/use-report-visitors'; | |||
| export { useReportVisitorsByLocation } from './hooks/use-report-visitors-by-location'; | |||
| export { useReportBookings } from './hooks/use-report-bookings'; | |||
| export { useReportSessionsByDevice } from './hooks/use-report-sessions-by-device'; | |||
| export * from './hooks/use-stats'; | |||
There was a problem hiding this comment.
Not sure why we re-exports each useReport* hook individually in upstream, while hooks/index.ts is supposed to be the "all hooks" but is actually missing 6 of them. 🤔
I don't think we should fix it in this PR, but it'd be nice to follow up by making hooks/index.ts the canonical barrel (every hook re-exported there) and having src/index.ts just do export * from './hooks' instead of listing each hook. That removes the per-hook duplication and the drift risk. Filing this as a note so we can pick it up later.
There was a problem hiding this comment.
b192b40 to
3bf4050
Compare
92cbcd5 to
e759bfd
Compare
3bf4050 to
9d1cf8b
Compare
e759bfd to
d3ec815
Compare
d3ec815 to
d50f7a0
Compare
|
@claude review plz |
|
Claude finished @kangzj's task in 1m 23s —— View job Code Review: Stats traffic hooks
Nice, clean split. The per-endpoint hooks are thin and consistent, comparison handling stays in the shared What looks good
Minor suggestions (non-blocking)
Re: @chihsuan's barrel-drift note — agreed, and worth flagging concretely: Verdict: looks good to merge. 👍 I followed the documented test/typecheck/build instructions as the source of truth rather than running the full pipeline here. |
|
Addressed the concrete review suggestions in 29f645b:
Validation:
|
Fixes #
Proposed changes
useStats*hooks for the core traffic endpoints introduced by Premium Analytics: add Stats traffic queries #49777.useStatsReporthelper while leaving each endpoint hook in its own file.Related product discussion/links
Does this pull request change what data or activity we track or use?
No. This exposes React Query hooks for existing Stats query factories.
Testing instructions
pnpm --dir projects/packages/premium-analytics test --runInBand.pnpm --dir projects/packages/premium-analytics typecheck.pnpm --dir projects/packages/premium-analytics build.summarize=1, while replayed raw requests withoutsummarize=1return the expecteddate,period, anddaysbucket shapes.