Skip to content

feat(surveys): add displaySurvey to show a survey on demand - #730

Closed
bs1180 wants to merge 2 commits into
mainfrom
posthog-code/display-survey-manual-api
Closed

feat(surveys): add displaySurvey to show a survey on demand#730
bs1180 wants to merge 2 commits into
mainfrom
posthog-code/display-survey-manual-api

Conversation

@bs1180

@bs1180 bs1180 commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

💡 Motivation and Context

Closes #648

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 adds the iOS counterpart:

PostHogSDK.shared.displaySurvey("survey-id")
  • Looks the survey up in the loaded surveys and routes it through the existing display path (translations, active-survey tracking, survey shown/sent/dismissed events), so all downstream behavior is unchanged.
  • Bypasses display conditions (targeting flags, event triggers, seen/wait-period checks), matching the web SDK's default behavior.
  • Also enables API-type surveys, which showNextSurvey() intentionally never auto-displays.
  • No-op (with a log) when the survey isn't found or another survey is already being displayed.

showNextSurvey() was refactored to share the new presentSurvey(_:) helper; its behavior is unchanged.

Sibling PRs: posthog-android and posthog-flutter (Flutter delegates survey logic to the native SDKs, so it needs this API first). RN is tracked in PostHog/posthog-js#3817.

💚 How did you test it?

Added PostHogSurveyDisplaySurveyTest covering: displaying an API-type survey by ID, bypassing an unfired event trigger, unknown ID as a no-op, and ignoring the call while another survey is active. Written to match the existing survey test harness, but not executed locally (authored in a Linux environment without an Apple toolchain) — relying on CI here.

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • I updated the docs if needed.
  • No breaking change or entry added to the changelog.

If releasing new changes

  • Ran pnpm changeset to 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 displaySurvey to the Flutter SDK; since Flutter's survey pipeline lives in the native SDKs, this iOS API is the prerequisite. Design choice: reuse the internal display path via a shared presentSurvey(_:) helper rather than a parallel code path, and keep v1 to a single surveyId parameter (no ignoreConditions/prefill options yet) to match the tracking issue's scope. Tests assert on integration state via the existing TESTING extension instead of rendering UI.


Created with PostHog Code

Adds PostHogSDK.shared.displaySurvey(surveyId), the mobile counterpart of the web SDK's posthog.displaySurvey(). It looks the survey up in the loaded surveys and routes it through the existing display 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
@bs1180
bs1180 requested review from ioannisj and turnipdabeets July 20, 2026 13:20

bs1180 commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

Hi @ioannisj @turnipdabeets 👋 — tagging you both since you maintain the surveys code here. This adds the manual displaySurvey API tracked in #648, as the first piece needed for Flutter support (PostHog/posthog-flutter#225). No rush at all, and happy to adjust the API shape (e.g. add an ignoreConditions option or restrict allowed survey types) if you'd prefer something different. Thanks for taking a look!

@github-actions

Copy link
Copy Markdown
Contributor

posthog-ios Compliance Report

Date: 2026-07-20 13:31:18 UTC
Duration: 228555ms

⚠️ Some Tests Failed

44/45 tests passed, 1 failed


Capture Tests

⚠️ 28/29 tests passed, 1 failed

View Details
Test Status Duration
Format Validation.Event Has Required Fields 4364ms
Format Validation.Event Has Uuid 2823ms
Format Validation.Event Has Lib Properties 3109ms
Format Validation.Distinct Id Is String 3332ms
Format Validation.Token Is Present 3228ms
Format Validation.Custom Properties Preserved 2814ms
Format Validation.Event Has Timestamp 443ms
Retry Behavior.Retries On 503 11790ms
Retry Behavior.Does Not Retry On 400 4892ms
Retry Behavior.Does Not Retry On 401 5003ms
Retry Behavior.Respects Retry After Header 7825ms
Retry Behavior.Implements Backoff 15566ms
Retry Behavior.Retries On 500 8835ms
Retry Behavior.Retries On 502 8866ms
Retry Behavior.Retries On 504 9305ms
Retry Behavior.Max Retries Respected 21768ms
Deduplication.Generates Unique Uuids 2962ms
Deduplication.Preserves Uuid On Retry 9270ms
Deduplication.Preserves Uuid And Timestamp On Retry 16538ms
Deduplication.Preserves Uuid And Timestamp On Batch Retry 8013ms
Deduplication.No Duplicate Events In Batch 3349ms
Deduplication.Different Events Have Different Uuids 546ms
Compression.Sends Gzip When Enabled 663ms
Batch Format.Uses Proper Batch Structure 2960ms
Batch Format.Flush With No Events Sends Nothing 437ms
Batch Format.Multiple Events Batched Together 3253ms
Error Handling.Does Not Retry On 403 4900ms
Error Handling.Does Not Retry On 413 4866ms
Error Handling.Retries On 408 9207ms

Failures

deduplication.different_events_have_different_uuids

Need at least 2 events

Feature_Flags Tests

16/16 tests passed

View Details
Test Status Duration
Request Payload.Request With Person Properties Device Id 2848ms
Request Payload.Flags Request Uses V2 Query Param 2832ms
Request Payload.Flags Request Hits Flags Path Not Decide 2783ms
Request Payload.Flags Request Omits Authorization Header 2782ms
Request Payload.Token In Flags Body Matches Init 2850ms
Request Payload.Groups Round Trip 2911ms
Request Payload.Groups Default To Empty Object 2829ms
Request Payload.Person Properties Distinct Id Auto Populated When Caller Omits It 2770ms
Request Payload.Disable Geoip False Propagates As Geoip Disable False 2764ms
Request Payload.Disable Geoip Omitted Defaults To False 2684ms
Request Payload.Flag Keys To Evaluate Contains Only Requested Key 2822ms
Request Lifecycle.No Flags Request On Init Alone 67ms
Request Lifecycle.No Flags Request On Normal Capture 2887ms
Request Lifecycle.Two Flag Calls Produce Two Remote Requests 5371ms
Request Lifecycle.Mock Response Value Is Returned To Caller 2935ms
Side Effect Events.Get Feature Flag Captures Feature Flag Called Event 2913ms

@posthog

posthog Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

🦔 ReviewHog reviewed this pull request

Found 0 must fix, 1 should fix, 3 consider.

Published 4 findings (view the review).

@posthog

posthog Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏

@posthog posthog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ReviewHog Report

Changes

Issues: 2 issues

Files (5)
  • .changeset/display-survey-manual-api.md
  • PostHog.xcodeproj/project.pbxproj
  • PostHog/PostHogSDK.swift
  • PostHog/Surveys/PostHogSurveyIntegration.swift
  • PostHogTests/PostHogSurveyDisplaySurveyTest.swift

Comment on lines +310 to +331
func displaySurvey(surveyId: String) {
#if os(iOS)
guard #available(iOS 15.0, *) else {
hedgeLog("[Surveys] Surveys can be rendered only on iOS 15+")
return
}
#endif

guard canShowNextSurvey() else {
hedgeLog("[Surveys] Cannot display survey \(surveyId) - another survey is already being displayed")
return
}

