-
Notifications
You must be signed in to change notification settings - Fork 134
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -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 () => { |
There was a problem hiding this comment.
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
this._urlBlocked = true | ||
document?.body?.classList?.add('ph-no-capture') |
There was a problem hiding this comment.
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
Size Change: -2.4 kB (-0.07%) Total Size: 3.22 MB
ℹ️ View Unchanged
|
@@ -1290,7 +1288,6 @@ export class SessionRecording { | |||
} | |||
|
|||
this._urlBlocked = false | |||
document?.body?.classList?.remove('ph-no-capture') |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
see: https://posthoghelp.zendesk.com/agent/tickets/22367
The user is using the URL pause list to entirely block domains. We wrote the feature thinking about pages within otherwise captured recordings so we don't completely stop emitting when paused. Which meant in combination with full page loads we would eventually emit an e.g. 40 minute session, with an apparent length of 0 seconds, that had an initial empty full snapshot and some network calls but nothing else
no bueno
let's not do that. we now explicitly check if the recording is paused and do not flush to the backend if so.