Repository navigation
docs(surveys): specify the survey popup delay - #121
posthog[bot] wants to merge 3 commits into
Conversation
Adds a Survey popup delay requirement covering the window between deciding to show a survey and the survey reaching the screen: the delay source and its non-positive fallback, reserving the survey before the wait so repeat triggers do not restart it, and what happens to a pending survey dropped before it is shown. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 8b8b7451-6b5a-4d7e-a81a-a8ab45933aca
…eact-native claims react-native does not implement the delay yet (commented-out TODO in PostHogSurveyProvider.tsx); fix proposal/design accordingly and pass strict validation. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
marandaneto
left a comment
There was a problem hiding this comment.
Automated advisory code review.
|
|
||
| - **posthog-js** (browser) conforms: `handlePopoverSurvey` claims focus before arming the timer, | ||
| re-checks eligibility when the timer fires, and releases focus when the survey is dropped. | ||
| - **posthog-ios** conforms: `scheduleSurveyDisplay` sets `pendingSurvey` before the delay and |
There was a problem hiding this comment.
blocking: Qualify iOS conformance for a close before first display — The blanket iOS conformance claim misses a valid pre-show delegate-close path: the integration reserves activeSurvey before calling the delegate, but handleSurveyClosed marks that still-current presentation seen and emits survey dismissed even when its delegate never called onSurveyShown. That contradicts this PR’s new no-seen/no-dismissal requirement and its instruction that an unable-to-display host report closed; normal teardown avoids the problem only by invalidating active state first. Record this as an implementation gap or qualify the conformance statement rather than documenting the stronger callback contract as already satisfied. Reproduction: reproduced — swift -module-cache-path "$V/.swift-module-cache" "$V/ios-cancellation-probe.swift" executes the unchanged cited close handler for an unshown current presentation and exits 1 with seen=true and survey dismissed recorded.
There was a problem hiding this comment.
Fixed in d45e483: iOS now listed as a gap for close-before-shown.
| - **posthog-ios** conforms: `scheduleSurveyDisplay` sets `pendingSurvey` before the delay and | ||
| `dismissPendingSurveyIfNeeded` releases it through the closed handler without marking it seen. | ||
| - **posthog-android** conforms as of #849 (`pendingPresentation` reserved before `renderSurvey`). | ||
| - **posthog-flutter** conforms as of #627 for its own bottom sheet; it relies on the native |
There was a problem hiding this comment.
blocking: Do not certify Flutter’s cancelled-delay report-back path — Flutter #627 does not satisfy the newly required host report-back guarantee: its _cancelPopupDelay deliberately suppresses onClosed, and showSurvey returns when that cancelled wait resolves false. Relying on native reservation does not implement the missing callback; a cancelled bottom-sheet presentation therefore cannot be described as conforming to the requirement that every handed-off survey report shown or closed. Native reset/teardown may independently clear its reservation, so this is not a claim that ordinary reset necessarily deadlocks; qualify the documented conformance and track the cancellation gap instead. Reproduction: reproduced — DART_SUPPRESS_ANALYTICS=true dart "$V/flutter-cancellation-probe.dart" executes the unchanged cited asynchronous display/cancellation methods and exits 1 with onShownCalls=0; onClosedCalls=0.
There was a problem hiding this comment.
Fixed in d45e483: Flutter now listed as a gap for cancelled-delay report-back.
iOS marks a survey seen and emits survey dismissed when the delegate closes it before it was shown, and Flutter's cancelled delay reports neither shown nor closed. Qualify the conformance claims instead of certifying both SDKs. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Why
Every surveys-capable SDK honors
appearance.surveyPopupDelaySeconds, but thesurveysspec never mentions it. The spec's nearby rules ("allow only one active prompt", "a survey should be marked active before its shown/response/closed callbacks can mutate response state") are written for a survey that is already on screen, so the window between deciding to show a survey and the survey appearing was unspecified — and SDKs diverged there.What changed in the SDKs
pendingPresentation) beforerenderSurvey, matching iOS (scheduleSurveyDisplaysetspendingSurveybefore the delay) and the browser (_addSurveyToFocusbefore the timer inhandlePopoverSurvey). The delegate contract now requires reporting back, because a silently dropped survey would hold the reservation.What this PR does
Adds a Survey popup delay requirement to
openspec/specs/surveys/spec.md(proposed, applied, and archived on this branch per the repo workflow):survey shown/survey dismissed; the reservation is released when it is shown, dropped, or on reset.Uncertain / worth a look
surveyPopupDelaySecondsin its survey provider, but I did not verify whether it reserves the survey before the wait. The Impact section says so rather than claiming conformance.Created with PostHog Desktop
🤖 Generated with Claude Code