getSurveys { [weak self] surveys in
guard let self else { return }
guard let survey = surveys.first(where: { $0.id == surveyId }) else {
hedgeLog("[Surveys] Cannot display survey \(surveyId) - survey not found")
return
}
self.presentSurvey(survey)
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

displaySurvey doesn't verify the survey is running, contradicting its documented contract

must_fix bug

Why we think it's a valid issue
  • Checked: getSurveys (PostHogSurveyIntegration.swift:166-189) and its terminal decodeAndSetSurveys (231-252) — they call back with the raw decoded loadedSurveys and apply no active/running filter. Compared against getActiveMatchingSurveys (98-143) and displaySurvey (310-331).
  • Found: getActiveMatchingSurveys applies .filter(\.isActive) at line 110; displaySurvey looks the survey up with surveys.first(where: { $0.id == surveyId }) at line 325 and calls presentSurvey directly at 329, never checking isActive. isActive is startDate != nil && endDate == nil (987-989). The public doc for displaySurvey states "The survey must be running and present in the project's surveys" (PostHogSDK.swift:2474) — the "present" half is enforced by the not-found guard (326), the "running" half is not enforced at all.
  • Found: The surveys pipeline reference confirms the backend (SurveyViewSet) returns ALL non-archived surveys and does no server-side eligibility filtering — stopping a survey sets end_date without archiving, so stopped surveys stay in the cached payload. isSurveyRunning (has start_date, no end_date) is the web SDK's first eligibility gate, a baseline check distinct from the display conditions (targeting_flag, event triggers, seen/wait) that displaySurvey intentionally bypasses. isActive here is the exact iOS equivalent.
  • Impact: Confirmed reachable, not speculative: calling displaySurvey(surveyId:) with the ID of a survey that has been stopped on the dashboard (end_date set) renders it and collects survey shown/sent responses, violating the method's own documented "must be running" precondition and diverging from the running/active baseline every other SDK enforces. Contract break + correctness defect on a brand-new public API, with a one-line fix that mirrors the existing automatic path.
Issue description

The public doc comment on PostHogSDK.displaySurvey (PostHog/PostHogSDK.swift:2474) explicitly states "The survey must be running", and the PR description enumerates exactly which conditions are bypassed — targeting flags, event triggers, and seen/wait-period checks — deliberately not including the survey's running/active state. However, PostHogSurveyIntegration.displaySurvey(surveyId:) looks the survey up directly in the raw, unfiltered getSurveys result (line 325: surveys.first(where: { $0.id == surveyId })) and never applies the isActive check (startDate != nil && endDate == nil, defined at line 987) that getActiveMatchingSurveys applies for automatic display (line 110: .filter(\.isActive)). That client-side filter only exists because the remote config payload can contain surveys that have already ended (endDate set) — otherwise there would be no need to filter it out for the automatic path. As written, calling displaySurvey(surveyId:) with the ID of a survey that has been stopped/archived on the PostHog dashboard (e.g. pulled for compliance, legal, or content-accuracy reasons) will still render it and collect responses, directly contradicting the documented contract and the intent of the automatic-display pipeline it otherwise mirrors.

Suggested fix

Apply the same survey.isActive check used by getActiveMatchingSurveys before presenting in displaySurvey, e.g.:

guard let survey = surveys.first(where: { $0.id == surveyId }) else {
    hedgeLog("[Surveys] Cannot display survey \(surveyId) - survey not found")
    return
}
guard survey.isActive else {
    hedgeLog("[Surveys] Cannot display survey \(surveyId) - survey is not running")
    return
}
self.presentSurvey(survey)

This keeps the intentionally-bypassed conditions (targeting flags, event triggers, wait period, seen state) bypassed while still honoring the documented "must be running" precondition, and add a test case asserting that a survey with an endDate set is rejected by displaySurvey.

Prompt to fix with AI (copy-paste)
## Context
@PostHog/Surveys/PostHogSurveyIntegration.swift#L310-331

<issue_description>
The public doc comment on `PostHogSDK.displaySurvey` (PostHog/PostHogSDK.swift:2474) explicitly states "The survey must be running", and the PR description enumerates exactly which conditions are bypassed — targeting flags, event triggers, and seen/wait-period checks — deliberately not including the survey's running/active state. However, `PostHogSurveyIntegration.displaySurvey(surveyId:)` looks the survey up directly in the raw, unfiltered `getSurveys` result (line 325: `surveys.first(where: { $0.id == surveyId })`) and never applies the `isActive` check (`startDate != nil && endDate == nil`, defined at line 987) that `getActiveMatchingSurveys` applies for automatic display (line 110: `.filter(\.isActive)`). That client-side filter only exists because the remote config payload can contain surveys that have already ended (endDate set) — otherwise there would be no need to filter it out for the automatic path. As written, calling `displaySurvey(surveyId:)` with the ID of a survey that has been stopped/archived on the PostHog dashboard (e.g. pulled for compliance, legal, or content-accuracy reasons) will still render it and collect responses, directly contradicting the documented contract and the intent of the automatic-display pipeline it otherwise mirrors.
</issue_description>

<issue_validation>
- **Checked:** `getSurveys` (PostHogSurveyIntegration.swift:166-189) and its terminal `decodeAndSetSurveys` (231-252) — they call back with the raw decoded `loadedSurveys` and apply no active/running filter. Compared against `getActiveMatchingSurveys` (98-143) and `displaySurvey` (310-331).
- **Found:** `getActiveMatchingSurveys` applies `.filter(\.isActive)` at line 110; `displaySurvey` looks the survey up with `surveys.first(where: { $0.id == surveyId })` at line 325 and calls `presentSurvey` directly at 329, never checking `isActive`. `isActive` is `startDate != nil && endDate == nil` (987-989). The public doc for `displaySurvey` states "The survey must be running and present in the project's surveys" (PostHogSDK.swift:2474) — the "present" half is enforced by the not-found guard (326), the "running" half is not enforced at all.
- **Found:** The surveys pipeline reference confirms the backend (`SurveyViewSet`) returns ALL non-archived surveys and does no server-side eligibility filtering — stopping a survey sets `end_date` without archiving, so stopped surveys stay in the cached payload. `isSurveyRunning` (has `start_date`, no `end_date`) is the web SDK's *first* eligibility gate, a baseline check distinct from the display conditions (`targeting_flag`, event triggers, seen/wait) that `displaySurvey` intentionally bypasses. `isActive` here is the exact iOS equivalent.
- **Impact:** Confirmed reachable, not speculative: calling `displaySurvey(surveyId:)` with the ID of a survey that has been stopped on the dashboard (end_date set) renders it and collects `survey shown`/`sent` responses, violating the method's own documented "must be running" precondition and diverging from the running/active baseline every other SDK enforces. Contract break + correctness defect on a brand-new public API, with a one-line fix that mirrors the existing automatic path.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Apply the same `survey.isActive` check used by `getActiveMatchingSurveys` before presenting in `displaySurvey`, e.g.:
```swift
guard let survey = surveys.first(where: { $0.id == surveyId }) else {
    hedgeLog("[Surveys] Cannot display survey \(surveyId) - survey not found")
    return
}
guard survey.isActive else {
    hedgeLog("[Surveys] Cannot display survey \(surveyId) - survey is not running")
    return
}
self.presentSurvey(survey)

This keeps the intentionally-bypassed conditions (targeting flags, event triggers, wait period, seen state) bypassed while still honoring the documented "must be running" precondition, and add a test case asserting that a survey with an endDate set is rejected by displaySurvey.
</potential_solution>


</details>

Comment on lines +310 to +331
func displaySurvey(surveyId: String) {
#if os(iOS)
guard #available(iOS 15.0, *) else {
hedgeLog("[Surveys] Surveys can be rendered only on iOS 15+")
return
}
#endif

guard canShowNextSurvey() else {
hedgeLog("[Surveys] Cannot display survey \(surveyId) - another survey is already being displayed")
return
}

getSurveys { [weak self] surveys in
guard let self else { return }
guard let survey = surveys.first(where: { $0.id == surveyId }) else {
hedgeLog("[Surveys] Cannot display survey \(surveyId) - survey not found")
return
}
self.presentSurvey(survey)
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

TOCTOU race between the active-survey check and claim lets a rendered survey diverge from tracked activeSurvey, permanently blocking all future surveys

must_fix bug

Why we think it's a valid issue
  • Checked: setActiveSurvey (751-762), presentSurvey (334-356), canShowNextSurvey (970-972), the three handle* rejection guards (482/509/576), clearActiveSurvey callers (grep: only 595 in production + a TESTING helper at 1111), the default delegate renderSurvey (PostHogSurveysDefaultDelegate.swift:23-50), and the trigger/threading model.
  • Found: setActiveSurvey writes only if activeSurvey == nil (753) under the lock, but presentSurvey calls it at 337 and then unconditionally calls renderSurvey at 344 — the return of the claim is never inspected. The pre-check canShowNextSurvey() in both displaySurvey (318) and showNextSurvey (293) is a separate lock acquisition, with an async getSurveys hop before the claim, so it is a genuine check-then-act across two locks.
  • Found: the divergence is not self-healing: handleSurveyClosed only reaches clearActiveSurvey() (595) when survey.id == activeSurvey.id (576); the default delegate overwrites its close handler (46) and re-renders without guarding, so the loser's survey ends up on screen while activeSurvey holds the winner, whose close event never arrives → activeSurvey never cleared → canShowNextSurvey() returns false for the rest of the process (in-memory state, no timeout, cleanupSurveys doesn't reset it).
  • Impact: Confirmed reachable and severe: a background-thread @objc displaySurvey call (the project mandates any-thread public APIs) overlapping a main-thread automatic trigger, or a startup double-load where two getSurveys cache-miss callbacks land on network threads, lets both flows pass the nil-check before either claims — one renders an untracked survey and permanently, silently disables ALL surveys until app restart. Race corrupting shared state with a silent, unrecoverable, hard-to-diagnose consequence and a cheap, clean fix (render only on a successful atomic claim) — squarely in the keep bar. The window is narrow and the most common usage (main-thread call with surveys already cached) is race-free, but at scale this will occur for some users; the reviewer named both the concrete trigger and the concrete consequence accurately.
Issue description

displaySurvey(surveyId:) calls canShowNextSurvey() (a synchronous read of activeSurvey == nil) and only later, after the async getSurveys round trip, calls presentSurvey(_:), which calls setActiveSurvey(...). setActiveSurvey (line 751) is a 'set only if nil' operation - if another call already claimed the slot, it silently no-ops - but presentSurvey never checks whether its own claim succeeded before unconditionally proceeding to invoke self.postHog?.config.surveysConfig.surveysDelegate.renderSurvey(survey.toDisplaySurvey(...), ...) on line 344. The automatic path showNextSurvey() (line 286) has the exact same check-then-act shape and calls the same shared presentSurvey. Since displaySurvey is a new public, @objc entry point that can be invoked from any thread (per this project's 'all public APIs should be callable from any thread' requirement) while showNextSurvey() is independently triggered from view-layout throttling, app-become-active, and captured-event callbacks, two calls can now race: both pass canShowNextSurvey() before either calls setActiveSurvey, the first call's claim wins, but the second call still renders its survey via the delegate. From then on, handleSurveyShown/handleSurveyResponse/handleSurveyClosed all compare the survey that's actually on screen against the wrong tracked activeSurvey and reject it ('Received a show/response/close event for a non-active survey'), so clearActiveSurvey() is never called for the survey that actually holds the slot. canShowNextSurvey() then returns false for the remainder of the process lifetime, silently disabling all surveys (both automatic and via displaySurvey) until the app is restarted. This is a genuine reliability regression introduced by promoting the previously-inline, single-caller logic into a shared presentSurvey now reachable from two independent, potentially concurrent triggers, and it is not covered by the new test suite (displaySurveyIgnoredWhileAnotherSurveyActive only exercises the case where a survey is already active before the call, not the async race).

Suggested fix

Make the claim-and-render atomic instead of check-then-act-later: have setActiveSurvey (or a new tryClaimActiveSurvey) return a Bool indicating whether it actually won the slot, and only call the delegate's renderSurvey when the claim succeeded; otherwise log and return, mirroring the existing 'another survey is already being displayed' log. For example:

private func presentSurvey(_ survey: PostHogSurvey) {
    let translations = ...
    guard claimActiveSurvey(survey: survey, language: translations.matchedKey, questionTranslations: translations.questions) else {
        hedgeLog("[Surveys] Cannot display survey \(survey.id) - another survey is already being displayed")
        return
    }
    // proceed to render only on a successful claim
}

where claimActiveSurvey performs the nil-check and assignment under activeSurveyLock and returns whether it wrote. This removes the redundant, racy canShowNextSurvey() pre-check in both displaySurvey and showNextSurvey and closes the window entirely.

Prompt to fix with AI (copy-paste)
## Context
@PostHog/Surveys/PostHogSurveyIntegration.swift#L310-331
@PostHog/Surveys/PostHogSurveyIntegration.swift#L333-356

<issue_description>
`displaySurvey(surveyId:)` calls `canShowNextSurvey()` (a synchronous read of `activeSurvey == nil`) and only later, after the async `getSurveys` round trip, calls `presentSurvey(_:)`, which calls `setActiveSurvey(...)`. `setActiveSurvey` (line 751) is a 'set only if nil' operation - if another call already claimed the slot, it silently no-ops - but `presentSurvey` never checks whether its own claim succeeded before unconditionally proceeding to invoke `self.postHog?.config.surveysConfig.surveysDelegate.renderSurvey(survey.toDisplaySurvey(...), ...)` on line 344. The automatic path `showNextSurvey()` (line 286) has the exact same check-then-act shape and calls the same shared `presentSurvey`. Since `displaySurvey` is a new public, `@objc` entry point that can be invoked from any thread (per this project's 'all public APIs should be callable from any thread' requirement) while `showNextSurvey()` is independently triggered from view-layout throttling, app-become-active, and captured-event callbacks, two calls can now race: both pass `canShowNextSurvey()` before either calls `setActiveSurvey`, the first call's claim wins, but the second call still renders its survey via the delegate. From then on, `handleSurveyShown`/`handleSurveyResponse`/`handleSurveyClosed` all compare the survey that's actually on screen against the wrong tracked `activeSurvey` and reject it ('Received a show/response/close event for a non-active survey'), so `clearActiveSurvey()` is never called for the survey that actually holds the slot. `canShowNextSurvey()` then returns `false` for the remainder of the process lifetime, silently disabling all surveys (both automatic and via `displaySurvey`) until the app is restarted. This is a genuine reliability regression introduced by promoting the previously-inline, single-caller logic into a shared `presentSurvey` now reachable from two independent, potentially concurrent triggers, and it is not covered by the new test suite (`displaySurveyIgnoredWhileAnotherSurveyActive` only exercises the case where a survey is already active *before* the call, not the async race).
</issue_description>

<issue_validation>
- **Checked:** `setActiveSurvey` (751-762), `presentSurvey` (334-356), `canShowNextSurvey` (970-972), the three `handle*` rejection guards (482/509/576), `clearActiveSurvey` callers (grep: only 595 in production + a TESTING helper at 1111), the default delegate `renderSurvey` (PostHogSurveysDefaultDelegate.swift:23-50), and the trigger/threading model.
- **Found:** `setActiveSurvey` writes only `if activeSurvey == nil` (753) under the lock, but `presentSurvey` calls it at 337 and then unconditionally calls `renderSurvey` at 344 — the return of the claim is never inspected. The pre-check `canShowNextSurvey()` in both `displaySurvey` (318) and `showNextSurvey` (293) is a separate lock acquisition, with an async `getSurveys` hop before the claim, so it is a genuine check-then-act across two locks.
- **Found:** the divergence is not self-healing: `handleSurveyClosed` only reaches `clearActiveSurvey()` (595) when `survey.id == activeSurvey.id` (576); the default delegate overwrites its close handler (46) and re-renders without guarding, so the loser's survey ends up on screen while `activeSurvey` holds the winner, whose close event never arrives → `activeSurvey` never cleared → `canShowNextSurvey()` returns false for the rest of the process (in-memory state, no timeout, `cleanupSurveys` doesn't reset it).
- **Impact:** Confirmed reachable and severe: a background-thread `@objc displaySurvey` call (the project mandates any-thread public APIs) overlapping a main-thread automatic trigger, or a startup double-load where two `getSurveys` cache-miss callbacks land on network threads, lets both flows pass the nil-check before either claims — one renders an untracked survey and permanently, silently disables ALL surveys until app restart. Race corrupting shared state with a silent, unrecoverable, hard-to-diagnose consequence and a cheap, clean fix (render only on a successful atomic claim) — squarely in the keep bar. The window is narrow and the most common usage (main-thread call with surveys already cached) is race-free, but at scale this will occur for some users; the reviewer named both the concrete trigger and the concrete consequence accurately.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Make the claim-and-render atomic instead of check-then-act-later: have `setActiveSurvey` (or a new `tryClaimActiveSurvey`) return a Bool indicating whether it actually won the slot, and only call the delegate's `renderSurvey` when the claim succeeded; otherwise log and return, mirroring the existing 'another survey is already being displayed' log. For example:
```swift
private func presentSurvey(_ survey: PostHogSurvey) {
    let translations = ...
    guard claimActiveSurvey(survey: survey, language: translations.matchedKey, questionTranslations: translations.questions) else {
        hedgeLog("[Surveys] Cannot display survey \(survey.id) - another survey is already being displayed")
        return
    }
    // proceed to render only on a successful claim
}

where claimActiveSurvey performs the nil-check and assignment under activeSurveyLock and returns whether it wrote. This removes the redundant, racy canShowNextSurvey() pre-check in both displaySurvey and showNextSurvey and closes the window entirely.
</potential_solution>


</details>

@github-actions

Copy link
Copy Markdown
Contributor

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.

@github-actions github-actions Bot added the stale label Jul 28, 2026
@bs1180
bs1180 marked this pull request as ready for review July 30, 2026 09:57
@bs1180
bs1180 requested a review from a team as a code owner July 30, 2026 09:57
Address ReviewHog feedback on the manual displaySurvey API:

- Honor the documented "must be running" precondition by checking
  `survey.isActive` before display, so a survey stopped on the dashboard
  (endDate set) is no longer rendered.
- Make the active-survey claim atomic: `setActiveSurvey` now returns
  whether it won the slot, and `presentSurvey` only renders on a
  successful claim. This closes a TOCTOU race between the pre-check and
  the claim that could render an untracked survey and permanently block
  all future surveys.

Adds a test covering the stopped-survey no-op.

Generated-By: PostHog Code
Task-Id: 33962449-be98-4944-85da-0a31ebad692b
@ioannisj ioannisj removed the stale label Jul 30, 2026
@turnipdabeets
turnipdabeets requested a review from a team July 30, 2026 14:06

@turnipdabeets turnipdabeets left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Two notes without a diff anchor: CI never ran on the head commit (only the Wiz checks fired) — it needs a re-trigger before merge; and the two ReviewHog threads have no reply even though both fixes are in 3a94107 — a one-liner on each would save reviewers re-deriving that.

hedgeLog("[Surveys] Cannot display survey \(surveyId) - survey is not running")
return
}
self.presentSurvey(survey)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[suggestion] Bypassing canRenderSurvey is what enables .api — but it also lets .unknown(type:) through to a popover UI built for other types (canRenderSurvey's own comment says popover-only for now). Could we allow-list .popover/.api/.widget and no-op with a log on unknown? Whether .widget should render as a popover here feels like a Team Surveys call, but failing closed on unknown seems right either way.

@posthog posthog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ReviewHog Report

Changes

Issues: 4 issues

Files (5)
  • .changeset/display-survey-manual-api.md
  • PostHog.xcodeproj/project.pbxproj
  • PostHog/PostHogSDK.swift
  • PostHog/Surveys/PostHogSurveyIntegration.swift
  • PostHogTests/PostHogSurveyDisplaySurveyTest.swift

Comment on lines 297 to +338
if let survey = activeSurveys.first(where: self.canRenderSurvey) {
let language = self.resolveDisplayLanguage()
let translations = resolveSurveyTranslations(survey: survey, targetLanguage: language)
self.setActiveSurvey(survey: survey, language: translations.matchedKey, questionTranslations: translations.questions)

DispatchQueue.main.async { [weak self] in
if let self {
// render the survey
self.postHog?.config.surveysConfig.surveysDelegate.renderSurvey(
survey.toDisplaySurvey(
surveyTranslation: translations.survey,
questionTranslations: translations.questions
),
onSurveyShown: self.handleSurveyShown,
onSurveyResponse: self.handleSurveyResponse,
onSurveyClosed: self.handleSurveyClosed
)
}
}
self.presentSurvey(survey)
}
}
#endif
}

