Skip to content
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

fix: recordings that are paused for their whole duration #1626

Merged
merged 1 commit into from
Dec 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 31 additions & 25 deletions src/__tests__/extensions/replay/sessionrecording.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ import {
} from '@rrweb/types'
import Mock = jest.Mock
import { ConsentManager } from '../../../consent'
import { waitFor } from '@testing-library/preact'
import { SimpleEventEmitter } from '../../../utils/simple-event-emitter'

// Type and source defined here designate a non-user-generated recording event
Expand Down Expand Up @@ -2312,48 +2311,55 @@ describe('SessionRecording', () => {
)
})

it('flushes buffer and includes pause event when hitting blocked URL', async () => {
it('does not flush buffer and includes pause event when hitting blocked URL', async () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that means that flushing before pausing is no longer safe.

really we should detect if we've ever flushed and then flush or not based on that but it's better to avoid this completely

// Emit some events before hitting blocked URL
_emit(createIncrementalSnapshot({ data: { source: 1 } }))
_emit(createIncrementalSnapshot({ data: { source: 2 } }))

// Simulate URL change to blocked URL
fakeNavigateTo('https://test.com/blocked')
_emit(createIncrementalSnapshot({ data: { source: 3 } }))
expect(document.body).toHaveClass('ph-no-capture')

await waitFor(() => {
// Verify the buffer was flushed with all events including pause
expect(posthog.capture).toHaveBeenCalledWith(
'$snapshot',
{
$session_id: sessionId,
$window_id: 'windowId',
$snapshot_bytes: expect.any(Number),
$snapshot_data: [
{ type: 3, data: { source: 1 } },
{ type: 3, data: { source: 2 } },
],
$lib: 'web',
$lib_version: '0.0.1',
},
expect.any(Object)
)
})
expect(posthog.capture).not.toHaveBeenCalled()

// Verify subsequent events are not captured while on blocked URL
_emit(createIncrementalSnapshot({ data: { source: 3 } }))
_emit(createIncrementalSnapshot({ data: { source: 4 } }))
expect(sessionRecording['buffer'].data).toHaveLength(0)

expect(sessionRecording['buffer'].data).toEqual([
{
data: {
source: 1,
},
type: 3,
},
{
data: {
source: 2,
},
type: 3,
},
])

// Simulate URL change to allowed URL
fakeNavigateTo('https://test.com/allowed')

// Verify recording resumes with resume event
_emit(createIncrementalSnapshot({ data: { source: 5 } }))

expect(document.body).not.toHaveClass('ph-no-capture')

expect(sessionRecording['buffer'].data).toStrictEqual([
{
data: {
source: 1,
},
type: 3,
},
{
data: {
source: 2,
},
type: 3,
},
// restarts with a snapshot
expect.objectContaining({
type: 2,
}),
Expand Down
13 changes: 5 additions & 8 deletions src/extensions/replay/sessionrecording.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1174,7 +1174,7 @@ export class SessionRecording {
const isBelowMinimumDuration =
isNumber(minimumDuration) && isPositiveSessionDuration && sessionDuration < minimumDuration

if (this.status === 'buffering' || isBelowMinimumDuration) {
if (this.status === 'buffering' || this.status === 'paused' || isBelowMinimumDuration) {
this.flushBufferTimer = setTimeout(() => {
this._flushBuffer()
}, RECORDING_BUFFER_TIMEOUT)
Expand Down Expand Up @@ -1269,17 +1269,15 @@ export class SessionRecording {
return
}

// we can't flush the buffer here since someone might be starting on a blocked page,
// and we need to be sure that we don't record that page
// so we might not get the below custom event but events will report the paused status
// which will allow debugging of sessions that start on blocked pages
this._urlBlocked = true
document?.body?.classList?.add('ph-no-capture')
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this snuck past me in the initial implemention. naughty paul
this doesn't just add belt and braces for not capturing the DOM, it stops other capture like autocapture too... no bueno


// Clear the snapshot timer since we don't want new snapshots while paused
clearInterval(this._fullSnapshotTimer)

// Running this in a timeout to ensure we can
setTimeout(() => {
this._flushBuffer()
}, 100)

logger.info('recording paused due to URL blocker')
this._tryAddCustomEvent('recording paused', { reason: 'url blocker' })
}
Expand All @@ -1290,7 +1288,6 @@ export class SessionRecording {
}

this._urlBlocked = false
document?.body?.classList?.remove('ph-no-capture')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the other changes make sense, this one does not feel related to the original issue?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, it's totally fly-by, commented on above. i don't think we should be doing that


this._tryTakeFullSnapshot()
this._scheduleFullSnapshot()
Expand Down
Loading