fix(replay): Fix video flickering on Safari #82769
Conversation
github-actions
bot
added
the
Scope: Frontend
Bundle ReportChanges will increase total bundle size by 5.49kB (0.02%) ⬆️. This is within the configured threshold ✅ Detailed changes
|
| @@ -3,6 +3,7 @@ import {css, type Theme} from '@emotion/react'; | |||
| // Base styles, to make the Replayer instance work | |||
| export const baseReplayerCss = css` | |||
| .replayer-wrapper { | |||
| z-index: 1000000; | |||
There was a problem hiding this comment.
Replays should be max 60 minutes and each clip is ~5 seconds so the z-index of the clips should never reach this
There was a problem hiding this comment.
can you add a comment about why/how the z-index is needed?
| // resets the soon-to-be previous video to the beginning if it's ended so it starts from the beginning on restart | ||
| if (this._currentVideo.ended) { | ||
| this.setVideoTime(this._currentVideo, 0); | ||
| for (const video of this._videos) { |
There was a problem hiding this comment.
how about destructuring to get named vars:
| for (const video of this._videos) { | |
| for (const [index, videoElem] of this._videos) { |
and then video[0] would be index and video[1] would be videoElem now
| @@ -3,6 +3,7 @@ import {css, type Theme} from '@emotion/react'; | |||
| // Base styles, to make the Replayer instance work | |||
| export const baseReplayerCss = css` | |||
| .replayer-wrapper { | |||
| z-index: 1000000; | |||
There was a problem hiding this comment.
can you add a comment about why/how the z-index is needed?
| // resets the soon-to-be previous video to the beginning if it's ended so it starts from the beginning on restart | ||
| if (this._currentVideo.ended) { | ||
| this.setVideoTime(this._currentVideo, 0); | ||
| for (const [index, videoElem] of this._videos) { |
There was a problem hiding this comment.
Why do we need to loop through all videos? Shouldn't there only be 2 videos that are relevant: the next video to show and the current/soon-to-be-previous video?
There was a problem hiding this comment.
We loop through them all since you can click on a timestamp much earlier or later in the video. It's harder to keep track of which videos need to be hidden in that case, especially if someone decided to click around different timestamps of the video. For instance, if someone's on video 5 and clicking on a timestamp that would show video 10, then we would need to hide video 4 and 5 and show video 9 and 10.
I decided to just loop through them all since adding that logic might be confusing, and when testing it with looping through everything, it didn't seem to add any visible lag or weirdness.
It works as expected! It shows the end of video 19 in that scenario |
To prevent flickering in Safari, we set the z-index of the clip depending on its index. We hide all videos except the next video and the video before that. This "fixes" the gap in the video by showing the previous video when the gap is happening.
Example replay: https://brustolin.sentry.io/replays/c817d3d7daac4440a2788fd0cbf521b0
Before:
Playthough with gaps
Screen.Recording.2025-01-02.at.5.57.26.PM.mov
Clicking to gap
After:
Playthrough is smooth
Screen.Recording.2025-01-02.at.5.58.46.PM.mov
No empty video at same timestamp
Fixes #82771