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: allow rrweb to control full snapshots #945

Closed
wants to merge 6 commits into from

Conversation

pauldambra
Copy link
Member

@pauldambra pauldambra commented Dec 28, 2023

Changes

In #847 we started taking a full snapshot every ten minutes. a common perf improvement. In long recordings we've seen seeking in replay perform poorly because we don't take full snapshots more than once in many recordings.

In #860 we reverted that because it was clashing with the full snapshots we were taking on return from idle.

It occurred to me #860 was the wrong change. If rrweb is taking snapshots on return from idle when checkoutEveryNms is set and that's what we want then we can remove our attempt to take a full snapshot on return from idle

TODO

This is tricky to test since we rely on full snapshots being taken when we return from idle (although I believe this is failing in many cases because we're trying to take the snapshot before rrweb is ready) and we rely on a full snapshot when we rotate the session id

So, we probably need an integration test for those two cases that show we capture two sessions each with a full snapshot

Checklist

Copy link

github-actions bot commented Dec 28, 2023

Size Change: -3.28 kB (0%)

Total Size: 752 kB

Filename Size Change
dist/array.full.js 176 kB -821 B (0%)
dist/array.js 118 kB -820 B (-1%)
dist/es.js 118 kB -820 B (-1%)
dist/module.js 118 kB -820 B (-1%)
ℹ️ View Unchanged
Filename Size
dist/exception-autocapture.js 12 kB
dist/recorder-v2.js 104 kB
dist/recorder.js 58.4 kB
dist/surveys.js 48.7 kB

compressed-size-action

@posthog-bot
Copy link
Collaborator

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@pauldambra
Copy link
Member Author

this isn't it... this just moves the clash of when to take the snapshot elsewhere

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants