Repository navigation
fix(react-native): stop session replay on opt-out and restart it on opt-in - #5124
nandinitiw wants to merge 11 commits into
Conversation
…pt-in optOut() only persisted the consent flag, so a recording that was already running (started by replay or by the app calling startSessionRecording()) kept going, and optIn() never brought replay back without an app restart. The replay evaluation now treats consent as the final gate. While opted out it never records: native is still initialized (it carries the push identity provider and its own opt-out flag, and replay needs it to exist to resume), but any running recording is stopped. optOut() and optIn() trigger that evaluation once the SDK is initialized; optIn() restarts replay under a new session instead of resuming the one that was stopped. Startup and flags reloads go through the same evaluation, so an app that starts opted out (defaultOptIn: false) also begins recording as soon as the user opts in. Refs PostHog#3813
turnipdabeets caught this in review: the opted-out stop check only fired when replay had just been initialized or _sessionReplayRecordingActive was already true, but a recording started by hand via startSessionRecording() under a linked flag (or event trigger) that's off never sets that flag — only the flags-driven path does. So optOut() left it running on Android (iOS happened to stop it when native uninstalled integrations). Simplified to stop whenever native replay is initialized, regardless of why it's recording. _stopSessionRecording() is already a no-op when nothing is running, so this doesn't change behavior for the cases that were already covered. Added the regression test requested in review. It needs the same "warm start" two-phase pattern already used in session-replay-rearm.spec.ts: the very first bootstrap evaluation for a token sees an empty cached config and defaults recordingActive to true before the real linked-flag config arrives, which sets _sessionReplayRecordingActive itself and masks the bug. Warming the persisted cache with the linked flag first, like the existing spec does, makes the real run's bootstrap read the correct cached config immediately.
|
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 |
…ed replay stopped Opt-in now waits for native setOptOut(false), which posthog-ios needs before a start, and stops the recorder native reinstalls when the app had stopped replay.
…dant opt-in Opt-in rotates the session before the trigger gate and only re-evaluates replay when the user was opted out.
…S-restarted manual recordings posthog-ios optIn() restarts the recorder, so the opt-in evaluation stops any recording it does not start itself.
…ed optIn() The opt-in evaluation no longer stops or rotates the session under a recording the app started after opting out.
… is refused while opted out
|
Thanks @nandinitiw I pushed a few changes. @PostHog/team-client-libraries mind giving this a look before merging? |
ioannisj
left a comment
There was a problem hiding this comment.
🤖 An AI agent wrote this on behalf of @ioannisj.
await optOut()can resolve before the replay stop lands, whileoptIn()now waits for its evaluation- Events captured right after an unawaited
optIn()keep the pre-opt-out session id reset()afteroptOut()skips the iOS recorder-restart correction (below, the line isn't in the diff)
On reset(): I think it needs the same opt-in handling as optIn(). It clears the persisted opt-out and _propagateNativeOptOut() (packages/react-native/src/posthog-rn.ts:919) sends setOptOut(false), which on iOS reinstalls integrations and restarts the recorder, but it never runs the freshSession evaluation that undoes that.
With replay off and a manual recording, startSessionRecording() then optOut() then reset() leaves the recorder running under mockNativeOptInRestartsRecorder(), while the same sequence ending in optIn() stops it (the "ends a manual recording on opt-in when replay is disabled" test). The plain case is fine, replay resumes on a new session through the flags reload. Would it work to capture wasOptedOut before super.reset() and chain _evaluateAndStartSessionReplay(undefined, true) after the native opt-in when consent flipped, with a test for this sequence?
Two smaller things:
- The "leaves a recording running when the user was never opted out" test asserts right after
reloadFeatureFlagsAsync(), but the flags-triggered evaluation isvoid, so it can pass before the evaluation even runs. A short settle like the other negative tests use would let it catch a regression. - Rotating the session on opt-in diverges from the browser SDK, where
opt_in_capturing()just restarts recording (packages/browser/src/posthog-core.ts:4907). #3813 asks for it so I think it's fine, but worth calling out in the PR description per AGENTS.md. The description is also a bit behind the code now (it mentions 5 tests, there are 19).
| this._sessionReplayStartedByApp = false | ||
| // Stop recording as soon as consent is withdrawn, not just drop what it captures afterwards. | ||
| if (this._isInitialized) { | ||
| void coreOptOut.then(() => this._evaluateAndStartSessionReplay()).catch(() => {}) |
There was a problem hiding this comment.
🤖 An AI agent wrote this on behalf of @ioannisj.
[suggestion] Should optOut() wait for the replay stop the way optIn() now waits for its evaluation? Right now the stop is fired with void coreOptOut.then(...) and isn't part of the returned Promise.all, so await posthog.optOut() can resolve while the recorder is still running.
With a 50ms async stopRecording in this spec's mock, isSessionReplayActive() is still true right after the await. On Android, native opt-out doesn't uninstall the recorder, so frames keep coming in that window.
Adding the stop to the returned promise would fix it, plus updating the comment here ("as soon as consent is withdrawn") and the optOut() JSDoc. Is there a reason it was left out?
There was a problem hiding this comment.
No good reason. Fixed in 84ab005: the stop is now part of the promise optOut() returns, and the comment and JSDoc say so. One side effect: await optOut() now also waits for native setup if it is still in flight. A push test that awaited optOut() before releasing setup had to change for that. Regression test: "waits for the replay stop before optOut() resolves", using your 50ms stop.
| // there is nothing to restart, because startup evaluates replay against the saved consent. | ||
| // Waits for native opt-in, because posthog-ios ignores a start while it is opted out. | ||
| if (this._isInitialized && wasOptedOut) { | ||
| const replay = Promise.all([result, nativeOptIn]) |
There was a problem hiding this comment.
🤖 An AI agent wrote this on behalf of @ioannisj.
[suggestion] Events captured right after an unawaited optIn() go out with the pre-opt-out $session_id, because the session only rotates inside the replay evaluation, after native opt-in and the eval chain.
The JSDoc example itself calls posthog.optIn() without await, so posthog.optIn(); posthog.capture('consent_given') puts the consent event on the old session while replay records under the new one. On iOS the recorder that native restarts also gets its first frames on the old session.
Could we rotate synchronously in optIn() when wasOptedOut and replay is enabled, and skip the rotation in the evaluation if it already happened? If that's too broad, maybe the JSDoc should at least say to await it before capturing.
There was a problem hiding this comment.
Fixed in 84ab005. optIn() now rotates synchronously before native opts in, under the same conditions the evaluation used (replay enabled, linked flag active, app has not started or stopped recording). The evaluation no longer rotates. This also matches posthog-ios#942, which starts the session in optIn() before reinstalling integrations. Two edge cases came out of review: rotating while native setup is still in flight (setup now reconciles native onto the new session), and a throwing native endSession when the module is not linked (guarded, so optIn() still opts in). Tests: "captures on the new session right after an unawaited optIn()", plus one for each edge case.
| } | ||
|
|
||
| // posthog-ios optIn() reinstalls its integrations, which restarts the recorder the app stopped. | ||
| if (freshSession && this._sessionReplayStoppedByApp) { |
There was a problem hiding this comment.
🤖 An AI agent wrote this on behalf of @ioannisj.
[nit] _sessionReplayStoppedByApp is only cleared by an app start, so it can go stale. If the app stops recording and the linked flag then goes off and back on, the flags path resumes recording with the flag still set, and the next opt-out/opt-in stops a recording that was running before.
Narrow, but the state differs after the round trip. Worth clearing it where the flags path restarts recording?
There was a problem hiding this comment.
Fixed in 84ab005. The flag is now cleared when the flags path confirms recording restarted, so after the round trip it means the same as iOS#942 and Android#839, which check whether replay was recording at opt-out. Test: "keeps a recording running across opt-out and opt-in once a linked flag restarted it after an app stop".
| * | ||
| * By default, PostHog has tracking enabled unless it is forcefully disabled by default using the option { defaultOptIn: false }. | ||
| * Once this has been called it is persisted and will be respected until optOut is called again or the reset function is called. | ||
| * When opting in after an opt-out, the promise resolves once session replay has been re-evaluated; if replay is eligible, it continues under a new session. |
There was a problem hiding this comment.
🤖 An AI agent wrote this on behalf of @ioannisj.
[nit] Rotation is skipped when the app started or stopped recording itself, so "continues under a new session" here (and "rotates the session" on the freshSession param) overstates it a bit. Maybe "if replay is eligible and the app hasn't started or stopped recording itself"?
There was a problem hiding this comment.
Reworded in 84ab005. The optIn() JSDoc now says events use a new session "if replay is eligible and the app hasn't started or stopped recording itself". The freshSession comment no longer mentions rotation, since the evaluation does not rotate anymore.
…d reset optOut() resolves once replay stops, optIn() starts the new session synchronously, reset() after optOut() re-evaluates replay, and a flags restart clears a stale app stop.
|
@ioannisj thanks for the review. The inline threads are answered. The points from your summary are all in 84ab005:
The latest commit is tested only with the mocks. An earlier commit was checked on Android and iOS devices. |
Fixes #3813
Problem
On React Native,
optOut()only saved the consent flag. A recording that was already running kept going, whether replay had started it or the app had started it withstartSessionRecording(). AndoptIn()never brought replay back without an app restart. The issue asks for integrations to stop on opt-out and restart on opt-in, with replay getting a new session.Much of the original scope has been fixed on
mainsince the issue was filed:startSessionRecording()and error tracking already check consent, push respects consent, and #4973 made JS own consent. This PR covers the remaining replay lifecycle gap.Changes
Consent is now the final gate in the replay evaluation (
_evaluateAndStartSessionReplayInternal). It is applied after the linked-flag and event-trigger checks, so neither can override it.await optOut()resolves once the stop has landed.optIn(), sooptIn(); capture(...)without an await already lands on it. Replay is re-evaluated after native opt-in finishes, because posthog-ios ignores a start while it is opted out.await optIn()resolves once that evaluation is done.stopSessionRecording(), opting back in leaves replay stopped. If the app starts a recording around opt-in, it keeps running. posthog-iosoptIn()reinstalls its integrations and restarts the recorder by itself, so the opt-in evaluation stops any recorder that should not be running.reset()afteroptOut(). Corereset()clears the saved opt-out, which opts native back in, so it runs the same opt-in evaluation asoptIn().defaultOptIn: false) begins recording as soon as the user opts in, without a restart.Behaviour to note:
await optOut()takes longer. It now also waits for native setup and any replay evaluation already in flight. Both native plugins always resolve their bridge calls, so this adds latency but cannot hang.opt_in_capturing()restarts recording on the current session, while RN starts a new one, as Stop all integrations like session replay and autocapture on opt-out #3813 asks.Cross-SDK. This matches posthog-ios PostHog/posthog-ios#942 and posthog-android PostHog/posthog-android#839: new session on opt-in, and a recording the app stopped stays stopped. Flutter PostHog/posthog-flutter#606 restarts replay only if opt-out was what stopped it. sdk-specs (
opt-in,consent-gating) only says replay may stop and restart, so there is no spec divergence.Not covered. The issue says "all integrations", but this PR only handles session replay. JS autocapture and lifecycle listeners don't keep unsubscribe handles, and native error/crash capture has no general stop API. All of them already drop their events at the core queue while opted out.
Release info Sub-libraries affected
Libraries affected
Checklist
If releasing new changes
pnpm changesetto generate a changeset fileTesting
test/session-replay-opt-out.spec.tshas 26 tests on the native bridge mock. The mock models posthog-ios's behaviour: it ignores a start while opted out and restarts the recorder on opt-in. Every fix in the latest commit has a test that failed before it and passes now.tscare clean./s/. The latest commit (84ab005) has only been tested with the mocks, not on a device.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
optOut()await, synchronous session rotation inoptIn(),reset()afteroptOut(), and a stale stopped-by-app flag.optIn()instead of inside the async evaluation. That way events and the first frames of the recorder iOS restarts land in the new session, as posthog-ios#942 does.🤖 Generated with Claude Code