Skip to content

Commit

Permalink
fixes and add tests
Browse files Browse the repository at this point in the history
  • Loading branch information
@michellewzhang
michellewzhang committed Apr 17, 2024
1 parent 3db9c6a commit be69d5f
Show file tree
Hide file tree
Showing 3 changed files with 142 additions and 19 deletions.
2 changes: 1 addition & 1 deletion static/app/components/replays/replayContext.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -495,7 +495,7 @@ function ProviderNonMemo({
setVideoBuffering(buffering);
},
clipWindow,
durationMs: durationMs,
durationMs,
});
// `.current` is marked as readonly, but it's safe to set the value from
// inside a `useEffect` hook.
Expand Down
130 changes: 121 additions & 9 deletions static/app/components/replays/videoReplayer.spec.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ describe('VideoReplayer - no starting gap', () => {
onFinished: jest.fn(),
onLoaded: jest.fn(),
onBuffer: jest.fn(),
durationMs: 40,
durationMs: 40000,
});
// @ts-expect-error private
expect(inst._currentIndex).toEqual(0);
Expand All @@ -113,7 +113,7 @@ describe('VideoReplayer - no starting gap', () => {
onFinished: jest.fn(),
onLoaded: jest.fn(),
onBuffer: jest.fn(),
durationMs: 40,
durationMs: 40000,
});
const playPromise = inst.play(18100);
// @ts-expect-error private
Expand Down Expand Up @@ -142,7 +142,7 @@ describe('VideoReplayer - no starting gap', () => {
onFinished: jest.fn(),
onLoaded: jest.fn(),
onBuffer: jest.fn(),
durationMs: 40,
durationMs: 40000,
});
const playPromise = inst.play(50000);
// 15000 -> 20000 is a gap, so player should start playing @ index 3, from
Expand All @@ -165,7 +165,7 @@ describe('VideoReplayer - no starting gap', () => {
onFinished: jest.fn(),
onLoaded: jest.fn(),
onBuffer: jest.fn(),
durationMs: 40,
durationMs: 40000,
});
const playPromise = inst.play(0);
jest.advanceTimersByTime(2500);
Expand All @@ -185,7 +185,7 @@ describe('VideoReplayer - no starting gap', () => {
onFinished: jest.fn(),
onLoaded: jest.fn(),
onBuffer: jest.fn(),
durationMs: 50,
durationMs: 50000,
});
// play at segment 7
const playPromise = inst.play(45_003);
Expand Down Expand Up @@ -220,7 +220,7 @@ describe('VideoReplayer - no starting gap', () => {
onFinished: jest.fn(),
onLoaded: jest.fn(),
onBuffer: jest.fn(),
durationMs: 60,
durationMs: 55000,
});
// play at segment 7
const playPromise = inst.play(45_003);
Expand Down Expand Up @@ -294,7 +294,7 @@ describe('VideoReplayer - with starting gap', () => {
onFinished: jest.fn(),
onLoaded: jest.fn(),
onBuffer: jest.fn(),
durationMs: 40,
durationMs: 40000,
});
// @ts-expect-error private
expect(inst._currentIndex).toEqual(0);
Expand All @@ -318,7 +318,7 @@ describe('VideoReplayer - with starting gap', () => {
onFinished: jest.fn(),
onLoaded: jest.fn(),
onBuffer: jest.fn(),
durationMs: 40,
durationMs: 40000,
});
const playPromise = inst.play(18100);
// @ts-expect-error private
Expand Down Expand Up @@ -347,7 +347,7 @@ describe('VideoReplayer - with starting gap', () => {
onFinished: jest.fn(),
onLoaded: jest.fn(),
onBuffer: jest.fn(),
durationMs: 40,
durationMs: 40000,
});
const playPromise = inst.play(50000);
// 15000 -> 20000 is a gap, so player should start playing @ index 3, from
Expand All @@ -361,3 +361,115 @@ describe('VideoReplayer - with starting gap', () => {
expect(inst.getVideo(inst._currentIndex)?.currentTime).toEqual(5);
});
});

