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: continue recording after reset #1135

Merged
merged 4 commits into from
Apr 12, 2024

Conversation

daibhin
Copy link
Contributor

@daibhin daibhin commented Apr 12, 2024

Changes

Addresses https://posthog.com/questions/session-recordings-not-being-resumed-after-calling-posthog-restart

Because we rely on the decide response to know whether session recording is enabled, we assume a falsey value when the persistent storage is cleared as part of the reset call.

This change sets up a listener onSessionId rotation that will re-establish the keys returned by the decide response. We need to re-register all keys otherwise things like the minimum session duration would not be available for the new session.

The sampling decision for the new session is handled separately in the _samplingSessionListener

Checklist

  • Tests for new code (see advice on the tests we use)
  • Accounted for the impact of any changes across different browsers
  • Accounted for backwards compatibility of any changes (no breaking changes in posthog-js!)

@daibhin daibhin changed the title Dn fix/continue recording after reset fix: continue recording after reset Apr 12, 2024
Copy link

github-actions bot commented Apr 12, 2024

Size Change: +600 B (0%)

Total Size: 951 kB

Filename Size Change
dist/array.full.js 226 kB +150 B (0%)
dist/array.js 128 kB +150 B (0%)
dist/es.js 128 kB +150 B (0%)
dist/module.js 128 kB +150 B (0%)
ℹ️ View Unchanged
Filename Size
dist/exception-autocapture.js 12.2 kB
dist/recorder-v2.js 105 kB
dist/recorder.js 105 kB
dist/surveys-module-previews.js 62 kB
dist/surveys.js 58.3 kB

compressed-size-action

cy.posthog().then((ph) => {
ph.reset()
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't call ensureActivitySendsSnapshots() again here because two custom events were missing: Screenshot 2024-04-12 at 12 46 32

I wasn't sure where these were coming from but assumed they may be fired after RRWeb initialises and hence won't fire a second time

Copy link
Member

Choose a reason for hiding this comment

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

Let's alter that ensure or duplicate it so we can test for any snapshots maybe?

Copy link
Member

@pauldambra pauldambra left a comment

Choose a reason for hiding this comment

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

LGTM

@daibhin daibhin added the bump patch Bump patch version when this PR gets merged label Apr 12, 2024
@daibhin daibhin merged commit 6d959bd into main Apr 12, 2024
11 checks passed
@daibhin daibhin deleted the dn-fix/continue-recording-after-reset branch April 12, 2024 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump patch Bump patch version when this PR gets merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants