feat: add oldest sort option to session replays #30848
Conversation
There was a problem hiding this comment.
PR Summary
This PR introduces an "Oldest" sorting option for session recordings, allowing users to view sessions from the beginning of a journey.
- /frontend/src/scenes/session-recordings/playlist/SessionRecordingsPlaylistSettings.tsx: Added new "Oldest" entry and updated label logic via the getLabel helper to handle the 'older' direction.
- /frontend/src/scenes/session-recordings/playlist/sessionRecordingsPlaylistLogic.ts: Enhanced sorting logic by propagating a new 'direction' parameter for proper ordering and pagination.
- /frontend/src/types.ts: Updated type definitions with a new RecordingDirection union to support sorting by 'newer' or 'older'.
💡 (3/5) Reply to the bot's comments like "Can you suggest a fix for this @greptileai?" or ask follow-up questions!
3 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
frontend/src/scenes/session-recordings/playlist/SessionRecordingsPlaylistSettings.tsx
Outdated
Show resolved
Hide resolved
|
@pauldambra nice. Done! ;D |
|
Oh sure, gonna check those. |
|
Hey! It was actually a bug! 😰🐛 I accidentally mixed up the offset (load older) logic with the asc/desc ordering in the recordings query. So I had to make a few backend tweaks to get everything working properly. Also added a new test case for ordering, so the tests can keep protecting us from me pushing bad stuff again, haha 😅 Nice experience overall! Learned a bit more about the project through this 🎉 |
|
Hey @pauldambra, can I ask for a quick help here? this was the only way I managed to make the order by work. Putting it direcly on the parse select was giving me some errors, duplicating the direction identifier and things like that |
|
did some tweaks and fixed some tests. also added this select query stuff to the mypy baseline, tests should pass now. |
frontend/src/types.ts
Outdated
| export interface RecordingUniversalFilters { | ||
| date_from?: string | null | ||
| date_to?: string | null | ||
| duration: RecordingDurationFilter[] | ||
| filter_test_accounts?: boolean | ||
| filter_group: UniversalFiltersGroup | ||
| order?: RecordingsQuery['order'] | ||
| direction?: SortDirection |
There was a problem hiding this comment.
🤔 so this makes the API more powerful than the UI
e.g. I can sort for least active
that's not a terrible thing
but we could copy the DRF/Django approach and accept start_time and -start_time and then pick that out when we construct the query
i'm not actually sure which I prefer
There was a problem hiding this comment.
Yeah, UI can be just as powerful if we split the direction logic into a new button. Keeping order + direction felt cleaner and more type-safe to me, but I like the DRF-style too. Also not sure which I prefer
There was a problem hiding this comment.
Thinking about it, even if we add a direction button in the UI, DRF style would still work fine… Looks like a solid way to go
There was a problem hiding this comment.
yeah, we've talked a few times about wanting to have more in the API than in the UI (so folk can always self-serve)
i think here no button in the UI is fine
and we can allow start_time and -start_time (or whatevs typed from memory 😅 )
that way we can avoid a lot of the schema changes and just check as we generate the order by in the query
i'm on PTO next week but @veryayskiy can help keep things moving if we don't nail this today, really appreciate your patience with backwards and forwards here! API/schema always slightly more painful to change since we're stuck with it once released
There was a problem hiding this comment.
Oh, no worries at all. Really enjoying the back and forth. It’s been super nice contributing to PostHog! Probably won’t get to it before the weekend, but sounds like a solid plan. Enjoy the PTO and thanks for looping in @veryayskiy 🙌
e51f0b0 to
905135b
Compare
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
|
Just a quick note: been a bit swamped, but I’m getting back to this soon! |
frontend/src/scenes/session-recordings/playlist/sessionRecordingsPlaylistLogic.ts
Show resolved
Hide resolved
|
Hey @pauldambra, I'm keeping |
|
I saw the schema has just changed. Fixing the conflicts soon |
Problem
As described in #30824, there's currently no way to sort session recordings from oldest to newest. This makes it harder to watch sessions in the order they happened. This PR adds an "Oldest" sorting option to make that easier.
Changes
Screenshots
Does this work well for both Cloud and self-hosted?
Yes, it works well for both Cloud and self-hosted deployments.
How did you test this code?