describe('VideoReplayer - with ending gap', () => {
beforeEach(() => {
jest.clearAllTimers();
});

const attachments = [
{
id: 0,
timestamp: 2500,
duration: 5000,
},
// no gap
{
id: 1,
timestamp: 5000,
duration: 5000,
},
{
id: 2,
timestamp: 10_001,
duration: 5000,
},
// 5 second gap
{
id: 3,
timestamp: 20_000,
duration: 5000,
},
// 5 second gap
{
id: 4,
timestamp: 30_000,
duration: 5000,
},
{
id: 5,
timestamp: 35_002,
duration: 5000,
},
];

it('keeps playing until the end if there is an ending gap', async () => {
const root = document.createElement('div');
const inst = new VideoReplayer(attachments, {
videoApiPrefix: '/foo/',
root,
start: 0,
onFinished: jest.fn(),
onLoaded: jest.fn(),
onBuffer: jest.fn(),
durationMs: 50000,
});
// actual length of the segments is 40s
// 10s gap at the end

// play at the last segment
const playPromise = inst.play(36000);
await playPromise;
jest.advanceTimersByTime(4000);

// we're still within the last segment (5)
// @ts-expect-error private
expect(inst._currentIndex).toEqual(5);
expect(inst.getCurrentTime()).toEqual(40000);

// now we are in the gap
// timer should still be going since the duration is 50s
jest.advanceTimersByTime(5000);
// @ts-expect-error private
expect(inst._isPlaying).toEqual(true);

// a long time passes
// ensure the timer stops at the end duration (50s)
jest.advanceTimersByTime(50000);
expect(inst.getCurrentTime()).toEqual(50000);
// @ts-expect-error private
expect(inst._isPlaying).toEqual(false);
});

it('ends at the proper time if seeking into a gap', async () => {
const root = document.createElement('div');
const inst = new VideoReplayer(attachments, {
videoApiPrefix: '/foo/',
root,
start: 0,
onFinished: jest.fn(),
onLoaded: jest.fn(),
onBuffer: jest.fn(),
durationMs: 50000,
});
// actual length of the segments is 40s
// 10s gap at the end

// play at the gap
const playPromise = inst.play(40002);
await playPromise;
jest.advanceTimersByTime(4000);

// we should be still playing in the gap
expect(inst.getCurrentTime()).toEqual(44002);
// @ts-expect-error private
expect(inst._isPlaying).toEqual(true);

// a long time passes
// ensure the timer stops at the end duration (50s)
jest.advanceTimersByTime(50000);
expect(inst.getCurrentTime()).toBeLessThan(50100);
// @ts-expect-error private
expect(inst._isPlaying).toEqual(false);
});
});
29 changes: 20 additions & 9 deletions static/app/components/replays/videoReplayer.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,11 @@ export class VideoReplayer {
this.stopReplay();
});
} else {
// Otherwise, if there's no clip window set, we should
// Tell the timer to stop at the replay end
this._timer.addNotificationAtTime(this._durationMs, () => {
this.stopReplay();
});
// If there's no clip window set, we should
// load the first segment by default so that users are not staring at a
// blank replay. This initially caused some issues
// (https://github.com/getsentry/sentry/pull/67911), but the problem was
Expand Down Expand Up @@ -251,12 +255,16 @@ export class VideoReplayer {
this._isPlaying = true;
this._timer.start(videoOffsetMs);

// This is used when a replay clip is restarted
// This is used when a replay is restarted
// Add another stop notification so the timer doesn't run over
if (this._clipDuration) {
this._timer.addNotificationAtTime(this._clipDuration, () => {
this.stopReplay();
});
} else {
this._timer.addNotificationAtTime(this._durationMs, () => {
this.stopReplay();
});
}
}

Expand Down Expand Up @@ -301,13 +309,16 @@ export class VideoReplayer {

// No more segments
if (nextIndex >= this._attachments.length) {
// If we're at the end of a segment, but there's a gap
// at the end, force the replay to play until the end duration
// rather than stopping right away.
this._timer.addNotificationAtTime(this._durationMs, () => {
this.stopReplay();
});
return;
if (this.getCurrentTime() < this._durationMs) {
// If we're at the end of a segment, but there's a gap
// at the end, force the replay to play until the end duration
// rather than stopping right away.
this._timer.addNotificationAtTime(this._durationMs, () => {
this.stopReplay();
});
return;
}
this.stopReplay();
}

// Final check in case replay was stopped immediately after a video
Expand Down

0 comments on commit be69d5f

Please sign in to comment.