feat: support survey-interaction segment filters (ENG-1275) - #52
Merged
Conversation
Ports the client half of the web SDK change for interaction-based segment
filters ("have seen X", "have completed X", ...). Membership for those filters
is computed server-side and can flip the moment a contact interacts with a
survey, so the SDK now refetches user state instead of waiting for it to expire.
- Decode the new per-survey `interactionRefresh` gate from the workspace-state
payload. The decoder is deliberately tolerant: a partial object reads missing
flags as false rather than failing the whole payload, which would leave the
user with no surveys at all.
- Add the missing `onFinished` bridge event. The surveys library has always
exposed the callback, but it was never passed in, so "have completed X" had no
client-side trigger. Because we pass `getSetIsResponseSendingFinished`, this
fires only after the finished response is accepted by the backend.
- Refresh user state after a display, response or finish, gated twice: no-op for
anonymous users, and no-op unless the server flagged that survey and event.
Routed through the UpdateQueue so a display -> response -> finish burst is
debounced into one request.
- Give UpdateQueue an in-flight join. APIClient does not serialise requests, so
two concurrent POST /user calls could race and the later response would
overwrite segments, displays and responses wholesale.
Also fixes a pre-existing bug this feature depends on: the user-state sync timer
never fired. `startSyncTimer` ran inside `syncUser`'s completion, which APIClient
delivers on URLSession's background delegate queue, and `Timer.scheduledTimer`
installs on `RunLoop.current` — a pooled thread with no run loop. The timer was
created and silently never ran, so user state was only ever refreshed by the
lazy check inside `setup()`. It is now built unscheduled and added to the main
run loop, mirroring what UpdateQueue already does for its debounce timer. The
interval is clamped so a device clock running ahead of the server cannot cause a
tight re-sync loop, and the fire block re-checks the user id so a logged-out or
switched user is not re-synced.
WalkthroughAdded interaction-based survey refresh configuration and event models. JavaScript survey callbacks now report completion, display, and response events to the native bridge. The bridge deduplicates events per WebView presentation and routes matching interactions to 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/FormbricksSDK/WebView/FormbricksViewModel.swift (1)
58-81: 📐 Maintainability & Code Quality | 🔵 TrivialRemove the completion refresh event from the JS bridge contract.
onFinishedandisResponseSendingFinishedare implementation details, not a public SDK API. If an older self-hosted bundle lacks this prop, this code silently disables the on-finish refresh path. Use the supported completion-flow mechanism, such as configured post-survey actions/webhooks, or document and pin the exact surveys bundle version that exposes it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/FormbricksSDK/WebView/FormbricksViewModel.swift` around lines 58 - 81, Remove the onFinished callback and getSetIsResponseSendingFinished/setResponseFinished wiring from the JavaScript bridge and surveyProps construction. Do not expose isResponseSendingFinished through this SDK contract; use the supported post-survey action or webhook completion mechanism instead, or pin the surveys bundle to a version that explicitly provides it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/FormbricksSDK/Manager/UserManager.swift`:
- Around line 149-154: Re-arm the sync cycle in the `.failure` branch of
`syncUser(withId:)` after releasing the queue lock and logging the error. Add a
private `scheduleSyncRetry(for:)` helper that invalidates the stale timer,
verifies the user ID still matches, and schedules a bounded one-shot retry on
the main run loop; optionally apply increasing backoff for consecutive failures.
- Around line 197-243: Confine all syncTimer reads, writes, invalidation, and
scheduling in startSyncTimer and stopSyncTimer to the main thread by moving the
complete bookkeeping into onMain closures. Ensure startSyncTimer validates
expiresAt/userId and creates or replaces the timer inside that closure, while
stopSyncTimer clears and invalidates the property there; preserve the existing
main-thread synchronous behavior and allow background callers to dispatch
asynchronously.
In `@Sources/FormbricksSDK/Networking/Queue/UpdateQueue.swift`:
- Around line 76-96: Defer refresh requests received during an in-flight sync
instead of dropping them: add pending-refresh state managed by
requestUserStateRefresh and replay it from syncDidFinish(), including after
successful sync completion. Clear this state in reset() and cleanup() to prevent
replaying a prior user’s nudge, while preserving the existing userId handling
and debounce flow.
In `@Tests/FormbricksSDKTests/SurveyInteractionRefreshTests.swift`:
- Around line 212-247: Replace the fixed async delays in
testMatchingFlagTriggersExactlyOneSync and
testInteractionBurstCoalescesIntoOneSync with callback-driven expectations. Add
an onPostUser callback to CountingMockService, invoke it when the mock records a
post, and fulfill each test’s expectation from that callback before asserting
the call count. Leave bounded waits unchanged for tests that verify no request
occurs.
---
Outside diff comments:
In `@Sources/FormbricksSDK/WebView/FormbricksViewModel.swift`:
- Around line 58-81: Remove the onFinished callback and
getSetIsResponseSendingFinished/setResponseFinished wiring from the JavaScript
bridge and surveyProps construction. Do not expose isResponseSendingFinished
through this SDK contract; use the supported post-survey action or webhook
completion mechanism instead, or pin the surveys bundle to a version that
explicitly provides it.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b9ef08f2-e415-4864-9fae-26e65ecaf13b
📒 Files selected for processing (12)
Sources/FormbricksSDK/Config.swiftSources/FormbricksSDK/Manager/SurveyManager.swiftSources/FormbricksSDK/Manager/UserManager.swiftSources/FormbricksSDK/Model/Javascript/EventType.swiftSources/FormbricksSDK/Model/Workspace/Survey.swiftSources/FormbricksSDK/Model/Workspace/Surveys/InteractionRefresh.swiftSources/FormbricksSDK/Networking/Queue/UpdateQueue.swiftSources/FormbricksSDK/WebView/FormbricksViewModel.swiftSources/FormbricksSDK/WebView/SurveyWebView.swiftTests/FormbricksSDKTests/FormbricksSDKTests.swiftTests/FormbricksSDKTests/Mock/Environment.jsonTests/FormbricksSDKTests/SurveyInteractionRefreshTests.swift
- Confine every `syncTimer` read and write to the main thread. `startSyncTimer()` runs on URLSession's delegate queue while `stopSyncTimer()` can run on main, so the two writes raced on an unprotected `Timer?`. A lost `nil` write stranded a live timer that nothing could cancel afterwards. All bookkeeping now happens inside `onMain`, via a shared `scheduleSync(after:for:)`. - Re-arm the timer when a sync fails. The timer that fired is spent and `startSyncTimer()` is otherwise only reached from a successful sync, so one transient network error ended the refresh cycle for the rest of the process. This was masked before, when the timer never fired at all. The retry backs off by `Config.User.retryAfterFailureInMinutes` rather than the minimum sync interval, so a sustained outage doesn't become a fixed-rate request stream. - Defer and replay a refresh that arrives mid-sync instead of dropping it. The in-flight request was built before that interaction, so its response cannot reflect it — dropping the nudge left segments stale until the next trigger. Only one deferred refresh is kept, so many interactions behind a slow sync still cost a single follow-up. `syncDidFinish()` now drains it, and is called on the success path too. - Drive the positive-count tests off the mock instead of fixed sleeps, so a loaded CI machine can't make them flake, and use `assertForOverFulfill` for the "exactly one request" half. 92 tests, 0 failures. Both new behaviours are mutation-checked: dropping the failure re-arm fails `testFailedSyncReArmsTheTimer`, and dropping the deferred nudge fails `testRefreshDuringAnInFlightSyncIsDeferredThenReplayed`.
|
Dhruwang
approved these changes
Aug 5, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
The web app now supports survey-interaction segment filters — targeting contacts by whether they have seen / have not seen / have started responding to / have completed / have not completed a survey within a time window (formbricks#8588).
Membership for those filters is computed server-side, and it can flip the moment a contact interacts with a survey. The web SDK reacts by refetching user state right away. iOS had no equivalent, so it kept using the segment list it received at app launch — meaning a rule like "completed survey A → show survey B" would not fire in the same session.
This ports the client half of that change.
What changed
The gate —
interactionRefreshThe client API now attaches a per-survey object saying whether interacting with that survey can change any live survey's membership:
It is absent for workspaces that don't use interaction targeting, and present-but-all-false for surveys no interaction filter references — both are handled.
InteractionRefresh's decoder is deliberately tolerant: a partial object reads missing flags asfalseinstead of throwing. A strict model would turn one malformed object into akeyNotFoundon the whole workspace-state decode, and the failure mode there is a silent total blackout —filteredSurveysstays empty and the user sees no surveys at all.The missing completion signal —
onFinishedonFinishedhas always been a prop of the surveys library, but iOS never passed it in, sohaveCompleted/haveNotCompletedhad no client-side trigger at all. Added as a newEventTypecase, a JS shim, and a native handler.Because iOS passes
getSetIsResponseSendingFinished,isResponseSendingFinishedstartsfalse, so on app surveysonFinishedgenuinely means the finished response was accepted by the backend — not merely "the UI finished". No change to the surveys bundle is needed.The refresh
Gated twice, because a
/usersync is not cheap:Routed through
UpdateQueuerather than callingsyncUserdirectly, so a display → response → finish burst debounces into a single request. A per-showing guard means a repeated event can't cost a second request.UpdateQueuein-flight joinAPIClientdoes not serialise requests, so two concurrentPOST /usercalls could race and whichever response landed last would overwritesegments/displays/responseswholesale. A refresh nudge now joins an in-flight sync instead of starting a second one.Bug fix: the user-state sync timer never fired
Worth reading separately — it's pre-existing, and this feature depends on it.
startSyncTimer()usedTimer.scheduledTimer, which installs onRunLoop.current. It is called from insidesyncUser's completion, whichAPIClientdelivers onURLSession's background delegate queue — a pooled thread with no run loop. The timer was created, retained, and silently never fired.Net effect: user state (segments, displays, responses) was frozen for the whole app session. The only working refresh was the lazy check inside
Formbricks.setup(), which runs once per process. There is no foreground observer either.The same file's sibling timer already hops to main (
UpdateQueue.startDebounceTimer), so this was an oversight rather than a design choice.Now:
RunLoop.mainin.commonmode, so it isn't postponed while the user is scrollingexpiresAtwould land in the device's past)Verification
89 tests, 0 failures— 65 existing plus 24 new.The timer fix has a real regression test: it drives a sync whose completion arrives on a background thread, mirroring
APIClient, and asserts a second sync happens. RevertingstartSyncTimertoTimer.scheduledTimermakes it fail withExceeded timeout of 5 seconds.The end-to-end wiring is covered by driving the real
JsMessageHandlerwith a realWKScriptMessage(bodyis overridable, so no production seam was needed). Confirmed with mutation testing — each of these makes a test go red:.onFinishedcase →breakfirst(where:)→firstin the survey lookupNotes for reviewers
interactionRefresh— identical decoded object graph every time. Swift's synthesisedCodableignores unknown keys and nothing here overrides that. Old apps are unaffected.Config.User.minimumSyncIntervalInSecondsis avarpurely so tests can shorten it. The SDK never writes to it. Happy to swap it for a different injection point if you'd rather.onResponseCreatedfires optimistically from the surveys library, before the response-create POST completes, andsyncUserreplacesresponses/displayswholesale. So an interaction-driven sync can drop a just-made local append, which could let adisplayMultiplesurvey re-display later in the session. The web SDK has the same characteristic, so this matches it rather than diverging. The real fix is upstream — movingonResponseCreatedontoResponseQueue's confirmed hook — which fixes both platforms at once.