-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(3000): Add automatic "Load more" #15863
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The infinite search is suuuuuper smooth. Also can't find anything else to complain about and can go in aside from one concern of mine: I think the f"SELECT count() FROM ({total_query})"
would be slow for projects with a lot of persons (it'd need to check all active parts?) - did you try running this in production or have better knowledge than me and know this is quick?
posthog/api/person.py
Outdated
actor_ids = [row[0] for row in raw_paginated_result] | ||
_, serialized_actors = get_people(team, actor_ids) | ||
|
||
total_query, total_params = person_query.get_query(paginate=False, filter_future_persons=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not something we need to fix now, but the persons endpoint and how we paginate here feels very bespoke. Would be great to have the right abstractions in place for a common thing like pagination.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, though here this gets a bit tricky because its ClickHouse – no ORM involved
posthog/api/person.py
Outdated
_, serialized_actors = get_people(team, actor_ids) | ||
|
||
total_query, total_params = person_query.get_query(paginate=False, filter_future_persons=True) | ||
total_query_aggregated = f"SELECT count() FROM ({total_query})" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd think this query would be slow for projects with a lot of persons (it'd need to check all active parts?) - did you try running this in production or have better knowledge than me and know this is quick?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran it for out project but indeed for some larger ones it takes a few seconds, which is not ideal… I think when we fully release this the solution will have to be we'll only load the total count once, but for now just put this behind a URL param that's only used in PostHog 3000.
Problem
A bunch of our lists are paginated server-side (like insights) or just have "Load more" (like persons), but neither was supported in the sidebar yet, and neither approach would even work smoothly in a sidebar.
Changes
Fortunately,
react-virtualized
has awesome building blocks for virtualized lists with just-in-time loading, which provide a really good experience for even the largest collections. So that's how we now handle items that are paginated/"Load more"'d in table format.