-
Notifications
You must be signed in to change notification settings - Fork 192
fix: start request queue when opting in during session #2353
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
base: main
Are you sure you want to change the base?
Conversation
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.
2 files reviewed, no comments
Size Change: +530 B (+0.01%) Total Size: 5.09 MB
ℹ️ View Unchanged
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Requesting a review from @robbie-c specifically since he understands cookieless better than the rest of us |
expect(posthog.sessionRecording).toBeFalsy() | ||
}) | ||
|
||
it('should restart autocapture after opt_in_capturing in cookieless mode', 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.
I'm confused about whether the bug is related to autocapture not being restarted on opt-in (which is what this is testing), or whether it's related to the request queue not starting (which is the title of this PR).
Is there a link to a ticket somewhere?
posthog.opt_in_capturing() | ||
|
||
// Autocapture should be restarted immediately after opt-in | ||
expect(mockStartIfEnabled).toHaveBeenCalledTimes(1) |
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.
Hmm I don't think this is the right test
This is just testing an implementation detail.
Ideally we would test whether an autocapture event is sent when a simulated click happens
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 |
This might or might not be superseded by #2408. The PR description seems to say it's about the same thing, but the implementation does something different. |
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 |
Problem
When calling
posthog.opt_in_capturing()
the request queue stays paused, so events are not captured until the user reloads/loads a new page.Changes
Added
this._start_queue_if_opted_in()
in the workflow that runs when callingposthog.opt_in_capturing()
Release info Sub-libraries affected
Libraries affected
Checklist
If releasing new changes
pnpm changeset
to generate a changeset file