-
Notifications
You must be signed in to change notification settings - Fork 2.1k
feat(customers): Filter replay playlist by person UUID in person feed canvas #42323
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(customers): Filter replay playlist by person UUID in person feed canvas #42323
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Size Change: +30 B (0%) Total Size: 3.5 MB ℹ️ View Unchanged
|
9d62715 to
a8167bb
Compare
d5cec3e to
a8bc547
Compare
a8bc547 to
0efa5c9
Compare
a8167bb to
820a46e
Compare
820a46e to
b19fac1
Compare
0efa5c9 to
f889355
Compare
07ce3a6 to
8fda4ca
Compare
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.
Additional Comments (1)
-
frontend/src/scenes/notebooks/Nodes/NotebookNodePlaylist.tsx, line 53 (link)style:
personUUIDis used on line 39 but not included in the dependency array. WhilecanvasFiltersOverrideis static after initialization (so this may not cause observable issues), adding it would follow React best practices.
1 file reviewed, 1 comment
| () => ({ | ||
| logicKey: playerKey, | ||
| filters: universalFilters, | ||
| ...(personUUID ? { personUUID } : {}), |
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.
are we doing this elsewhere as well?
that line of code looks suspiciously familiar
i've got a dusty corner of my brain that elsewhere we do our best to use the set of distinct ids instead of the person id - although only in places where we already have the distinct ids
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.
hmm not that I know of. in this case the distinct ids are not easily available, is there a reason for us to prefer them?
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.
for looking up replay it's way faster
(but if you don't already have them, then the you stand the performance hit to get them that you're trying to avoid by using them 😅)
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.
hahaha got it! will keep this in mind, though
pauldambra
left a comment
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.
some questions
but i trust you've tested this works for your use case 💖
8fda4ca to
ba3f42d
Compare

Problem
When viewing session recordings in a notebook, the canvas filters are not being applied to the playlist. This means if we add a playlist node to person feed canvas, it is going to show recordings from all people
Changes
Added functionality to apply person filters from canvas to notebook playlists by:
notebookLogicto access canvas filter overridesgetPersonUUIDFromOverrideto extract person_id from canvas filtersHow did you test this code?
Tested by:
/session_recordings was being called withpersonUUID correctly setChangelog:
No, under feature flag