fix: initializing surveys twice and calling survey shown/dismissed more than once per survey

[lucasheriques]

Fix race conditions in survey initialization

Problem

Two intermittent race conditions causing duplicate survey displays:

1. Multiple concurrent calls to /surveys endpoint
2. Multiple SurveyManager instances being created during PostHog's remote config loading process

These issues don't occur consistently because they depend on:

• Network response timing between config loading attempts
• Browser's script loading/execution speed
• Whether PostHog needs to fall back to different config loading strategies

Solution

Added synchronization mechanisms:

1. _isFetchingSurveys flag to prevent concurrent API calls
2. _isInitializingSurveys flag with proper cleanup to ensure only one SurveyManager instance is created

Note: Multiple "Already initializing surveys" logs are expected due to PostHog's config loading retry mechanism, but only one initialization actually occurs.

Testing

Verify:

1. Single network call to /surveys
2. Single interval ID in console logs
3. Surveys display only once

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

[vercel]

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

Name	Status	Preview	Updated (UTC)
posthog-js	✅ Ready (Inspect)	Visit Preview	Feb 22, 2025 5:06am

[greptile-apps]

PR Summary

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here: app.greptile.com/review/github.

_{1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile}

[github-actions]

Size Change: +6.41 kB (+0.19%)

Total Size: 3.32 MB

Filename	Size	Change
dist/array.full.es5.js	270 kB	+619 B (+0.23%)
dist/array.full.js	373 kB	+646 B (+0.17%)
dist/array.full.no-external.js	372 kB	+646 B (+0.17%)
dist/array.js	184 kB	+642 B (+0.35%)
dist/array.no-external.js	183 kB	+642 B (+0.35%)
dist/main.js	185 kB	+642 B (+0.35%)
dist/module.full.js	373 kB	+646 B (+0.17%)
dist/module.full.no-external.js	372 kB	+646 B (+0.17%)
dist/module.js	185 kB	+642 B (+0.35%)
dist/module.no-external.js	183 kB	+642 B (+0.35%)

ℹ️ View Unchanged

Filename	Size
dist/all-external-dependencies.js	216 kB
dist/customizations.full.js	14 kB
dist/dead-clicks-autocapture.js	14.5 kB
dist/exception-autocapture.js	9.51 kB
dist/external-scripts-loader.js	2.64 kB
dist/recorder-v2.js	115 kB
dist/recorder.js	115 kB
dist/surveys-preview.js	70.3 kB
dist/surveys.js	73.4 kB
dist/tracing-headers.js	1.76 kB
dist/web-vitals.js	10.4 kB

_{compressed-size-action}

[marandaneto]

if there's no connection, is the callback still called so we can reset the _isFetchingSurveys?
or is it gonna throw and the catch will reset it?

[rafaeelaudibert]

This needs to be tested, but I believe callback will still be called even if there's no connection

[marandaneto]

agree, and this test actually calls the callback in the test, so its not really testing what it would happen in case of a timeout/no connection

[marandaneto]

posthog-js/src/posthog-surveys.ts

Lines 257 to 259 in 8a82b35

	if (this._surveyEventReceiver == null) {
	this._surveyEventReceiver = new SurveyEventReceiver(this.instance)
	}

i think this should be removed and let be init only by loadIfEnabled

[marandaneto]

posthog-js/src/posthog-surveys.ts

Lines 224 to 226 in 8a82b35

	if (this._surveyEventReceiver == null) {
	this._surveyEventReceiver = new SurveyEventReceiver(this.instance)
	}

i think this should only be init if the _surveyManager was also properly inited, _surveyManager might fail because of eg 'Could not load surveys script' but the _surveyEventReceiver is now in a limbo state

[rafaeelaudibert]

Dropping a couple of very cheap 2 Brazilian cents

[rafaeelaudibert]

This needs to be tested, but I believe callback will still be called even if there's no connection

[lucasheriques]

@marandaneto also changed to now only instantiate SurveyEventReceiver once, on loadIfEnabled

edit: actually, nevermind, as we need to make sure SurveyEventReceiver is instantiated on getSurveys too. As that's the moment when we register the event receivers. (it broke a test when I removed it)

If surveys is disabled, we still return early so no code is executed.

[marandaneto]

SurveyEventReceiver

if getSurveys is called before loadIfEnabled is fully initialized, either getSurveys should await for loadIfEnabled, or it should return nothing, but it seems wrong to instant things elsewhere.

loadIfEnabled calls generateSurveys, and only generateSurveys calls getSurveys automatically, so at this point everything should be init.

IMO the test should be wrong then unless I miss something

[lucasheriques]

@marandaneto agree - getSurveys shouldn't be called if loadIfEnabled wasn't.

I changed the test to first call loadIfEnabled and instantiate the event receiver.

Also, couple of other changes:

• Added the config for surveys request timeout (plus add default value for survey timeout)
• remove double check for the survey manager - not needed
• move _isInitializingSurveys to inside the callback: as you pointed, it's an aync call, so it should be only called if the survey is sucessfully loaded. on the finally block, it could be called even without finishing it