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

feat: add recording url blocklist #1500

Merged
merged 18 commits into from
Nov 12, 2024
Merged

feat: add recording url blocklist #1500

merged 18 commits into from
Nov 12, 2024

Conversation

richard-better
Copy link
Contributor

@richard-better richard-better commented Oct 28, 2024

Changes

Client for PostHog/posthog#25845
Enables pausing the recording when certain urls are visited.

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!)

Copy link

vercel bot commented Oct 28, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
posthog-js ✅ Ready (Inspect) Visit Preview Nov 11, 2024 0:56am

@posthog-bot
Copy link
Collaborator

Hey @richard-better! 👋
This pull request seems to contain no description. Please add useful context, rationale, and/or any other information that will help make sense of this change now and in the distant Mars-based future.

Copy link

github-actions bot commented Oct 28, 2024

Size Change: +11.4 kB (+0.38%)

Total Size: 3.02 MB

Filename Size Change
dist/array.full.es5.js 252 kB +1.2 kB (+0.48%)
dist/array.full.js 343 kB +1.13 kB (+0.33%)
dist/array.full.no-external.js 342 kB +1.13 kB (+0.33%)
dist/array.js 169 kB +1.13 kB (+0.67%)
dist/array.no-external.js 168 kB +1.13 kB (+0.67%)
dist/main.js 170 kB +1.13 kB (+0.67%)
dist/module.full.js 343 kB +1.13 kB (+0.33%)
dist/module.full.no-external.js 342 kB +1.13 kB (+0.33%)
dist/module.js 169 kB +1.13 kB (+0.67%)
dist/module.no-external.js 168 kB +1.13 kB (+0.67%)
ℹ️ View Unchanged
Filename Size
dist/all-external-dependencies.js 193 kB
dist/dead-clicks-autocapture.js 12.8 kB
dist/exception-autocapture.js 8.8 kB
dist/external-scripts-loader.js 2.19 kB
dist/recorder-v2.js 102 kB
dist/recorder.js 103 kB
dist/surveys-preview.js 56.7 kB
dist/surveys.js 62.1 kB
dist/tracing-headers.js 1.33 kB
dist/web-vitals.js 10.3 kB

compressed-size-action

@richard-better richard-better added the bump minor Bump minor version when this PR gets merged label Oct 28, 2024
@richard-better richard-better marked this pull request as ready for review October 28, 2024 13:44
@richard-better richard-better requested review from pauldambra and a team October 28, 2024 13:45
this._checkUrlTrigger()
this._checkTriggerConditions()

if (this.status === 'paused' && !isRecordingPausedEvent(rawEvent)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I've added this to make sure this one event can go through even if the recording is paused, but... it seems like onRRwebEmit never gets called with this event?

Copy link
Member

Choose a reason for hiding this comment

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

i ran it locally and these get called for me (in tests iirc there's no real rrweb so custom events don't work - could it be that)

Screenshot 2024-10-31 at 11 43 20

i got lots of paused events... don't know if it's because i had breakpoints set or if i'm getting a paused on every mutation?

this._tryTakeFullSnapshot()

this._scheduleFullSnapshot()
this._tryAddCustomEvent('recording resumed', { reason: 'left blocked url' })
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same deal as the paused event, this one never makes it to onRRwebEmit 🤔
@pauldambra, do you have any idea what I'm doing wrong here?

{ type: 3, data: { source: 1 } },
{ type: 3, data: { source: 2 } },
// This should be in the buffer
// { type: 5, data: { tag: 'recording paused', payload: { reason: 'url blocker' } } },
Copy link
Member

Choose a reason for hiding this comment

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

ah, yes, this'll be because mocked rrweb i bet... may well be a way we can improve the mock to pass along custom events - although feel free to not spend too much time on that

if I really care then i put tests into cypress so i'm exercising a real rrweb

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.

2024-10-31 11 43 49

this works great!

i did get a flash of the blocked page as i navigated away

i think i'm ok telling people they should combine masking and blocking since its best effort and then improve it over time

@pauldambra
Copy link
Member

really silly scope creep idea... we could add ph-no-capture to the body and remove it again as we hit/leave no-record pages 🤔

@pauldambra
Copy link
Member

further scope creep... "skipping inactivity" while we're on a blocked page is the wrong player message.
since we have the custom events 🙌 the player can be aware it's on a blocked page and change that message for 💯 🦔 points

@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.

@richard-better richard-better merged commit 8a1746e into main Nov 12, 2024
13 checks passed
@richard-better richard-better deleted the blocked-pages branch November 12, 2024 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump minor Bump minor version when this PR gets merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants