feat(replay): Add support for playing mobile replays #66111
Conversation
This adds some initial support for playing mobile replays.
github-actions
bot
added
the
Scope: Frontend
billyvg
changed the title
| const projectSlug = useProjectFromId({ | ||
| project_id: replay?.getReplay().project_id, | ||
| })?.slug; |
There was a problem hiding this comment.
We need project slug here to fetch the video file
| // check if this is a video replay and if we can use the video replayer | ||
| if (isVideoReplay && videoAttachments && startTimestampMs) { | ||
| const inst = new VideoReplayer(videoAttachments, { | ||
| videoApiPrefix: `/api/0/projects/${ | ||
| organization.slug | ||
| }/${projectSlug}/replays/${replay?.getReplay().id}/videos/`, | ||
| root, | ||
| start: startTimestampMs, | ||
| onFinished: setReplayFinished, | ||
| onLoaded: event => { | ||
| forceDimensions({ | ||
| height: event.target.videoHeight, | ||
| width: event.target.videoWidth, | ||
| }); | ||
| }, | ||
| }); | ||
| // `.current` is marked as readonly, but it's safe to set the value from | ||
| // inside a `useEffect` hook. | ||
| // See: https://reactjs.org/docs/hooks-faq.html#is-there-something-like-instance-variables | ||
| // @ts-expect-error | ||
| replayerRef.current = inst; | ||
| applyInitialOffset(); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Where the magic happens... VideoReplayer should have the same interface as Replayer
| const forceDimensions = useCallback( | ||
| (dimension: Dimensions) => { | ||
| setDimensions(dimension); | ||
| }, | ||
| [setDimensions] | ||
| ); |
There was a problem hiding this comment.
This is sus because setDimensions is already the return from setState
the before code was no better
| getVideoAttachments = memoize(() => | ||
| ( | ||
| this._sortedRRWebEvents.filter( | ||
| event => event.type === EventType.Custom && event.data.tag === 'video' | ||
| ) as VideoFrameEvent[] | ||
| ).map(event => ({ | ||
| duration: event.data.payload.duration, | ||
| id: event.data.payload.segmentId, | ||
| timestamp: event.timestamp, | ||
| })) | ||
| ); |
There was a problem hiding this comment.
NIT: i'd put this kind of stuff into hydrateFrames.tsx and then endup passing in something like videoFrames next to the existing rrwebFrames array.
followup kind of thing, but it should make isVideoReplay slightly faster for web-based but long replays.
static/app/utils/replays/timer.tsx
Outdated
| for (let i = 0; i < this._callbacks.length; i++) { | ||
| if (this._time >= this._callbacks[i][0]) { | ||
| this._callbacks[i][1](); | ||
| // Remove from _callbacks | ||
| this._callbacks.splice(i, 1); | ||
| } | ||
| } |
There was a problem hiding this comment.
Do we skip callbacks in the case where two in a row should run?
ie: the array is something like: [t=1, t=2, t=3] and the current _time=2
In this case we start with array.length==3 iterate to i=0 and the callback for t=1 will run because t=1 is less than _time=2
splice reduces the array to length==2, and i is set to 1, which points to t=3. t=2 is skipped :(
Then the array
There was a problem hiding this comment.
Ah yeah, I'll clean this up and add a test.
Co-authored-by: Scott Cooper <scttcper@gmail.com>
Co-authored-by: Ryan Albrecht <ryan.albrecht@sentry.io>
| ); | ||
|
|
||
| it('returns first segment if target timestamp is before the first segment when there is only a single attachment', () => { | ||
| const segments2 = [ |
There was a problem hiding this comment.
There's a segments in the outer block :/
| attachments: VideoEvent[], | ||
| {root, start, videoApiPrefix, onFinished, onLoaded}: VideoReplayerOptions | ||
| ) { | ||
| this._attachments = attachments.filter(attachment => attachment.duration > 0); |
There was a problem hiding this comment.
The filter could go into hydrateFrames too
| protected hideVideo(index: number | undefined): void { | ||
| const video = this.getVideo(index); | ||
|
|
||
| if (!video) { | ||
| return; | ||
| } | ||
|
|
||
| video.style.display = 'none'; | ||
| } | ||
|
|
||
| protected showVideo(video: HTMLVideoElement | null): void { | ||
| if (!video) { |
There was a problem hiding this comment.
mix of undefined and null types in the return statements, and return types above. ... could cause trouble later.
| return null; | ||
| } | ||
|
|
||
| return this._videos[index]; |
There was a problem hiding this comment.
This could be .at(index), and/or suffixed with ?? null explicitly because types nulls and undefined are all over the place.
|
|
||
| protected playVideo(video: HTMLVideoElement | null): Promise<void> | undefined { | ||
| if (!video) { | ||
| return undefined; |
There was a problem hiding this comment.
still on the types thing... I'd consider returning Promise.resolve(undefined) to cleanup the method return signature here, and in all the downstream places. the idea being that everything could say Promise<undefined>, for consistentcy, and to remove the | from return types.
This adds some initial support for playing mobile replays.
Internal e2e release
Future improvements