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: session id should start null #878

Merged
merged 2 commits into from
Nov 8, 2023
Merged

Conversation

pauldambra
Copy link
Member

@pauldambra pauldambra commented Nov 7, 2023

Maybe the change from starting with null session id was too excitable...

With the session id starting as null we're less likely to pick up a previous session as continuing incorrectly and are more consistent taking a full snapshot when the session starts.

Tested by

  • clearing storage and logging in -> seeing a recording that shows both the pre and post login activity
  • and by picking up an old logged in session in Firefox and seeing that the new activity is in its own session

Copy link

github-actions bot commented Nov 7, 2023

Size Change: +334 B (0%)

Total Size: 719 kB

Filename Size Change
dist/array.full.js 172 kB +85 B (0%)
dist/array.js 114 kB +83 B (0%)
dist/es.js 114 kB +83 B (0%)
dist/module.js 114 kB +83 B (0%)
ℹ️ View Unchanged
Filename Size
dist/exception-autocapture.js 12 kB
dist/recorder-v2.js 95 kB
dist/recorder.js 58.3 kB
dist/surveys.js 39.6 kB

compressed-size-action

@pauldambra pauldambra added the bump patch Bump patch version when this PR gets merged label Nov 7, 2023
@pauldambra pauldambra changed the title fix: session id nulliness fix: session id should start null Nov 7, 2023
@pauldambra pauldambra marked this pull request as ready for review November 7, 2023 20:46
@pauldambra pauldambra marked this pull request as draft November 8, 2023 08:11
@pauldambra pauldambra marked this pull request as ready for review November 8, 2023 09:12
Copy link
Collaborator

@benjackwhite benjackwhite left a comment

Choose a reason for hiding this comment

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

I think this makes sense 👍

@pauldambra pauldambra merged commit 5417b82 into master Nov 8, 2023
16 checks passed
@pauldambra pauldambra deleted the fix/session-id-nulliness branch November 8, 2023 09:20
pauldambra added a commit that referenced this pull request Nov 13, 2023
I want to get to a place where the session id bug fixed in #878 would be impossible to merge.

Our session recording integration tests currently check that

when you type in a box
PostHog sends two things to capture
They don't even have to be $snapshot events let alone correct $snapshot events 🙈

This changes the integration tests to look at what was captured and make some assertions on the content.

It wouldn't protect us from that specific bug we can build on this much more easily
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