Conversation
apata
force-pushed
the
internal-api-refactor--typescript
branch
from
1423b3c to
f62199e
Compare
|
There was a problem hiding this comment.
visitor-graph looks a lot cleaner, and caching is definitely a good step for us. Nice work! 🙌
Everything looks good to me codewise. Noticed a couple of UI things that should be fixed before merging though. There's one comment inline about bottom margin, but there's also this:
Screen.Recording.2026-03-04.at.09.56.11.mov
Making two requests is expected in realtime -- one is for current visitors. But what's not expected is the flicker when the stats update. Hope you can see it in the recording.
| import GraphTooltip from './graph-tooltip' | ||
| import { buildDataSet, METRIC_LABELS, hasMultipleYears } from './graph-util' | ||
| import dateFormatter from './date-formatter' | ||
| import FadeIn from '../../fade-in' |
There was a problem hiding this comment.
Could we keep this? Comparing with master, the graph appearing with a tiny fade-in makes it noticeably smoother.
Or does it not play nice with the instant "cached stats render" transition?
There was a problem hiding this comment.
Thanks! The show prop was always truthy for it, so I removed it. Will try to add something like it back in.
| </div> | ||
| </FadeIn> | ||
| ) | ||
| return <canvas id="main-graph-canvas" className={canvasClass} /> |
There was a problem hiding this comment.
Not sure if I'm commenting on the correct line, but just noticed on staging that the padding below the time labels (x-axis of the graph) was removed. Let’s add back that empty space.
There was a problem hiding this comment.
Oh, nice catch! I did change around which components have margins and made sure there was no jumpiness during load, but missed this one. In this PR, the chart is 4 tailwind units taller. Will fix
| .filter((stat) => stat.graphable) | ||
| .map((stat) => stat.metric) | ||
|
|
||
| setSelectedMetric((currentlySelectedMetric) => { |
There was a problem hiding this comment.
Just curious: what's the reason for passing this kind of function? Why not just this:
if (currentlySelectedMetric && availableMetrics.includes(currentlySelectedMetric)) {
setSelectedMetric(currentlySelectedMetric)
} else {
setSelectedMetric(availableMetrics[0])
}There was a problem hiding this comment.
Good question. selectedMetric state is a reactive value. It should then become a dependency of the useEffect hook, because the hook makes use of that reactive value.
Because the effect changes it sometimes to a new value, the effect's dependencies changed, and the effect would run again immediately. On the second run, it would probably not change the value again (because the effect is idempotent: given the same dependencies, yields the same value), so there wouldn't be an infinite loop, but still inefficient.
If we'd leave it out of the dependency array (and ignore the linter), sometimes the effect might not run when it should: a subtle bug in our code. https://react.dev/learn/removing-effect-dependencies#why-is-suppressing-the-dependency-linter-so-dangerous
There was a problem hiding this comment.
Navigating around with cached frontend data feels amazing 👏 I'm also curious to see if it makes Clickhouse any happier.
Besides the comments, there are two general issues I'm seeing that I can't comment on the diff:
Caching doesn't work for relative date ranges
Date ranges like 28d don't work properly with the caching implementation. This is because the nowForSite(site) function called in DashboardState will return a timestamp with millisecond precision which is used for caching. It will never match the cache key.
Proposed fix: in DashboardContext call nowForSite(site).startOf('day') instead to clamp the timestamp properly for caching.
Dashboard rendering freeze
When clicking on top stats metrics or changing intervals, the UI freezes momentarily. This is because the line-graph's componentDidUpdate is synchronously destroying the chart and rebuilding it within the same rendering cycle. This creates a noticeable delay in the the UI giving user feedback for the click they just made (the top stat highlighting is delayed).
Proposed fix: wrap the chart rebuilding in line-graph in requestAnimationFrame so chart is rebuild after the react rendering cycle which provides feedback to user that the click was registered.
| const isRealtime = dashboardState.period === 'realtime' | ||
| const refetchTopStats = topStatsQuery.refetch | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Seems like only top stats are re-fetched in realtime view. I tested by staying on the realtime view for a few minutes and the main graph does not change after multiple ticks.
There was a problem hiding this comment.
Thanks, on it! Also, I noticed that the main-graph is sometimes requested with disallowed interval. Working on that too.
| return await fetchTopStats(site, dashboardState) | ||
| }, | ||
| placeholderData: (previousData) => previousData, | ||
| staleTime: Infinity |
There was a problem hiding this comment.
I think this might be too aggressive for when the tab is left open for a long time and the user returns. Something like 30 minutes would get most of the benefit but also make sure the data isn't too stale, what do you think?
There was a problem hiding this comment.
Thanks for pointing it out! Because refreshing throws out all stored data at once, we want the user to refresh as little as possible. That means we need to set really unaggressive cache times, as appropriate by period.
Of the periods that include the present moment, I think probably the shorter the range, the shorter the staleTime should be. For year to date, it can be hours. For month to date, an hour. Last 7 days, 15min, Today, 5min. Realtime data should be no more stale than the last updated ticker shows.
Historical ranges can have much longer cache times, let's say 10h, enough to last one working day. By historical I mean anything that isn't supposed to include the present moment.
There was a problem hiding this comment.
Something like this eab7baa#diff-6ebff7af37ed5b145ec3ca60c9db1593545938d3d71b5ed7be93a7c8d64dc7fd
No doubt it could be further improved (take into account native stats begin and comparison date range), but I hope it's good enough for starting out.
There was a problem hiding this comment.
Agree with the idea to make it dynamic based on the time range - nice touch!
3 participants
Changes
This PR refactors top stats and the main graph to cache results locally. This makes the stats and the main graph load instantly for views that the user has already visited in the session.
The results are cached until the next time the page is refreshed (normal, not hard refresh), or the page is closed and reopened.
Realtime results are updated any time that period is selected and then at regular intervals.
Other changes
Tests
Changelog
Documentation
Dark mode