Skip to content

feat(surveys): support survey popup delay - #520

Merged
ioannisj merged 8 commits into
mainfrom
03-10-feat_surveys_support_survey_popup_delay
Apr 17, 2026
Merged

ioannisj merged 8 commits into
mainfrom
03-10-feat_surveys_support_survey_popup_delay

Conversation

@adboio

@adboio adboio commented Mar 10, 2026 •

Copy link
Copy Markdown
Contributor

💡 Motivation and Context

ios does not support survey popup delay

closes #499

💚 How did you test it?

unit test / manual testing

📝 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
  • Added the release label to the PR

@adboio adboio mentioned this pull request Mar 10, 2026
6 tasks done

adboio commented Mar 10, 2026 •

Copy link
Copy Markdown
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@adboio adboio added the release label Mar 10, 2026 — with Graphite App
@adboio
adboio requested a review from a team March 10, 2026 23:52
@github-actions

github-actions Bot commented Mar 11, 2026 •

Copy link
Copy Markdown
Contributor

posthog-ios Compliance Report

Date: 2026-04-15 14:33:20 UTC
Duration: 2993ms

⚠️ Some Tests Failed

0/1 tests passed, 1 failed


Feature_Flags Tests

⚠️ 0/1 tests passed, 1 failed

View Details
Test Status Duration
Request Payload.Request With Person Properties Device Id ❌ 396ms

Failures

request_payload.request_with_person_properties_device_id

404, message='Not Found', url='http://host.docker.internal:8080/get_feature_flag'

@adboio
adboio force-pushed the 03-10-feat_surveys_support_survey_popup_delay branch 2 times, most recently from 8caa460 to eaaf956 Compare March 11, 2026 18:21
@adboio
adboio force-pushed the 03-10-feat_surveys_support_event_property_filters branch from a334421 to 81c5433 Compare March 11, 2026 18:21
@adboio
adboio changed the base branch from 03-10-feat_surveys_support_event_property_filters to graphite-base/520 March 11, 2026 19:32
@adboio
adboio force-pushed the graphite-base/520 branch from 81c5433 to a9366ad Compare March 11, 2026 19:32
@adboio
adboio force-pushed the 03-10-feat_surveys_support_survey_popup_delay branch from eaaf956 to b7c6843 Compare March 11, 2026 19:32
@graphite-app
graphite-app Bot changed the base branch from graphite-base/520 to main March 11, 2026 19:32
@adboio
adboio force-pushed the 03-10-feat_surveys_support_survey_popup_delay branch from b7c6843 to 268e6e5 Compare March 11, 2026 19:32
@adboio
adboio force-pushed the 03-10-feat_surveys_support_survey_popup_delay branch from 268e6e5 to 9e7acb3 Compare March 12, 2026 17:27
@marandaneto

Copy link
Copy Markdown
Member

@adboio @ioannisj what is missing here? no reviews and still draft

@marandaneto marandaneto reopened this Mar 24, 2026
@ioannisj
ioannisj marked this pull request as ready for review April 7, 2026 17:06
Comment thread PostHogTests/PostHogSurveysTest.swift Outdated
]

let sorted = surveys.sorted {
($0.appearance?.surveyPopupDelaySeconds ?? 0) < ($1.appearance?.surveyPopupDelaySeconds ?? 0)

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.

nit: I think this could be flaky cause .sorted is not guaranteed to be stable for equal keys I think. Maybe add tie-breaker as well or remove the scenario where there are equal keys (no-delay and delayed-0)

Also, this just duplicates production code, so if we change sorting at some point, this will still pass. I think we just need to test what getActiveMatchingSurveys returns instead

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.

On first point above, actually it seems that it's guaranteed, so never mind

https://developer.apple.com/documentation/swift/array/sorted()

The sorting algorithm is guaranteed to be stable. A stable sort preserves the relative order of elements that compare as equal.

Comment on lines +296 to +297
let delay = survey.appearance?.surveyPopupDelaySeconds ?? 0
let deadline: DispatchTime = delay > 0 ? .now() + delay : .now()

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.

I think this may be a bit unecessary and just adds noise. We could just do this?

Suggested change
let delay = survey.appearance?.surveyPopupDelaySeconds ?? 0
let deadline: DispatchTime = delay > 0 ? .now() + delay : .now()
let delay = max(survey.appearance?.surveyPopupDelaySeconds ?? 0, 0)
let deadline: DispatchTime = .now() + delay

Comment on lines +288 to +290
let sortedSurveys = activeSurveys.sorted {
($0.appearance?.surveyPopupDelaySeconds ?? 0) < ($1.appearance?.surveyPopupDelaySeconds ?? 0)
}

@ioannisj ioannisj Apr 7, 2026 •

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.

Does web sdk behave similar to this? This feels like the wrong approach to me cause it changes the whole survey selection behavior, right? Now surveys with a smaller delay are given display priority, whereas before, selection was simply by first(where:) to a list returned by the server/config

I feel that the delay should actually be handled in the UI layer, in PostHogSurveysDefaultDelegate where the delegate will delay the presentation of a survey (and handle the dismissal scenario before delay like we do in L302) after the survey has been selected by getActiveMatchingSurveys()

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.

Yes, looks like js SDK is performing this delay logic in display layer in handlePopoverSurvey

@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 Apr 15, 2026
@ioannisj
ioannisj requested review from a team and ioannisj April 15, 2026 13:33
@ioannisj
ioannisj removed their request for review April 15, 2026 13:33
@ioannisj

ioannisj commented Apr 15, 2026 •

Copy link
Copy Markdown
Contributor

@PostHog/team-client-libraries pushed some changes if you can please review?

@ioannisj ioannisj removed the stale label Apr 15, 2026
@ioannisj
ioannisj merged commit feeaeac into main Apr 17, 2026
32 checks passed
@ioannisj
ioannisj deleted the 03-10-feat_surveys_support_survey_popup_delay branch April 17, 2026 07:39

This branch was previously deployed

1 inactive deployment
Release — a1cfd0a9 Deployed Apr 17, 2026 by ioannisj via Bump version and commit to main #209
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.

Implement survey popup delay (surveyPopupDelaySeconds)

3 participants