Refactor FE API v2 interface + current visitors bugfix#6351
Conversation
- usePaginatedQueryApi -> useSearchAndPaginateQueryApi (include addSearchFilter logic in the hook) - enforce dimensions on ReportParams type and make it a part of queryKey - define explicit StatsReportQueryKey type
The new hook also includes automatic realtime re-fetch logic.
- fixes a bug: current visitors in realtime top stats, in shared links limited to segment, are fetched from /query api which enforces segment filter -> 400 Bad Request - uses Tanstack useQuery against the old GET endpoint - The value is now also cached for 30s (CACHE_TTL_REALTIME) - Do not fetch current visitors at all when not needed (i.e. not realtime and dashboard has filters applied). - The same context value is now used by both top-stats.js and current-visitors.js
|
apata
left a comment
There was a problem hiding this comment.
I think it's going in a good direction. Maybe the useQuery wrapper could be thinner, but that's just my personal preference. I appreciate the refactor to make greater use of query-api-schema.json!
| }, [dashboardState, updateCount]) | ||
|
|
||
| if (currentVisitors !== null && dashboardState.filters.length === 0) { | ||
| if (currentVisitors !== null) { |
There was a problem hiding this comment.
question, non-blocking: why is the guard against dashboardState.filters.length === 0 not needed any more?
There was a problem hiding this comment.
I moved this into the context provider. It now only fetches current visitors if isRealTimeDashboard(dashboardState) || dashboardState.filters.length === 0. If this condition is not met, the query is not executed and CurrentVisitorsContext.Provider provides the value as null. Therefore, the additional filter length check in the CurrentVisitors component is redundant.
Forgot to mention this in the PR description but this is actually a pretty important fix. Currently on production, if there are filters applied and period != realtime, we still fetch current visitors. To see the behaviour on prod, go to https://plausible.io/plausible.io?f=is,page,/:dashboard&period=28d and monitor the network tab until a realtime tick. You can see a redundant GET /current-visitors request even though the value never gets rendered. This PR fixes that.
There was a problem hiding this comment.
Thanks for clarifying! Should it be the responsibility of current visitors context to toggle this top bar component on and off? Is data === null the best signal for that?
There was a problem hiding this comment.
IMHO completely fine to let the context decide in what situation the value is needed to be fetched. It's not only up to the context to decide whether to render it though. E.g. TopBar also takes a showCurrentVisitors prop which is false on a realtime dashboard. And top stats only renders it in realtime.
| // Added client-side in the queryFn before storing to TanStack cache. | ||
| // Needed to make sure that the time/metric labels we're constructing | ||
| // in stats reports are in sync with the dashboardState that was used | ||
| // to make that query. Otherwise, relying on current dashboardState | ||
| // while rendering previous (placeholder) data, it'd be out of sync. | ||
| export type ExtraContext = { | ||
| isRealtime: boolean | ||
| hasConversionGoalFilter: boolean | ||
| } | ||
|
|
||
| export type QueryApiResponse = { | ||
| query: QueryResultQuery | ||
| meta: QueryResultMeta | ||
| results: QueryResultRow[] | ||
| extraContext: ExtraContext | ||
| } |
There was a problem hiding this comment.
issue, non-blocking: I think useQuery has API for this so we don't actually need to hand roll it. Check the description for meta at https://tanstack.com/query/latest/docs/framework/react/reference/useQuery. I didn't try it yet locally, but I think we should
There was a problem hiding this comment.
I don't think that solves the problem with rendering previousData. As soon as queryKey changes, meta does too. In other words, it will always represent the dashboardState of the current (new) query, rather than placeholderData.
Changes
For reviewers: commit-by-commit highly recommended
Makes the API interface more solid on the frontend. Smaller improvements should be pretty self-explanatory and easy to understand when skimming through the PR commit by commit.
There were also some bigger API v2 interface design changes though.
1. Introduce
use-query-api.tsIt's a module containing two hooks:
useQueryApi(new){apiState, isRealtimeSilentUpdate})useSearchAndPaginateQueryApi(partially existing as usePaginatedQueryApi)['contains', dimensions[0], [searchString]]filter gets added to the constructed statsQuery.Both hooks now operate with a queryKey of a new unified type --
StatsReportQueryKey.2. The bug fix
To be able to switch to the new hook in top stats, the "current visitors" query had to be extracted somewhere else. We're using current visitors in two places, so it makes sense for them to be fetched in a single context. Doing that, I also discovered a bug in shared links limited to a segment, where realtime top stats fetch current visitors from the
/queryAPI, enforcing the segment filter -- 400 returned while should allow current visitors to ignore even the enforced segment filter. This PR fixes that by revertingg back to the old/current-visitorsendpoint.Tests
Changelog
Documentation
Dark mode