-
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: no mixing the streams #29831
base: master
Are you sure you want to change the base?
feat: no mixing the streams #29831
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.
PR Summary
This PR removes the 'SAVED_NOT_PINNED' feature flag and modifies session recording playlists to prevent mixing saved filters and pinned recordings for playlists created after March 11, 2025, aiming to reduce user confusion.
- Added
canMixFiltersAndPinned
prop infrontend/src/scenes/session-recordings/playlist/SessionRecordingsPlaylist.tsx
to control filter/pin mixing - Removed conditional 'saved vs pinned' terminology across components, defaulting to consistent 'Pin/Unpin' language
- Added creation date check in
frontend/src/scenes/session-recordings/playlist/SessionRecordingsPlaylistScene.tsx
to enforce new behavior only for playlists after March 11, 2025 - Modified filter actions visibility in
Playlist.tsx
to hide filters when pinned recordings exist in non-legacy playlists
6 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
frontend/src/scenes/session-recordings/playlist/SessionRecordingsPlaylist.tsx
Show resolved
Hide resolved
frontend/src/scenes/session-recordings/playlist/SessionRecordingsPlaylist.tsx
Show resolved
Hide resolved
Size Change: -56 B (0%) Total Size: 9.93 MB ℹ️ View Unchanged
|
📸 UI snapshots have been updated4 snapshot changes in total. 0 added, 4 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated5 snapshot changes in total. 0 added, 5 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
const tooltip = logicProps.pinned ? 'Unpin from this list' : 'Pin to this list' | ||
const description = 'Pin' |
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.
just the removal of an old flag here
return ( | ||
<Tooltip placement="top-end" title={<>This recording is {description} to this list.</>}> | ||
<Tooltip placement="top-end" title={<>This recording is pinned to this list.</>}> |
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.
just removal of old flag
📸 UI snapshots have been updated4 snapshot changes in total. 0 added, 4 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated4 snapshot changes in total. 0 added, 4 modified, 0 deleted:
Triggered by this commit. |
OK, after that last user interview I think we've more than enough evidence that even if some people are mixing saved filters and collections that most people are confused by it
let's pick a date not allow this for new playlists
we can do the more sweeping changes later, but we need to stop teaching new users that it's a thing