feat(surveys): add displaySurvey to show a survey on demand - #730
Conversation
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
|
Hi @ioannisj @turnipdabeets 👋 — tagging you both since you maintain the surveys code here. This adds the manual |
posthog-ios Compliance ReportDate: 2026-07-20 13:31:18 UTC
|
| 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 |
🦔 ReviewHog reviewed this pull requestFound 0 must fix, 1 should fix, 3 consider. Published 4 findings (view the review). |
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
There was a problem hiding this comment.
ReviewHog Report
Changes
Issues: 2 issues
Files (5)
.changeset/display-survey-manual-api.mdPostHog.xcodeproj/project.pbxprojPostHog/PostHogSDK.swiftPostHog/Surveys/PostHogSurveyIntegration.swiftPostHogTests/PostHogSurveyDisplaySurveyTest.swift
| 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) | ||
| } | ||
| } |
There was a problem hiding this comment.
displaySurvey doesn't verify the survey is running, contradicting its documented contract
Why we think it's a valid issue
- Checked:
getSurveys(PostHogSurveyIntegration.swift:166-189) and its terminaldecodeAndSetSurveys(231-252) — they call back with the raw decodedloadedSurveysand apply no active/running filter. Compared againstgetActiveMatchingSurveys(98-143) anddisplaySurvey(310-331). - Found:
getActiveMatchingSurveysapplies.filter(\.isActive)at line 110;displaySurveylooks the survey up withsurveys.first(where: { $0.id == surveyId })at line 325 and callspresentSurveydirectly at 329, never checkingisActive.isActiveisstartDate != nil && endDate == nil(987-989). The public doc fordisplaySurveystates "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 setsend_datewithout archiving, so stopped surveys stay in the cached payload.isSurveyRunning(hasstart_date, noend_date) is the web SDK's first eligibility gate, a baseline check distinct from the display conditions (targeting_flag, event triggers, seen/wait) thatdisplaySurveyintentionally bypasses.isActivehere 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 collectssurvey shown/sentresponses, 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>
| 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) | ||
| } | ||
| } |
There was a problem hiding this comment.
TOCTOU race between the active-survey check and claim lets a rendered survey diverge from tracked activeSurvey, permanently blocking all future surveys
Why we think it's a valid issue
- Checked:
setActiveSurvey(751-762),presentSurvey(334-356),canShowNextSurvey(970-972), the threehandle*rejection guards (482/509/576),clearActiveSurveycallers (grep: only 595 in production + a TESTING helper at 1111), the default delegaterenderSurvey(PostHogSurveysDefaultDelegate.swift:23-50), and the trigger/threading model. - Found:
setActiveSurveywrites onlyif activeSurvey == nil(753) under the lock, butpresentSurveycalls it at 337 and then unconditionally callsrenderSurveyat 344 — the return of the claim is never inspected. The pre-checkcanShowNextSurvey()in bothdisplaySurvey(318) andshowNextSurvey(293) is a separate lock acquisition, with an asyncgetSurveyshop before the claim, so it is a genuine check-then-act across two locks. - Found: the divergence is not self-healing:
handleSurveyClosedonly reachesclearActiveSurvey()(595) whensurvey.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 whileactiveSurveyholds the winner, whose close event never arrives →activeSurveynever cleared →canShowNextSurvey()returns false for the rest of the process (in-memory state, no timeout,cleanupSurveysdoesn't reset it). - Impact: Confirmed reachable and severe: a background-thread
@objc displaySurveycall (the project mandates any-thread public APIs) overlapping a main-thread automatic trigger, or a startup double-load where twogetSurveyscache-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>
|
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 |
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
turnipdabeets
left a comment
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
ReviewHog Report
Changes
Issues: 4 issues
Files (5)
.changeset/display-survey-manual-api.mdPostHog.xcodeproj/project.pbxprojPostHog/PostHogSDK.swiftPostHog/Surveys/PostHogSurveyIntegration.swiftPostHogTests/PostHogSurveyDisplaySurveyTest.swift
| 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) | ||
| } | ||
| } |
There was a problem hiding this comment.
displaySurvey bypasses the popover-only rendering gate (canRenderSurvey), letting widget/unknown survey types reach a popover-only delegate
Why we think it's a valid issue
- Checked: PR-head
displaySurvey(surveyId:)(310–338),showNextSurvey(285–301),canRenderSurvey(280–282),getSurveys(166–188), thePostHogSurveyTypeenum (PostHog/Models/Surveys/PostHogSurveyEnums.swift:22–26), and the default delegatePostHogSurveysDefaultDelegate.renderSurvey(PostHog/Surveys/PostHogSurveysDefaultDelegate.swift:23–48). - Found: all premises hold.
showNextSurveygates viaactiveSurveys.first(where: self.canRenderSurvey)(297);canRenderSurveyissurvey.type == .popover(comment: 'only render popover surveys for now', 280–282).displaySurvey(surveyId:)resolves by ID and applies onlycanShowNextSurvey()+guard survey.isActive, then callspresentSurvey(survey)with no type check.getSurveysreturns the full decoded list unfiltered, so.widget/.unknownsurveys are reachable. The default delegaterenderSurveyhas no type branching — it unconditionally schedules a full-screen popover (scheduleSurveyDisplay, line 48) for anyPostHogDisplaySurvey. - Found: the pre-existing
canRenderSurveygate cannot be reused verbatim because enabling.apidisplay is the feature's whole point (.apiwould fail== .popover); a correct fix must widen the allow-list to.popover/.apiand reject.widget/.unknown, as the reviewer acknowledges. - Impact: real, directly PR-introduced.
presentSurveybecame 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/.unknownsurvey passed todisplaySurveyrenders as a full-screen takeover popover instead of being rejected — concrete trigger, concrete consequence — with a forward-compat angle (.unknowntypes 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
displaySurveycall 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>
|
|
||
| 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) | ||
| } |
There was a problem hiding this comment.
PostHogSDK.displaySurvey has no fallback when the surveys integration isn't installed, unlike the established lazy-install precedent for on-demand replay APIs
Why we think it's a valid issue
- Checked: PR-head
PostHogSDK.swift— thedisplaySurveynil guard,installIntegrations()(2738–2775),installReplayIntegration()(2779–2793),startSessionRecording(resumeCurrent:)(2387–2401), andrequiresSwizzlingacross the three integrations. - Found: all premises hold.
PostHogSurveyIntegration.requiresSwizzlingis hardcodedtrue(PostHog/Surveys/PostHogSurveyIntegration.swift:16);installIntegrations()doesif integration.requiresSwizzling, !config.enableSwizzling { continue }(2749–2750), so withenableSwizzling=falsethe surveys integration is never installed →surveysIntegrationstays nil →displaySurveyis a logged no-op.startSessionRecording(resumeCurrent:)self-heals viaif replayIntegration == nil { installReplayIntegration() }(2397–2401), andinstallReplayIntegration()installs the integration directly, bypassing the swizzling gate — the reviewer's precedent parallel is exact. The manual survey path (getSurveysvia remote config →presentSurvey→ delegaterenderSurveyover a dedicatedSurveysWindow) does not use the swizzleddidLayoutViewToken/didBecomeActiveTokenauto-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=falseconfig; 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>
| /// 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 { |
There was a problem hiding this comment.
@discardableResult on setActiveSurvey contradicts its own doc comment's safety contract
Why we think it's a valid issue
- Checked: PR-head
setActiveSurveydoc comment + annotation (765–772), every call site, thesetShownSurveyTESTING helper (1131–1134), and the base (12140bb) version ofsetActiveSurvey. - 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 returnedVoid; both theBoolreturn and@discardableResultare new in this PR. Swift warns on an ignored non-Voidresult and@discardableResultis exactly what suppresses that warning, so the annotation does defeat the enforcement the doc describes. - Found: both live callers are correct —
presentSurveyusesguard setActiveSurvey(...) else(348); the only ignoring caller is thesetShownSurveytest helper (1133), safe becauseclearActiveSurvey()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
@discardableResultsuppresses 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>
|
|
||
| /** | ||
| 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) | ||
| } |
There was a problem hiding this comment.
The new public PostHogSDK.displaySurvey(_:) entry point is never exercised by any test
Why we think it's a valid issue
- Checked: PR-head
PostHogTests/PostHogSurveyDisplaySurveyTest.swift(all 5@Testfunctions) and the public wrapperPostHogSDK.displaySurvey(_:)(2483–2492). - Found: claim is accurate. Each of the 5 tests calls
integration.displaySurvey(surveyId:)on a manually-constructedPostHogSurveyIntegration(viagetSurveyIntegration), and uses thepostHogSUT only forgetSut()/install/close()/reset(). None callpostHog.displaySurvey(...), so the wrapper's three behaviors —isEnabled()no-op guard,surveysIntegration == nilno-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>
|
moving to draft until comments/conflicts are resolved |
|
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 |
|
This PR was closed due to lack of activity. Feel free to reopen if it's still relevant. |
💡 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:survey shown/sent/dismissedevents), so all downstream behavior is unchanged.showNextSurvey()intentionally never auto-displays.showNextSurvey()was refactored to share the newpresentSurvey(_:)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
PostHogSurveyDisplaySurveyTestcovering: 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
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 iOS API is the prerequisite. Design choice: reuse the internal display path via a sharedpresentSurvey(_:)helper rather than a parallel code path, and keep v1 to a singlesurveyIdparameter (noignoreConditions/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