feat(surveys): add displaySurvey to show a survey on demand#643
Draft
bs1180 wants to merge 1 commit into
Draft
Conversation
Adds PostHog.displaySurvey(surveyId), the mobile counterpart of the web SDK's posthog.displaySurvey(). It looks the survey up in the cached surveys and routes it through the existing showSurvey() path (translations, active-survey tracking, shown/sent/dismissed events), bypassing display conditions such as targeting flags, event triggers, and the seen/wait-period checks. This also enables API-type surveys, which are never auto-displayed. Generated-By: PostHog Code Task-Id: 96def33a-2460-42b7-9690-0a75ae8383fe
Author
|
Hi @ioannisj @turnipdabeets 👋 — tagging you both since you maintain the surveys code here. This is the Android half of the manual |
5 tasks
Contributor
posthog-android Compliance ReportDate: 2026-07-20 13:31:22 UTC ✅ All Tests Passed!46/46 tests passed Capture Tests✅ 29/29 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
|
Contributor
🦔 ReviewHog reviewed this pull requestFound 1 must fix, 2 should fix, 0 consider. Published 3 findings (view the review). |
Contributor
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
ReviewHog Report
Changes
Issues: 3 issues
Files (8)
.changeset/display-survey-manual-api.mdposthog-android/api/posthog-android.apiposthog-android/src/main/java/com/posthog/android/surveys/PostHogSurveysIntegration.ktposthog/api/posthog.apiposthog/src/main/java/com/posthog/PostHog.ktposthog/src/main/java/com/posthog/PostHogInterface.ktposthog/src/main/java/com/posthog/internal/surveys/PostHogSurveysHandler.ktposthog/src/testFixtures/java/com/posthog/PostHogFake.kt
Comment on lines
+151
to
+167
| public override fun displaySurvey(surveyId: String) { | ||
| if (!config.surveys) { | ||
| config.logger.log("[Surveys] Cannot display survey $surveyId - surveys are disabled in the config") | ||
| return | ||
| } | ||
| val isIntegrationStarted = synchronized(lifecycleLock) { isStarted } | ||
| if (!isIntegrationStarted) { | ||
| config.logger.log("[Surveys] Cannot display survey $surveyId - surveys integration is not started") | ||
| return | ||
| } | ||
| val survey = synchronized(surveysLock) { cachedSurveys.firstOrNull { it.id == surveyId } } | ||
| if (survey == null) { | ||
| config.logger.log("[Surveys] Cannot display survey $surveyId - survey not found") | ||
| return | ||
| } | ||
| showSurvey(survey) | ||
| } |
Contributor
There was a problem hiding this comment.
displaySurvey() doesn't verify the survey is currently running, despite the documented contract requiring it
Why we think it's a valid issue
- Checked: the public contract (
PostHogInterface.kt:327-340), the implementation (PostHogSurveysIntegration.kt:151-167), the auto-display filter (getActiveMatchingSurveys, lines 255-306), howcachedSurveysis populated (line 128 ←PostHog.kt:299←PostHogRemoteConfig.getSurveys()atPostHogRemoteConfig.kt:1243-1247), theSurveymodel (Survey.kt:26-29), and the new test fixtures. - Found:
PostHogInterface.kt:330explicitly documents "The survey must be running ...; targeting flags, event triggers, and the seen/wait-period checks are bypassed" — enumerating only three bypassed conditions and requiring running. ButdisplaySurvey(line 161) resolves the survey solely bycachedSurveys.firstOrNull { it.id == surveyId }and callsshowSurveywith nostartDate/endDateguard. - Found:
getActiveMatchingSurveysappliesif (survey.startDate == null || survey.endDate != null) return@filter falseas its very first check (line 260, commented "must have start date and no end date") against the samecachedSurveyslist. That guard existing on the auto-display path is direct in-repo evidence that the raw cache is expected to contain non-running (not-yet-started or ended) surveys — so the premise is not speculative. - Found:
cachedSurveys(line 128) is assigned the unfiltered list fromonSurveysLoaded, which receivesremoteConfig.getSurveys()verbatim; no date filtering happens anywhere on that path. - Impact:
displaySurvey("ended-or-draft-id")for a survey still present in the cache will render it and firesurvey shown/sent/dismissedevents, collecting responses for a stopped/unlaunched campaign — contradicting the method's own documented "must be running" contract and polluting that survey's response data past its intended end date. This is a real correctness/contract gap on newly added code with a trivial one-line fix (mirror thegetActiveMatchingSurveysrunning check), and the new test suite (createSurveyalways usesstartDate = Date(), endDate = null) never exercises it. It clears the keep bar: concrete trigger and concrete consequence, not overengineering, style, or defensive paranoia.
Issue description
The public contract in PostHogInterface.kt (lines 327-341) explicitly states: "The survey must be running and returned by the project's remote config; targeting flags, event triggers, and the seen/wait-period checks are bypassed." The PR description and changeset also scope the bypass to only "targeting flags, event triggers, and seen/wait-period checks" — the active-window (running) requirement is not listed among the bypassed conditions.
However, the actual implementation of displaySurvey only checks that the survey ID exists in cachedSurveys (cachedSurveys.firstOrNull { it.id == surveyId }, line 161) — it never checks survey.startDate/survey.endDate. Compare this with getActiveMatchingSurveys() (lines 255-306), which explicitly filters if (survey.startDate == null || survey.endDate != null) return@filter false as its very first "is this survey running" check before any of the other display-condition checks that displaySurvey intentionally bypasses. cachedSurveys itself is the raw, unfiltered list pushed straight from PostHogRemoteConfig.getSurveys() (confirmed in PostHog.kt line 299 and PostHogRemoteConfig.kt line 1243-1247), so it can legitimately contain surveys that haven't started yet (no startDate) or have already ended (endDate set) — the existence of the dedicated filter in getActiveMatchingSurveys is proof that such surveys are present in the cache.
As written, calling PostHog.displaySurvey("ended-survey-id") for a survey whose endDate has already passed (e.g. a closed campaign) will still render it and fire survey shown/survey sent/survey dismissed events for it, contradicting the documented "must be running" requirement and potentially polluting the closed survey's response data after its intended end date. The new PostHogSurveysDisplaySurveyTest doesn't cover this: every survey fixture in that test file is constructed with startDate = Date() and endDate = null (always running), so this gap has no test coverage.
Suggested fix
Add the same "is running" check that getActiveMatchingSurveys() already uses before calling showSurvey(survey), e.g.:
val survey = synchronized(surveysLock) { cachedSurveys.firstOrNull { it.id == surveyId } }
if (survey == null || survey.startDate == null || survey.endDate != null) {
config.logger.log("[Surveys] Cannot display survey $surveyId - survey not found or not running")
return
}Alternatively, if showing not-yet-started/ended surveys on demand is actually intended (e.g. to preview drafts), update the docstring in PostHogInterface.kt (and the changeset) to drop the "must be running" claim so the documented contract matches the real behavior, and add a test exercising an ended/draft survey ID to lock in the intended behavior either way.
Prompt to fix with AI (copy-paste)
## Context
@posthog-android/src/main/java/com/posthog/android/surveys/PostHogSurveysIntegration.kt#L151-167
<issue_description>
The public contract in PostHogInterface.kt (lines 327-341) explicitly states: "The survey must be running and returned by the project's remote config; targeting flags, event triggers, and the seen/wait-period checks are bypassed." The PR description and changeset also scope the bypass to only "targeting flags, event triggers, and seen/wait-period checks" — the active-window (running) requirement is not listed among the bypassed conditions.
However, the actual implementation of `displaySurvey` only checks that the survey ID exists in `cachedSurveys` (`cachedSurveys.firstOrNull { it.id == surveyId }`, line 161) — it never checks `survey.startDate`/`survey.endDate`. Compare this with `getActiveMatchingSurveys()` (lines 255-306), which explicitly filters `if (survey.startDate == null || survey.endDate != null) return@filter false` as its very first "is this survey running" check before any of the other display-condition checks that `displaySurvey` intentionally bypasses. `cachedSurveys` itself is the raw, unfiltered list pushed straight from `PostHogRemoteConfig.getSurveys()` (confirmed in `PostHog.kt` line 299 and `PostHogRemoteConfig.kt` line 1243-1247), so it can legitimately contain surveys that haven't started yet (no `startDate`) or have already ended (`endDate` set) — the existence of the dedicated filter in `getActiveMatchingSurveys` is proof that such surveys are present in the cache.
As written, calling `PostHog.displaySurvey("ended-survey-id")` for a survey whose `endDate` has already passed (e.g. a closed campaign) will still render it and fire `survey shown`/`survey sent`/`survey dismissed` events for it, contradicting the documented "must be running" requirement and potentially polluting the closed survey's response data after its intended end date. The new `PostHogSurveysDisplaySurveyTest` doesn't cover this: every survey fixture in that test file is constructed with `startDate = Date()` and `endDate = null` (always running), so this gap has no test coverage.
</issue_description>
<issue_validation>
- **Checked:** the public contract (`PostHogInterface.kt:327-340`), the implementation (`PostHogSurveysIntegration.kt:151-167`), the auto-display filter (`getActiveMatchingSurveys`, lines 255-306), how `cachedSurveys` is populated (line 128 ← `PostHog.kt:299` ← `PostHogRemoteConfig.getSurveys()` at `PostHogRemoteConfig.kt:1243-1247`), the `Survey` model (`Survey.kt:26-29`), and the new test fixtures.
- **Found:** `PostHogInterface.kt:330` explicitly documents "The survey must be running ...; targeting flags, event triggers, and the seen/wait-period checks are bypassed" — enumerating only three bypassed conditions and requiring running. But `displaySurvey` (line 161) resolves the survey solely by `cachedSurveys.firstOrNull { it.id == surveyId }` and calls `showSurvey` with no `startDate`/`endDate` guard.
- **Found:** `getActiveMatchingSurveys` applies `if (survey.startDate == null || survey.endDate != null) return@filter false` as its very first check (line 260, commented "must have start date and no end date") against the *same* `cachedSurveys` list. That guard existing on the auto-display path is direct in-repo evidence that the raw cache is expected to contain non-running (not-yet-started or ended) surveys — so the premise is not speculative.
- **Found:** `cachedSurveys` (line 128) is assigned the unfiltered list from `onSurveysLoaded`, which receives `remoteConfig.getSurveys()` verbatim; no date filtering happens anywhere on that path.
- **Impact:** `displaySurvey("ended-or-draft-id")` for a survey still present in the cache will render it and fire `survey shown`/`sent`/`dismissed` events, collecting responses for a stopped/unlaunched campaign — contradicting the method's own documented "must be running" contract and polluting that survey's response data past its intended end date. This is a real correctness/contract gap on newly added code with a trivial one-line fix (mirror the `getActiveMatchingSurveys` running check), and the new test suite (`createSurvey` always uses `startDate = Date(), endDate = null`) never exercises it. It clears the keep bar: concrete trigger and concrete consequence, not overengineering, style, or defensive paranoia.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Add the same "is running" check that `getActiveMatchingSurveys()` already uses before calling `showSurvey(survey)`, e.g.:
```kotlin
val survey = synchronized(surveysLock) { cachedSurveys.firstOrNull { it.id == surveyId } }
if (survey == null || survey.startDate == null || survey.endDate != null) {
config.logger.log("[Surveys] Cannot display survey $surveyId - survey not found or not running")
return
}
Alternatively, if showing not-yet-started/ended surveys on demand is actually intended (e.g. to preview drafts), update the docstring in PostHogInterface.kt (and the changeset) to drop the "must be running" claim so the documented contract matches the real behavior, and add a test exercising an ended/draft survey ID to lock in the intended behavior either way.
</potential_solution>
</details>
Comment on lines
+2019
to
+2027
| override fun displaySurvey(surveyId: String) { | ||
| if (!isEnabled()) { | ||
| return | ||
| } | ||
|
|
||
| surveysHandler?.displaySurvey(surveyId) ?: run { | ||
| config?.logger?.log("Cannot display survey $surveyId - surveys integration isn't installed.") | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
New displaySurvey() entry point lacks the exception guard every other survey-triggering call site uses
Why we think it's a valid issue
- Checked: the new
displaySurveyentry point (PostHog.kt:2019-2027), the three sibling survey-triggering call sites it is compared against, and whether the guard is instead applied inside the integration (PostHogSurveysIntegration.displaySurvey/showSurvey). - Found (siblings all guarded):
PostHog.kt:193-198wrapssurveysHandler?.onSurveysLoaded(surveys)intry/catch (Throwable);PostHog.kt:298-303wraps the install-timeit.onSurveysLoaded(surveys); andPostHog.kt:816surveysHandler?.onEvent(...)sits insidecapture()'s top-leveltry { ... } catch (e: Throwable)(lines 713-822). All three reachshowSurvey()(viashowNextSurvey()), so the SDK deliberately guards this exact chain at thePostHog.ktboundary. - Found (new path unguarded, synchronous):
PostHog.kt:2024callssurveysHandler?.displaySurvey(surveyId)with no guard. The integration'sdisplaySurvey(PostHogSurveysIntegration.kt:151-166) callsshowSurvey(survey)directly and synchronously, andshowSurveyrunsresolveSurveyTranslations,PostHogDisplaySurvey.toDisplaySurvey, thengetSurveysDelegate().renderSurvey(...)at line 437 — none wrapped in try/catch. So a throw from the customer-suppliedPostHogSurveysDelegateor from translation/model conversion propagates synchronously back out through the publicPostHog.displaySurvey(). - Impact:
displaySurveyis a public, synchronous API invoked from app code (e.g. a button tap, typically the main thread) and is the only path that renders API-type/condition-bypassing surveys — the shapes least exercised elsewhere — and the delegate is an explicit customer extension point. An uncaught exception there crashes the host app, a failure mode an analytics SDK is expected to absorb and that every other survey call site in this file already absorbs. Concrete trigger + concrete consequence, directly on the new code, matching an established unanimous convention — clears the keep bar; not defensive paranoia (the delegate is real external code that can throw) or style. - Priority: Keeping
must_fix— the defect is a host-app crash escaping a public SDK entry point, and 3/3 sibling call sites treat this chain as requiring the guard, so the omission is a clear crash-safety regression rather than a minor inconsistency.
Issue description
Every other place in this file that can trigger PostHogSurveysIntegration.showSurvey() (and thus delegate renderSurvey(), translation resolution, and PostHogDisplaySurvey.toDisplaySurvey conversion) wraps the call in try { } catch (e: Throwable): the onRemoteConfigLoaded callback's surveysHandler?.onSurveysLoaded(surveys) (lines 193-198), the install-time cached-surveys push it.onSurveysLoaded(surveys) (lines 298-303), and the event-driven surveysHandler?.onEvent(event, mergedProperties) inside capture()'s own top-level try/catch (lines 704-822). The new override fun displaySurvey(surveyId: String) calls surveysHandler?.displaySurvey(surveyId) with no such guard. displaySurvey is a public, synchronous, on-demand API explicitly meant to be invoked directly from app code (e.g. a button tap) and, per the PR description, is also the only path that renders API-type surveys and any survey configuration that bypasses the normal display-condition filtering — i.e. it is the first path likely to hit malformed/unexercised survey shapes or a buggy custom PostHogSurveysDelegate (delegates are an explicit customer extension point). If showSurvey() or the delegate throws, that exception now propagates uncaught straight out of the SDK's public API into the host app, unlike every other survey code path in this file, risking a host-app crash from what is supposed to be a resilient analytics SDK call.
Suggested fix
Wrap the delegate call the same way the other call sites in this file do:
override fun displaySurvey(surveyId: String) {
if (!isEnabled()) {
return
}
try {
surveysHandler?.displaySurvey(surveyId) ?: run {
config?.logger?.log("Cannot display survey $surveyId - surveys integration isn't installed.")
}
} catch (e: Throwable) {
config?.logger?.log("Failed to display survey $surveyId: $e.")
}
}Prompt to fix with AI (copy-paste)
## Context
@posthog/src/main/java/com/posthog/PostHog.kt#L2019-2027
<issue_description>
Every other place in this file that can trigger `PostHogSurveysIntegration.showSurvey()` (and thus delegate `renderSurvey()`, translation resolution, and `PostHogDisplaySurvey.toDisplaySurvey` conversion) wraps the call in `try { } catch (e: Throwable)`: the `onRemoteConfigLoaded` callback's `surveysHandler?.onSurveysLoaded(surveys)` (lines 193-198), the install-time cached-surveys push `it.onSurveysLoaded(surveys)` (lines 298-303), and the event-driven `surveysHandler?.onEvent(event, mergedProperties)` inside `capture()`'s own top-level try/catch (lines 704-822). The new `override fun displaySurvey(surveyId: String)` calls `surveysHandler?.displaySurvey(surveyId)` with no such guard. `displaySurvey` is a public, synchronous, on-demand API explicitly meant to be invoked directly from app code (e.g. a button tap) and, per the PR description, is also the only path that renders API-type surveys and any survey configuration that bypasses the normal display-condition filtering — i.e. it is the first path likely to hit malformed/unexercised survey shapes or a buggy custom `PostHogSurveysDelegate` (delegates are an explicit customer extension point). If `showSurvey()` or the delegate throws, that exception now propagates uncaught straight out of the SDK's public API into the host app, unlike every other survey code path in this file, risking a host-app crash from what is supposed to be a resilient analytics SDK call.
</issue_description>
<issue_validation>
- **Checked:** the new `displaySurvey` entry point (`PostHog.kt:2019-2027`), the three sibling survey-triggering call sites it is compared against, and whether the guard is instead applied inside the integration (`PostHogSurveysIntegration.displaySurvey`/`showSurvey`).
- **Found (siblings all guarded):** `PostHog.kt:193-198` wraps `surveysHandler?.onSurveysLoaded(surveys)` in `try/catch (Throwable)`; `PostHog.kt:298-303` wraps the install-time `it.onSurveysLoaded(surveys)`; and `PostHog.kt:816` `surveysHandler?.onEvent(...)` sits inside `capture()`'s top-level `try { ... } catch (e: Throwable)` (lines 713-822). All three reach `showSurvey()` (via `showNextSurvey()`), so the SDK deliberately guards this exact chain at the `PostHog.kt` boundary.
- **Found (new path unguarded, synchronous):** `PostHog.kt:2024` calls `surveysHandler?.displaySurvey(surveyId)` with no guard. The integration's `displaySurvey` (`PostHogSurveysIntegration.kt:151-166`) calls `showSurvey(survey)` directly and synchronously, and `showSurvey` runs `resolveSurveyTranslations`, `PostHogDisplaySurvey.toDisplaySurvey`, then `getSurveysDelegate().renderSurvey(...)` at line 437 — none wrapped in try/catch. So a throw from the customer-supplied `PostHogSurveysDelegate` or from translation/model conversion propagates synchronously back out through the public `PostHog.displaySurvey()`.
- **Impact:** `displaySurvey` is a public, synchronous API invoked from app code (e.g. a button tap, typically the main thread) and is the only path that renders API-type/condition-bypassing surveys — the shapes least exercised elsewhere — and the delegate is an explicit customer extension point. An uncaught exception there crashes the host app, a failure mode an analytics SDK is expected to absorb and that every other survey call site in this file already absorbs. Concrete trigger + concrete consequence, directly on the new code, matching an established unanimous convention — clears the keep bar; not defensive paranoia (the delegate is real external code that can throw) or style.
- **Priority:** Keeping `must_fix` — the defect is a host-app crash escaping a public SDK entry point, and 3/3 sibling call sites treat this chain as requiring the guard, so the omission is a clear crash-safety regression rather than a minor inconsistency.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Wrap the delegate call the same way the other call sites in this file do:
```kotlin
override fun displaySurvey(surveyId: String) {
if (!isEnabled()) {
return
}
try {
surveysHandler?.displaySurvey(surveyId) ?: run {
config?.logger?.log("Cannot display survey $surveyId - surveys integration isn't installed.")
}
} catch (e: Throwable) {
config?.logger?.log("Failed to display survey $surveyId: $e.")
}
}
</potential_solution>
</details>
Comment on lines
+151
to
+167
| public override fun displaySurvey(surveyId: String) { | ||
| if (!config.surveys) { | ||
| config.logger.log("[Surveys] Cannot display survey $surveyId - surveys are disabled in the config") | ||
| return | ||
| } | ||
| val isIntegrationStarted = synchronized(lifecycleLock) { isStarted } | ||
| if (!isIntegrationStarted) { | ||
| config.logger.log("[Surveys] Cannot display survey $surveyId - surveys integration is not started") | ||
| return | ||
| } | ||
| val survey = synchronized(surveysLock) { cachedSurveys.firstOrNull { it.id == surveyId } } | ||
| if (survey == null) { | ||
| config.logger.log("[Surveys] Cannot display survey $surveyId - survey not found") | ||
| return | ||
| } | ||
| showSurvey(survey) | ||
| } |
Contributor
There was a problem hiding this comment.
displaySurvey()'s documented 'ignored if another survey is active' guarantee is not enforced atomically
Why we think it's a valid issue
- Checked: the guard/claim ordering across
showSurvey(line 318-323, 437),canShowNextSurvey(line 683-685), theactiveSurveywrite in theonSurveyShowncallback (lines 341-361), the default delegate (PostHogSurveysDefaultDelegate.renderSurvey), and thesurveyPopupDelaySecondsfield in the public appearance model. - Found (non-atomic claim):
showSurveyonly readsactiveSurveyviacanShowNextSurvey()(line 320 → 683-685) and then callsgetSurveysDelegate().renderSurvey(...)(line 437). The corresponding writeactiveSurvey = originalSurveyhappens at line 347, but only inside theonSurveyShownlambda that the delegate invokes — never duringshowSurveyitself. So betweenrenderSurveybeing called and the delegate later firingonSurveyShown,activeSurveystaysnulland any secondshowSurveypasses the check. This is a genuine TOCTOU, confirmed by the code, not the snippet. - Found (async is the norm, not the exception):
onSurveyShownis a "survey became visible" callback fired by delegate UI code afterrenderSurveyreturns; the publicsurveyPopupDelaySecondsfield (PostHogDisplaySurveyAppearance.kt:40,SurveyAppearance.kt:27) explicitly encodes a delay between trigger and display, so the window is architecturally expected. The bundledPostHogSurveysDefaultDelegaterenders nothing and never callsonSurveyShown, and its own log says "Implement your own PostHogSurveysDelegate to render surveys" — i.e. every functional deployment uses a custom delegate subject to this window. - Found (spurious events): in
onSurveyShown,sendSurveyShownEvent(originalSurvey, ...)(line 354) runs unconditionally, outside theif (activeSurvey == null)block (lines 346-351), so two overlapping renders each emit asurvey shown(and laterdismissed) event even though only one wins the active slot. - Impact:
displaySurveyis a public API built for uncoordinated app-code invocation (button tap, double-tap, or racing an automaticonEvent/onSurveysLoadedtrigger). Two calls landing in the render window both reachrenderSurvey, breaking the docstring's "If another survey is already being displayed, this call is ignored" guarantee and polluting analytics with events for surveys the user never saw. Concrete trigger + concrete consequence on newly added code; the suggested fix (claimactiveSurveyin the sameactiveSurvey Locksection as the check) is the standard correct pattern. Clears the keep bar — a race corrupting a documented shared-state invariant, not speculation or paranoia. - Priority: keeping
should_fix— real invariant break with data-quality/UX impact, but it needs an overlapping-trigger window and yields duplicate events rather than a crash, data loss, or security hole, so must_fix would overstate it.
Issue description
The docstring for displaySurvey (lines 140-150) promises: "If another survey is already being displayed, this call is ignored." That guarantee relies entirely on showSurvey()'s canShowNextSurvey() check (synchronized(activeSurveyLock) { activeSurvey == null }), but activeSurvey is only ever set later, inside the onSurveyShown callback that the delegate invokes asynchronously — potentially after a configurable popup delay (e.g. surveyPopupDelaySeconds in the bundled Compose delegate), or on whatever schedule a custom PostHogSurveysDelegate implementation chooses. Between showSurvey(survey) being called and the delegate eventually invoking onSurveyShown, activeSurvey remains null, so a concurrent second call — another displaySurvey(otherId), or an automatic trigger via onEvent/onSurveysLoaded firing in that same window — also passes canShowNextSurvey() and reaches getSurveysDelegate().renderSurvey(...). Before this PR, only automatic triggers could race each other; displaySurvey is designed to be called from arbitrary, uncoordinated app code (e.g. a button's onClick), which meaningfully raises the odds of hitting this window in production and produces two concurrent renderSurvey() calls, spurious survey shown/survey dismissed events for a survey the user never actually saw, and a violated 'single active survey' invariant.
Suggested fix
Mark the survey as claimed atomically with the decision to render, inside the same activeSurveyLock critical section that performs the canShowNextSurvey() check, instead of deferring that write to the async onSurveyShown callback, e.g.:
internal fun showSurvey(survey: Survey) {
synchronized(activeSurveyLock) {
if (activeSurvey != null) {
config.logger.log("Cannot show survey - another survey is already active")
return
}
activeSurvey = survey
activeSurveyCompleted = false
currentSurveyResponses.clear()
}
// ... existing setup, with onSurveyShown now just sending the 'survey shown' event
// instead of also setting activeSurvey
}Prompt to fix with AI (copy-paste)
## Context
@posthog-android/src/main/java/com/posthog/android/surveys/PostHogSurveysIntegration.kt#L151-167
<issue_description>
The docstring for `displaySurvey` (lines 140-150) promises: "If another survey is already being displayed, this call is ignored." That guarantee relies entirely on `showSurvey()`'s `canShowNextSurvey()` check (`synchronized(activeSurveyLock) { activeSurvey == null }`), but `activeSurvey` is only ever *set* later, inside the `onSurveyShown` callback that the delegate invokes asynchronously — potentially after a configurable popup delay (e.g. `surveyPopupDelaySeconds` in the bundled Compose delegate), or on whatever schedule a custom `PostHogSurveysDelegate` implementation chooses. Between `showSurvey(survey)` being called and the delegate eventually invoking `onSurveyShown`, `activeSurvey` remains `null`, so a concurrent second call — another `displaySurvey(otherId)`, or an automatic trigger via `onEvent`/`onSurveysLoaded` firing in that same window — also passes `canShowNextSurvey()` and reaches `getSurveysDelegate().renderSurvey(...)`. Before this PR, only automatic triggers could race each other; `displaySurvey` is designed to be called from arbitrary, uncoordinated app code (e.g. a button's onClick), which meaningfully raises the odds of hitting this window in production and produces two concurrent `renderSurvey()` calls, spurious `survey shown`/`survey dismissed` events for a survey the user never actually saw, and a violated 'single active survey' invariant.
</issue_description>
<issue_validation>
- **Checked:** the guard/claim ordering across `showSurvey` (line 318-323, 437), `canShowNextSurvey` (line 683-685), the `activeSurvey` write in the `onSurveyShown` callback (lines 341-361), the default delegate (`PostHogSurveysDefaultDelegate.renderSurvey`), and the `surveyPopupDelaySeconds` field in the public appearance model.
- **Found (non-atomic claim):** `showSurvey` only *reads* `activeSurvey` via `canShowNextSurvey()` (line 320 → 683-685) and then calls `getSurveysDelegate().renderSurvey(...)` (line 437). The corresponding *write* `activeSurvey = originalSurvey` happens at line 347, but only inside the `onSurveyShown` lambda that the delegate invokes — never during `showSurvey` itself. So between `renderSurvey` being called and the delegate later firing `onSurveyShown`, `activeSurvey` stays `null` and any second `showSurvey` passes the check. This is a genuine TOCTOU, confirmed by the code, not the snippet.
- **Found (async is the norm, not the exception):** `onSurveyShown` is a "survey became visible" callback fired by delegate UI code after `renderSurvey` returns; the public `surveyPopupDelaySeconds` field (`PostHogDisplaySurveyAppearance.kt:40`, `SurveyAppearance.kt:27`) explicitly encodes a delay between trigger and display, so the window is architecturally expected. The bundled `PostHogSurveysDefaultDelegate` renders nothing and never calls `onSurveyShown`, and its own log says "Implement your own PostHogSurveysDelegate to render surveys" — i.e. every functional deployment uses a custom delegate subject to this window.
- **Found (spurious events):** in `onSurveyShown`, `sendSurveyShownEvent(originalSurvey, ...)` (line 354) runs unconditionally, outside the `if (activeSurvey == null)` block (lines 346-351), so two overlapping renders each emit a `survey shown` (and later `dismissed`) event even though only one wins the active slot.
- **Impact:** `displaySurvey` is a public API built for uncoordinated app-code invocation (button tap, double-tap, or racing an automatic `onEvent`/`onSurveysLoaded` trigger). Two calls landing in the render window both reach `renderSurvey`, breaking the docstring's "If another survey is already being displayed, this call is ignored" guarantee and polluting analytics with events for surveys the user never saw. Concrete trigger + concrete consequence on newly added code; the suggested fix (claim `activeSurvey` in the same `activeSurvey Lock` section as the check) is the standard correct pattern. Clears the keep bar — a race corrupting a documented shared-state invariant, not speculation or paranoia.
- **Priority:** keeping `should_fix` — real invariant break with data-quality/UX impact, but it needs an overlapping-trigger window and yields duplicate events rather than a crash, data loss, or security hole, so must_fix would overstate it.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Mark the survey as claimed atomically with the decision to render, inside the same `activeSurveyLock` critical section that performs the `canShowNextSurvey()` check, instead of deferring that write to the async `onSurveyShown` callback, e.g.:
```kotlin
internal fun showSurvey(survey: Survey) {
synchronized(activeSurveyLock) {
if (activeSurvey != null) {
config.logger.log("Cannot show survey - another survey is already active")
return
}
activeSurvey = survey
activeSurveyCompleted = false
currentSurveyResponses.clear()
}
// ... existing setup, with onSurveyShown now just sending the 'survey shown' event
// instead of also setting activeSurvey
}
</potential_solution>
</details>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
💡 Motivation and Context
The web SDK exposes
posthog.displaySurvey()to display a survey on demand (see implementing custom surveys), but there is no mobile equivalent, so surveys can only be shown when the automatic display conditions are met, and API-type surveys can't be shown at all. This is tracked for the other mobile SDKs in PostHog/posthog-ios#648, PostHog/posthog-flutter#225, and PostHog/posthog-js#3817 (there is no posthog-android issue, but the gap is the same). This adds the Android counterpart:displaySurvey(surveyId)onPostHogInterface/PostHog, delegated throughPostHogSurveysHandlertoPostHogSurveysIntegration.showSurvey()path (translations, active-survey tracking,survey shown/sent/dismissedevents), so all downstream behavior is unchanged.showNextSurvey()intentionally never auto-displays.Sibling PRs: PostHog/posthog-ios#730 and a posthog-flutter PR that builds on this API (Flutter delegates survey logic to the native SDKs).
💚 How did you test it?
Added
PostHogSurveysDisplaySurveyTestcovering: displaying an API-type survey by ID, bypassing an unfired event trigger, unknown ID as a no-op, ignoring the call while another survey is active, and the surveys-disabled config case. Ran:posthog:apiCheck :posthog-android:apiCheck :posthog-android:testDebugUnitTest --tests "com.posthog.android.surveys.PostHogSurveysDisplaySurveyTest"locally — all green.📝 Checklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Authored with PostHog Code (Claude Code) from a request to bring the JS SDK's
displaySurveyto the Flutter SDK; since Flutter's survey pipeline lives in the native SDKs, this Android API is a prerequisite. Design choices: expose the method onPostHogInterface(so the Flutter plugin can call the staticPostHog.displaySurvey), reuse the internalshowSurvey()path rather than a parallel code path, and keep v1 to a singlesurveyIdparameter (noignoreConditions/prefill options yet). API dumps updated for both modules;PostHogFakeextended for the new interface method.Created with PostHog Code