/// Displays the survey with the given ID on demand.
///
/// Unlike automatic display, this bypasses display conditions (targeting flags, event
/// triggers, and the seen/wait-period checks), so it also works for API-type surveys,
/// which are never auto-displayed. If another survey is already being displayed, the
/// call is ignored.
func displaySurvey(surveyId: String) {
#if os(iOS)
guard #available(iOS 15.0, *) else {
hedgeLog("[Surveys] Surveys can be rendered only on iOS 15+")
return
}
#endif

guard canShowNextSurvey() else {
hedgeLog("[Surveys] Cannot display survey \(surveyId) - another survey is already being displayed")
return
}

getSurveys { [weak self] surveys in
guard let self else { return }
guard let survey = surveys.first(where: { $0.id == surveyId }) else {
hedgeLog("[Surveys] Cannot display survey \(surveyId) - survey not found")
return
}
// Honor the documented "must be running" precondition. We deliberately bypass
// targeting flags, event triggers, and seen/wait-period checks, but a survey
// that has been stopped on the dashboard (endDate set) must not be shown.
guard survey.isActive else {
hedgeLog("[Surveys] Cannot display survey \(surveyId) - survey is not running")
return
}
self.presentSurvey(survey)
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

displaySurvey bypasses the popover-only rendering gate (canRenderSurvey), letting widget/unknown survey types reach a popover-only delegate

should_fix bug

Why we think it's a valid issue
  • Checked: PR-head displaySurvey(surveyId:) (310–338), showNextSurvey (285–301), canRenderSurvey (280–282), getSurveys (166–188), the PostHogSurveyType enum (PostHog/Models/Surveys/PostHogSurveyEnums.swift:22–26), and the default delegate PostHogSurveysDefaultDelegate.renderSurvey (PostHog/Surveys/PostHogSurveysDefaultDelegate.swift:23–48).
  • Found: all premises hold. showNextSurvey gates via activeSurveys.first(where: self.canRenderSurvey) (297); canRenderSurvey is survey.type == .popover (comment: 'only render popover surveys for now', 280–282). displaySurvey(surveyId:) resolves by ID and applies only canShowNextSurvey() + guard survey.isActive, then calls presentSurvey(survey) with no type check. getSurveys returns the full decoded list unfiltered, so .widget/.unknown surveys are reachable. The default delegate renderSurvey has no type branching — it unconditionally schedules a full-screen popover (scheduleSurveyDisplay, line 48) for any PostHogDisplaySurvey.
  • Found: the pre-existing canRenderSurvey gate cannot be reused verbatim because enabling .api display is the feature's whole point (.api would fail == .popover); a correct fix must widen the allow-list to .popover/.api and reject .widget/.unknown, as the reviewer acknowledges.
  • Impact: real, directly PR-introduced. presentSurvey became a shared choke point reachable from two entry points, but the 'only popover reaches the renderer' invariant (which the popover-only default delegate relies on) is enforced by only one. A .widget/.unknown survey passed to displaySurvey renders as a full-screen takeover popover instead of being rejected — concrete trigger, concrete consequence — with a forward-compat angle (.unknown types from a newer server silently rendered as popovers). Meets the keep bar (mishandled edge case real input can hit + bypassed invariant).
  • Priority: lowering must_fix → should_fix. The trigger requires an explicit displaySurvey call with a non-popover/non-api survey ID (widget is essentially a web display mode, so an unusual mobile input), and the outcome is a mis-presented survey — no crash, data loss, or security impact. Legitimate and cheap to fix, but not blocking-severe.
Issue description

In showNextSurvey, only surveys passing canRenderSurvey (survey.type == .popover) are ever handed to the render path (line 297: activeSurveys.first(where: self.canRenderSurvey)). displaySurvey(surveyId:) resolves the survey purely by ID via getSurveys and calls self.presentSurvey(survey) directly, with no equivalent type check. PostHogSurveyType (PostHog/Models/Surveys/PostHogSurveyEnums.swift:22-26) also defines .widget and .unknown(type:) cases besides .popover/.api, and the default surveysDelegate implementation (PostHogSurveysDefaultDelegate.renderSurvey) has no type branching either — it unconditionally builds a full-screen popover (SurveyDisplayController/scheduleSurveyDisplay) for whatever PostHogDisplaySurvey it receives. canRenderSurvey used to be the single gate every survey passed through before reaching the render call; now that presentSurvey is a shared choke point reachable from two independent entry points, that invariant is enforced by only one of them. A dashboard-created .widget-type survey (meant for a feedback-widget embed, not a takeover popup) passed to displaySurvey(surveyId:) would render as a full-screen popover instead of being rejected or left alone.

Suggested fix

Move the renderability check into presentSurvey itself (possibly widened to allow .api alongside .popover, since enabling API-type display is this feature's whole point) so the invariant lives once in the shared choke point instead of being re-derived per caller. Alternatively, add the same canRenderSurvey-style guard to displaySurvey and log a clear 'survey type not supported for on-demand display' message rather than silently handing non-popover surveys to a popover-only delegate.

Prompt to fix with AI (copy-paste)
## Context
@PostHog/Surveys/PostHogSurveyIntegration.swift#L297-338

<issue_description>
In `showNextSurvey`, only surveys passing `canRenderSurvey` (`survey.type == .popover`) are ever handed to the render path (line 297: `activeSurveys.first(where: self.canRenderSurvey)`). `displaySurvey(surveyId:)` resolves the survey purely by ID via `getSurveys` and calls `self.presentSurvey(survey)` directly, with no equivalent type check. `PostHogSurveyType` (PostHog/Models/Surveys/PostHogSurveyEnums.swift:22-26) also defines `.widget` and `.unknown(type:)` cases besides `.popover`/`.api`, and the default `surveysDelegate` implementation (`PostHogSurveysDefaultDelegate.renderSurvey`) has no type branching either — it unconditionally builds a full-screen popover (`SurveyDisplayController`/`scheduleSurveyDisplay`) for whatever `PostHogDisplaySurvey` it receives. `canRenderSurvey` used to be the single gate every survey passed through before reaching the render call; now that `presentSurvey` is a shared choke point reachable from two independent entry points, that invariant is enforced by only one of them. A dashboard-created `.widget`-type survey (meant for a feedback-widget embed, not a takeover popup) passed to `displaySurvey(surveyId:)` would render as a full-screen popover instead of being rejected or left alone.
</issue_description>

<issue_validation>
- **Checked:** PR-head `displaySurvey(surveyId:)` (310–338), `showNextSurvey` (285–301), `canRenderSurvey` (280–282), `getSurveys` (166–188), the `PostHogSurveyType` enum (`PostHog/Models/Surveys/PostHogSurveyEnums.swift:22–26`), and the default delegate `PostHogSurveysDefaultDelegate.renderSurvey` (`PostHog/Surveys/PostHogSurveysDefaultDelegate.swift:23–48`).
- **Found:** all premises hold. `showNextSurvey` gates via `activeSurveys.first(where: self.canRenderSurvey)` (297); `canRenderSurvey` is `survey.type == .popover` (comment: 'only render popover surveys for now', 280–282). `displaySurvey(surveyId:)` resolves by ID and applies only `canShowNextSurvey()` + `guard survey.isActive`, then calls `presentSurvey(survey)` with no type check. `getSurveys` returns the full decoded list unfiltered, so `.widget`/`.unknown` surveys are reachable. The default delegate `renderSurvey` has no type branching — it unconditionally schedules a full-screen popover (`scheduleSurveyDisplay`, line 48) for any `PostHogDisplaySurvey`.
- **Found:** the pre-existing `canRenderSurvey` gate cannot be reused verbatim because enabling `.api` display is the feature's whole point (`.api` would fail `== .popover`); a correct fix must widen the allow-list to `.popover`/`.api` and reject `.widget`/`.unknown`, as the reviewer acknowledges.
- **Impact:** real, directly PR-introduced. `presentSurvey` became a shared choke point reachable from two entry points, but the 'only popover reaches the renderer' invariant (which the popover-only default delegate relies on) is enforced by only one. A `.widget`/`.unknown` survey passed to `displaySurvey` renders as a full-screen takeover popover instead of being rejected — concrete trigger, concrete consequence — with a forward-compat angle (`.unknown` types from a newer server silently rendered as popovers). Meets the keep bar (mishandled edge case real input can hit + bypassed invariant).
- **Priority:** lowering must_fix → should_fix. The trigger requires an explicit `displaySurvey` call with a non-popover/non-api survey ID (widget is essentially a web display mode, so an unusual mobile input), and the outcome is a mis-presented survey — no crash, data loss, or security impact. Legitimate and cheap to fix, but not blocking-severe.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Move the renderability check into `presentSurvey` itself (possibly widened to allow `.api` alongside `.popover`, since enabling API-type display is this feature's whole point) so the invariant lives once in the shared choke point instead of being re-derived per caller. Alternatively, add the same `canRenderSurvey`-style guard to `displaySurvey` and log a clear 'survey type not supported for on-demand display' message rather than silently handing non-popover surveys to a popover-only delegate.
</potential_solution>

Comment thread PostHog/PostHogSDK.swift
Comment on lines +2478 to +2493

This method will have no effect if PostHog is not enabled, or if surveys are disabled in the SDK configuration.

- Parameter surveyId: The ID of the survey to display
*/
@objc public func displaySurvey(_ surveyId: String) {
if !isEnabled() {
return
}

guard let surveysIntegration else {
return hedgeLog("Cannot display survey \(surveyId) - surveys integration is not installed.")
}

surveysIntegration.displaySurvey(surveyId: surveyId)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PostHogSDK.displaySurvey has no fallback when the surveys integration isn't installed, unlike the established lazy-install precedent for on-demand replay APIs

consider best_practice

Why we think it's a valid issue
  • Checked: PR-head PostHogSDK.swift — the displaySurvey nil guard, installIntegrations() (2738–2775), installReplayIntegration() (2779–2793), startSessionRecording(resumeCurrent:) (2387–2401), and requiresSwizzling across the three integrations.
  • Found: all premises hold. PostHogSurveyIntegration.requiresSwizzling is hardcoded true (PostHog/Surveys/PostHogSurveyIntegration.swift:16); installIntegrations() does if integration.requiresSwizzling, !config.enableSwizzling { continue } (2749–2750), so with enableSwizzling=false the surveys integration is never installed → surveysIntegration stays nil → displaySurvey is a logged no-op. startSessionRecording(resumeCurrent:) self-heals via if replayIntegration == nil { installReplayIntegration() } (2397–2401), and installReplayIntegration() installs the integration directly, bypassing the swizzling gate — the reviewer's precedent parallel is exact. The manual survey path (getSurveys via remote config → presentSurvey → delegate renderSurvey over a dedicated SurveysWindow) does not use the swizzled didLayoutViewToken/didBecomeActiveToken auto-trigger hooks, so 'manual display doesn't need swizzling' is correct.
  • Found: the suggestion partly overreaches — it proposes lazy-install even when surveys are disabled at config, which would wrongly override an explicit config._surveys = false. Only the swizzling case is the legitimate gap.
  • Impact: real, accurately-diagnosed functional gap: an app with enableSwizzling=false (surveys enabled) cannot use the new on-demand API at all, contradicting the feature's stated purpose, and the repo already has a self-healing precedent for exactly this. Not noise — a substantive design/reliability observation a maintainer would want to decide on consciously.
  • Priority: lowering should_fix → consider. Trigger is strictly the non-default enableSwizzling=false config; the failure degrades gracefully (logged no-op, no crash/data loss); and whether the swizzling gate should apply to the manual path is a defensible design-judgment call (swizzling-off ⇒ surveys-off, consistent with autocapture/screen-views). Worth recording, but below should-fix urgency.
Issue description

PostHogSDK.displaySurvey(_:) gives up and only logs when surveysIntegration is nil: guard let surveysIntegration else { return hedgeLog("Cannot display survey \(surveyId) - surveys integration is not installed.") }. PostHogSurveyIntegration is only appended during installIntegrations() when config._surveys is true, and even then it's skipped entirely if config.enableSwizzling is false (since PostHogSurveyIntegration.requiresSwizzling is hardcoded true and installIntegrations() skips any swizzling-requiring integration when swizzling is disabled). So surveysIntegration stays permanently nil for any app that disabled swizzling, even though nothing about the new manual displaySurvey mechanism (an ID lookup plus a render call) actually depends on swizzling — only the automatic triggers wired up in start() (didLayoutViewToken/didBecomeActiveToken) do. This diverges from the precedent this same file already set for the conceptually identical 'user explicitly asks to trigger an optional integration on demand' scenario: startSessionRecording(resumeCurrent:) (lines 2438-2453) actively calls installReplayIntegration() to install PostHogReplayIntegration on the spot when it's missing, rather than silently failing. The new manual survey-display API — whose entire stated purpose is to work even when automatic-trigger conditions aren't met — is the one built without that self-healing behavior.

Suggested fix

Either give PostHogSurveyIntegration a requiresSwizzling value that reflects that only its automatic-trigger wiring needs swizzling (not the manual displaySurvey path), or mirror installReplayIntegration()'s pattern inside PostHogSDK.displaySurvey to lazily install the surveys integration on demand when it's missing, so the new API isn't silently unusable for any app that has disabled swizzling (or surveys) at config time.

Prompt to fix with AI (copy-paste)
## Context
@PostHog/PostHogSDK.swift#L2478-2493

<issue_description>
`PostHogSDK.displaySurvey(_:)` gives up and only logs when `surveysIntegration` is nil: `guard let surveysIntegration else { return hedgeLog("Cannot display survey \(surveyId) - surveys integration is not installed.") }`. `PostHogSurveyIntegration` is only appended during `installIntegrations()` when `config._surveys` is true, and even then it's skipped entirely if `config.enableSwizzling` is false (since `PostHogSurveyIntegration.requiresSwizzling` is hardcoded `true` and `installIntegrations()` skips any swizzling-requiring integration when swizzling is disabled). So `surveysIntegration` stays permanently nil for any app that disabled swizzling, even though nothing about the new manual `displaySurvey` mechanism (an ID lookup plus a render call) actually depends on swizzling — only the *automatic* triggers wired up in `start()` (`didLayoutViewToken`/`didBecomeActiveToken`) do. This diverges from the precedent this same file already set for the conceptually identical 'user explicitly asks to trigger an optional integration on demand' scenario: `startSessionRecording(resumeCurrent:)` (lines 2438-2453) actively calls `installReplayIntegration()` to install `PostHogReplayIntegration` on the spot when it's missing, rather than silently failing. The new manual survey-display API — whose entire stated purpose is to work even when automatic-trigger conditions aren't met — is the one built without that self-healing behavior.
</issue_description>

<issue_validation>
- **Checked:** PR-head `PostHogSDK.swift` — the `displaySurvey` nil guard, `installIntegrations()` (2738–2775), `installReplayIntegration()` (2779–2793), `startSessionRecording(resumeCurrent:)` (2387–2401), and `requiresSwizzling` across the three integrations.
- **Found:** all premises hold. `PostHogSurveyIntegration.requiresSwizzling` is hardcoded `true` (`PostHog/Surveys/PostHogSurveyIntegration.swift:16`); `installIntegrations()` does `if integration.requiresSwizzling, !config.enableSwizzling { continue }` (2749–2750), so with `enableSwizzling=false` the surveys integration is never installed → `surveysIntegration` stays nil → `displaySurvey` is a logged no-op. `startSessionRecording(resumeCurrent:)` self-heals via `if replayIntegration == nil { installReplayIntegration() }` (2397–2401), and `installReplayIntegration()` installs the integration directly, bypassing the swizzling gate — the reviewer's precedent parallel is exact. The manual survey path (`getSurveys` via remote config → `presentSurvey` → delegate `renderSurvey` over a dedicated `SurveysWindow`) does not use the swizzled `didLayoutViewToken`/`didBecomeActiveToken` auto-trigger hooks, so 'manual display doesn't need swizzling' is correct.
- **Found:** the suggestion partly overreaches — it proposes lazy-install even when surveys are *disabled* at config, which would wrongly override an explicit `config._surveys = false`. Only the swizzling case is the legitimate gap.
- **Impact:** real, accurately-diagnosed functional gap: an app with `enableSwizzling=false` (surveys enabled) cannot use the new on-demand API at all, contradicting the feature's stated purpose, and the repo already has a self-healing precedent for exactly this. Not noise — a substantive design/reliability observation a maintainer would want to decide on consciously.
- **Priority:** lowering should_fix → consider. Trigger is strictly the non-default `enableSwizzling=false` config; the failure degrades gracefully (logged no-op, no crash/data loss); and whether the swizzling gate should apply to the manual path is a defensible design-judgment call (swizzling-off ⇒ surveys-off, consistent with autocapture/screen-views). Worth recording, but below should-fix urgency.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Either give `PostHogSurveyIntegration` a `requiresSwizzling` value that reflects that only its automatic-trigger wiring needs swizzling (not the manual `displaySurvey` path), or mirror `installReplayIntegration()`'s pattern inside `PostHogSDK.displaySurvey` to lazily install the surveys integration on demand when it's missing, so the new API isn't silently unusable for any app that has disabled swizzling (or surveys) at config time.
</potential_solution>

Comment on lines +765 to +772
/// Atomically claims the active-survey slot for the given survey.
///
/// Returns `true` if this call won the slot (it was previously empty), or `false` if
/// another survey already holds it. Callers must only render when this returns `true`,
/// otherwise two concurrent flows could render a survey that diverges from the tracked
/// `activeSurvey`, which would permanently block all future surveys.
@discardableResult
private func setActiveSurvey(survey: PostHogSurvey, language: String? = nil, questionTranslations: [PostHogSurveyQuestionTranslation?]? = nil) -> Bool {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@discardableResult on setActiveSurvey contradicts its own doc comment's safety contract

consider best_practice

Why we think it's a valid issue
  • Checked: PR-head setActiveSurvey doc comment + annotation (765–772), every call site, the setShownSurvey TESTING helper (1131–1134), and the base (12140bb) version of setActiveSurvey.
  • Found: all premises accurate. Doc (765–770): 'Callers must only render when this returns true, otherwise … permanently block all future surveys'; line 771 is @discardableResult. The base version returned Void; both the Bool return and @discardableResult are new in this PR. Swift warns on an ignored non-Void result and @discardableResult is exactly what suppresses that warning, so the annotation does defeat the enforcement the doc describes.
  • Found: both live callers are correct — presentSurvey uses guard setActiveSurvey(...) else (348); the only ignoring caller is the setShownSurvey test helper (1133), safe because clearActiveSurvey() runs immediately before (1132) so the claim always succeeds. The annotation exists solely to keep that helper warning-free, as the reviewer surmised.
  • Impact: as actually reached today there is no defect (every real caller checks the result). But the PR introduced a genuine self-contradiction — doc declares the return mandatory-to-check while @discardableResult suppresses the warning that would enforce it — on the exact function underpinning the PR's TOCTOU fix, and the fix is a trivial, correct one-liner (_ = setActiveSurvey(...) at the one safe site). Legitimate low-cost safety/consistency tightening, not noise; its value is preventing a future regression rather than fixing a current bug.
  • Priority: lowering should_fix → consider. Zero current-code impact (both callers correct); the risk is a hypothetical future caller dropping the check. Given the severe failure mode (permanent survey lockout) and near-zero fix cost the warning is worth restoring, so keep it on record — but should_fix overstates a finding with no present defect.
Issue description

The new doc comment on setActiveSurvey explicitly states the return value is safety-critical: "Callers must only render when this returns true, otherwise two concurrent flows could render a survey that diverges from the tracked activeSurvey, which would permanently block all future surveys." Yet the function is annotated @discardableResult, which tells the compiler (and future callers) it is safe to ignore the return value with no warning. This directly undermines the documented contract: any future caller added to this file (or a refactor of an existing one) can silently drop the check the doc says is mandatory, with no compiler signal, reintroducing exactly the TOCTOU/active-survey-divergence bug this PR's fix is designed to prevent. The annotation appears to exist only so the pre-existing setShownSurvey(_:) test helper (which calls clearActiveSurvey() immediately before, making the result always true) keeps compiling without a new warning.

Suggested fix

Remove @discardableResult so the compiler enforces the documented contract, and make the one legitimately-safe ignore explicit at its call site instead, e.g. _ = setActiveSurvey(survey: survey) in setShownSurvey. This keeps the safety net intact for every future caller while still allowing the one known-safe exception without a warning.

Prompt to fix with AI (copy-paste)
## Context
@PostHog/Surveys/PostHogSurveyIntegration.swift#L765-772

<issue_description>
The new doc comment on `setActiveSurvey` explicitly states the return value is safety-critical: "Callers must only render when this returns `true`, otherwise two concurrent flows could render a survey that diverges from the tracked `activeSurvey`, which would permanently block all future surveys." Yet the function is annotated `@discardableResult`, which tells the compiler (and future callers) it is safe to ignore the return value with no warning. This directly undermines the documented contract: any future caller added to this file (or a refactor of an existing one) can silently drop the check the doc says is mandatory, with no compiler signal, reintroducing exactly the TOCTOU/active-survey-divergence bug this PR's fix is designed to prevent. The annotation appears to exist only so the pre-existing `setShownSurvey(_:)` test helper (which calls `clearActiveSurvey()` immediately before, making the result always `true`) keeps compiling without a new warning.
</issue_description>

<issue_validation>
- **Checked:** PR-head `setActiveSurvey` doc comment + annotation (765–772), every call site, the `setShownSurvey` TESTING helper (1131–1134), and the base (`12140bb`) version of `setActiveSurvey`.
- **Found:** all premises accurate. Doc (765–770): 'Callers must only render when this returns `true`, otherwise … permanently block all future surveys'; line 771 is `@discardableResult`. The base version returned `Void`; both the `Bool` return and `@discardableResult` are new in this PR. Swift warns on an ignored non-`Void` result and `@discardableResult` is exactly what suppresses that warning, so the annotation does defeat the enforcement the doc describes.
- **Found:** both live callers are correct — `presentSurvey` uses `guard setActiveSurvey(...) else` (348); the only ignoring caller is the `setShownSurvey` test helper (1133), safe because `clearActiveSurvey()` runs immediately before (1132) so the claim always succeeds. The annotation exists solely to keep that helper warning-free, as the reviewer surmised.
- **Impact:** as actually reached today there is no defect (every real caller checks the result). But the PR introduced a genuine self-contradiction — doc declares the return mandatory-to-check while `@discardableResult` suppresses the warning that would enforce it — on the exact function underpinning the PR's TOCTOU fix, and the fix is a trivial, correct one-liner (`_ = setActiveSurvey(...)` at the one safe site). Legitimate low-cost safety/consistency tightening, not noise; its value is preventing a future regression rather than fixing a current bug.
- **Priority:** lowering should_fix → consider. Zero current-code impact (both callers correct); the risk is a hypothetical future caller dropping the check. Given the severe failure mode (permanent survey lockout) and near-zero fix cost the warning is worth restoring, so keep it on record — but should_fix overstates a finding with no present defect.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Remove `@discardableResult` so the compiler enforces the documented contract, and make the one legitimately-safe ignore explicit at its call site instead, e.g. `_ = setActiveSurvey(survey: survey)` in `setShownSurvey`. This keeps the safety net intact for every future caller while still allowing the one known-safe exception without a warning.
</potential_solution>

Comment thread PostHog/PostHogSDK.swift
Comment on lines +2470 to +2493

/**
Displays the survey with the given ID on demand, regardless of its display conditions.

The survey must be running and present in the project's surveys. Display conditions
(targeting flags, event triggers, and the seen/wait-period checks) are bypassed, so this
also works for API-type surveys, which are never displayed automatically. If another
survey is already being displayed, the call is ignored.

This method will have no effect if PostHog is not enabled, or if surveys are disabled in the SDK configuration.

- Parameter surveyId: The ID of the survey to display
*/
@objc public func displaySurvey(_ surveyId: String) {
if !isEnabled() {
return
}

guard let surveysIntegration else {
return hedgeLog("Cannot display survey \(surveyId) - surveys integration is not installed.")
}

surveysIntegration.displaySurvey(surveyId: surveyId)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The new public PostHogSDK.displaySurvey(_:) entry point is never exercised by any test

consider testing

Why we think it's a valid issue
  • Checked: PR-head PostHogTests/PostHogSurveyDisplaySurveyTest.swift (all 5 @Test functions) and the public wrapper PostHogSDK.displaySurvey(_:) (2483–2492).
  • Found: claim is accurate. Each of the 5 tests calls integration.displaySurvey(surveyId:) on a manually-constructed PostHogSurveyIntegration (via getSurveyIntegration), and uses the postHog SUT only for getSut()/install/close()/reset(). None call postHog.displaySurvey(...), so the wrapper's three behaviors — isEnabled() no-op guard, surveysIntegration == nil no-op-with-log, and the forwarding — are uncovered.
  • Found (tempering): the wrapper is 3 lines of boilerplate identical in shape to the SDK's other public methods, and the substantive survey-display logic (ID lookup, isActive, condition-bypass for API/event surveys, unknown-ID no-op, atomic claim while another survey is active) is thoroughly tested at the integration level by these same 5 tests.
  • Impact: a genuine, verifiable coverage gap on the PR's headline public entry point — a future regression in the wrapper (missing forward, inverted isEnabled) would pass the entire suite. Accurate and specific, not noise, so it clears the keep bar. But the uncovered code is trivial standard boilerplate while the real logic is well-covered, so the marginal regression-protection value is modest.
  • Priority: lowering should_fix → consider. The gap is limited to a 3-line delegation wrapper of a well-established pattern with the meaningful behavior already tested one layer down; worth recording but below should-fix urgency.
Issue description

This PR's headline public contract is PostHogSDK.shared.displaySurvey("survey-id") (per the PR title and description), implemented here as a thin wrapper: it returns early if isEnabled() is false, returns early with a hedgeLog if surveysIntegration is nil, and otherwise forwards to surveysIntegration.displaySurvey(surveyId:). The only test file added by this PR, PostHogTests/PostHogSurveyDisplaySurveyTest.swift, never calls this method — every one of its 5 @Test functions calls integration.displaySurvey(surveyId:) directly on a manually-constructed PostHogSurveyIntegration instance, using postHog only for getSut()/close()/reset(). None of the three behaviors unique to this wrapper (the isEnabled() no-op guard, the surveysIntegration == nil no-op-with-log guard, and the delegation itself) are covered. A regression here — e.g. forgetting to forward the call, inverting the isEnabled() check, or a typo in the guard — would ship silently: every test in the suite would still pass because they all bypass this method entirely.

Suggested fix

Add at least one test that calls through the real public surface, e.g. postHog.displaySurvey("api-survey-id") instead of integration.displaySurvey(surveyId:), asserting the same integration.getActiveSurvey()?.id outcome. Additionally cover the isEnabled() == false case (call postHog.disable() or equivalent, then assert displaySurvey is a no-op) and, if feasible, the surveysIntegration == nil case (an SDK instance with surveys integration never installed) to lock down the guard behavior described in the doc comment ("This method will have no effect if PostHog is not enabled...").

Prompt to fix with AI (copy-paste)
## Context
@PostHog/PostHogSDK.swift#L2470-2493

<issue_description>
This PR's headline public contract is `PostHogSDK.shared.displaySurvey("survey-id")` (per the PR title and description), implemented here as a thin wrapper: it returns early if `isEnabled()` is false, returns early with a `hedgeLog` if `surveysIntegration` is nil, and otherwise forwards to `surveysIntegration.displaySurvey(surveyId:)`. The only test file added by this PR, `PostHogTests/PostHogSurveyDisplaySurveyTest.swift`, never calls this method — every one of its 5 `@Test` functions calls `integration.displaySurvey(surveyId:)` directly on a manually-constructed `PostHogSurveyIntegration` instance, using `postHog` only for `getSut()`/`close()`/`reset()`. None of the three behaviors unique to this wrapper (the `isEnabled()` no-op guard, the `surveysIntegration == nil` no-op-with-log guard, and the delegation itself) are covered. A regression here — e.g. forgetting to forward the call, inverting the `isEnabled()` check, or a typo in the guard — would ship silently: every test in the suite would still pass because they all bypass this method entirely.
</issue_description>

<issue_validation>
- **Checked:** PR-head `PostHogTests/PostHogSurveyDisplaySurveyTest.swift` (all 5 `@Test` functions) and the public wrapper `PostHogSDK.displaySurvey(_:)` (2483–2492).
- **Found:** claim is accurate. Each of the 5 tests calls `integration.displaySurvey(surveyId:)` on a manually-constructed `PostHogSurveyIntegration` (via `getSurveyIntegration`), and uses the `postHog` SUT only for `getSut()`/`install`/`close()`/`reset()`. None call `postHog.displaySurvey(...)`, so the wrapper's three behaviors — `isEnabled()` no-op guard, `surveysIntegration == nil` no-op-with-log, and the forwarding — are uncovered.
- **Found (tempering):** the wrapper is 3 lines of boilerplate identical in shape to the SDK's other public methods, and the substantive survey-display logic (ID lookup, `isActive`, condition-bypass for API/event surveys, unknown-ID no-op, atomic claim while another survey is active) is thoroughly tested at the integration level by these same 5 tests.
- **Impact:** a genuine, verifiable coverage gap on the PR's headline public entry point — a future regression in the wrapper (missing forward, inverted `isEnabled`) would pass the entire suite. Accurate and specific, not noise, so it clears the keep bar. But the uncovered code is trivial standard boilerplate while the real logic is well-covered, so the marginal regression-protection value is modest.
- **Priority:** lowering should_fix → consider. The gap is limited to a 3-line delegation wrapper of a well-established pattern with the meaningful behavior already tested one layer down; worth recording but below should-fix urgency.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Add at least one test that calls through the real public surface, e.g. `postHog.displaySurvey("api-survey-id")` instead of `integration.displaySurvey(surveyId:)`, asserting the same `integration.getActiveSurvey()?.id` outcome. Additionally cover the `isEnabled() == false` case (call `postHog.disable()` or equivalent, then assert `displaySurvey` is a no-op) and, if feasible, the `surveysIntegration == nil` case (an SDK instance with surveys integration never installed) to lock down the guard behavior described in the doc comment ("This method will have no effect if PostHog is not enabled...").
</potential_solution>

@marandaneto
marandaneto requested a review from a team August 7, 2026 06:27
@marandaneto
marandaneto marked this pull request as draft August 7, 2026 06:57
@marandaneto

Copy link
Copy Markdown
Member

moving to draft until comments/conflicts are resolved

@github-actions

Copy link
Copy Markdown
Contributor

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.

@github-actions github-actions Bot added the stale label Aug 14, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This PR was closed due to lack of activity. Feel free to reopen if it's still relevant.

@github-actions github-actions Bot closed this Aug 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support manual API displaySurvey

4 participants