Skip to content

Commit 6271f85

Browse files
Kim DuganKim Dugan
authored andcommitted
replay: wait for new config before recording decision
1 parent 755c2d7 commit 6271f85

File tree

4 files changed

+74
-35
lines changed

4 files changed

+74
-35
lines changed

packages/browser/src/__tests__/extensions/replay/lazy-sessionrecording.test.ts

Lines changed: 2 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -3819,49 +3819,16 @@ describe('Lazy SessionRecording', () => {
38193819
})
38203820
})
38213821

3822-
describe('stale config retry on script loaded', () => {
3823-
const FIVE_MINUTES_IN_MS = 5 * 60 * 1000
3824-
3822+
describe('wait for fresh config before starting', () => {
38253823
beforeEach(() => {
38263824
addRRwebToWindow()
38273825
})
38283826

3829-
it('requests fresh config when persisted config is stale', () => {
3830-
posthog.persistence?.register({
3831-
[SESSION_RECORDING_REMOTE_CONFIG]: {
3832-
enabled: true,
3833-
endpoint: '/s/',
3834-
cache_timestamp: Date.now() - FIVE_MINUTES_IN_MS - 1000,
3835-
},
3836-
})
3837-
3838-
sessionRecording.startIfEnabledOrStop()
3839-
3840-
expect(mockRemoteConfigLoad).toHaveBeenCalledTimes(1)
3841-
expect(sessionRecording.started).toBe(false)
3842-
})
3843-
3844-
it('does not request fresh config more than once', () => {
3845-
posthog.persistence?.register({
3846-
[SESSION_RECORDING_REMOTE_CONFIG]: {
3847-
enabled: true,
3848-
endpoint: '/s/',
3849-
cache_timestamp: Date.now() - FIVE_MINUTES_IN_MS - 1000,
3850-
},
3851-
})
3852-
3853-
sessionRecording.startIfEnabledOrStop()
3854-
sessionRecording.startIfEnabledOrStop()
3855-
3856-
expect(mockRemoteConfigLoad).toHaveBeenCalledTimes(1)
3857-
})
3858-
3859-
it('starts recording after fresh config arrives', () => {
3827+
it('does not start recording until fresh config arrives', () => {
38603828
posthog.persistence?.register({
38613829
[SESSION_RECORDING_REMOTE_CONFIG]: {
38623830
enabled: true,
38633831
endpoint: '/s/',
3864-
cache_timestamp: Date.now() - FIVE_MINUTES_IN_MS - 1000,
38653832
},
38663833
})
38673834

packages/browser/src/__tests__/extensions/replay/sessionRecording-onRemoteConfig.test.ts

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -374,5 +374,65 @@ describe('SessionRecording', () => {
374374
expect(posthog.get_property(SESSION_RECORDING_REMOTE_CONFIG).enabled).toBe(true)
375375
expect(sessionRecording['_lazyLoadedSessionRecording']['_endpoint']).toEqual('/ses/')
376376
})
377+
378+
it('does not start recording before remote config is received', () => {
379+
// Set stale config in persistence (simulating cached/CDN config)
380+
posthog.persistence?.register({
381+
[SESSION_RECORDING_REMOTE_CONFIG]: {
382+
enabled: true,
383+
endpoint: '/s/',
384+
urlTriggers: [],
385+
},
386+
})
387+
388+
// Try to start without receiving remote config
389+
sessionRecording.startIfEnabledOrStop()
390+
391+
// Should not have started recording yet
392+
expect(loadScriptMock).not.toHaveBeenCalled()
393+
expect(sessionRecording.status).toBe('lazy_loading')
394+
395+
// Now receive remote config
396+
sessionRecording.onRemoteConfig(
397+
makeFlagsResponse({
398+
sessionRecording: { endpoint: '/s/' },
399+
})
400+
)
401+
402+
// Should now start
403+
expect(loadScriptMock).toHaveBeenCalled()
404+
expect(sessionRecording.status).toBe('active')
405+
})
406+
407+
it('discards buffer on beforeunload if remote config not yet loaded', () => {
408+
// Set persistence to simulate config exists
409+
posthog.persistence?.register({
410+
[SESSION_RECORDING_REMOTE_CONFIG]: {
411+
enabled: true,
412+
endpoint: '/s/',
413+
},
414+
})
415+
416+
// Receive config to start recording
417+
sessionRecording.onRemoteConfig(
418+
makeFlagsResponse({
419+
sessionRecording: { endpoint: '/s/', urlTriggers: [{ url: 'example.com', matching: 'regex' }] },
420+
})
421+
)
422+
423+
// Should be buffering (waiting for trigger)
424+
expect(sessionRecording.status).toBe('buffering')
425+
426+
const lazyRecorder = sessionRecording['_lazyLoadedSessionRecording']
427+
const clearBufferSpy = jest.spyOn(lazyRecorder as any, '_clearBuffer')
428+
const flushBufferSpy = jest.spyOn(lazyRecorder as any, '_flushBuffer')
429+
430+
// Trigger beforeunload
431+
window.dispatchEvent(new Event('beforeunload'))
432+
433+
// Should have cleared buffer, not flushed it
434+
expect(clearBufferSpy).toHaveBeenCalled()
435+
expect(flushBufferSpy).not.toHaveBeenCalled()
436+
})
377437
})
378438
})

packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1334,6 +1334,13 @@ export class LazyLoadedSessionRecording implements LazyLoadedSessionRecordingInt
13341334
}
13351335

13361336
private _onBeforeUnload = (): void => {
1337+
// If still buffering (waiting for remote config), discard the buffer
1338+
// Better to lose this data than record incorrectly.
1339+
if (this.status === BUFFERING) {
1340+
this._clearBuffer()
1341+
return
1342+
}
1343+
13371344
this._flushBuffer()
13381345
}
13391346

packages/browser/src/extensions/replay/session-recording.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,11 @@ export class SessionRecording {
7373
}
7474

7575
startIfEnabledOrStop(startReason?: SessionStartReason) {
76+
// Wait for fresh remote config before starting recording
77+
if (!this._receivedFlags) {
78+
return
79+
}
80+
7681
if (this._isRecordingEnabled && this._lazyLoadedSessionRecording?.isStarted) {
7782
return
7883
}

0 commit comments

Comments
 (0)