From 40b4fb0324078e1ef71dcb9ba7ae72513aa2ba5c Mon Sep 17 00:00:00 2001 From: pandeymangg Date: Wed, 5 Aug 2026 18:06:13 +0530 Subject: [PATCH 1/2] feat: support survey-interaction segment filters (ENG-1275) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- Sources/FormbricksSDK/Config.swift | 10 + .../FormbricksSDK/Manager/SurveyManager.swift | 9 + .../FormbricksSDK/Manager/UserManager.swift | 80 ++- .../Model/Javascript/EventType.swift | 1 + .../Model/Workspace/Survey.swift | 3 + .../Surveys/InteractionRefresh.swift | 55 ++ .../Networking/Queue/UpdateQueue.swift | 48 +- .../WebView/FormbricksViewModel.swift | 8 + .../FormbricksSDK/WebView/SurveyWebView.swift | 26 +- .../FormbricksSDKTests.swift | 3 +- .../FormbricksSDKTests/Mock/Environment.json | 5 + .../SurveyInteractionRefreshTests.swift | 501 ++++++++++++++++++ 12 files changed, 738 insertions(+), 11 deletions(-) create mode 100644 Sources/FormbricksSDK/Model/Workspace/Surveys/InteractionRefresh.swift create mode 100644 Tests/FormbricksSDKTests/SurveyInteractionRefreshTests.swift diff --git a/Sources/FormbricksSDK/Config.swift b/Sources/FormbricksSDK/Config.swift index affe2509..d9c0d5b1 100644 --- a/Sources/FormbricksSDK/Config.swift +++ b/Sources/FormbricksSDK/Config.swift @@ -1,6 +1,16 @@ +import Foundation + struct Config { struct Environment { /// On error, the environment will be refreshed after this amount of time (in minutes) static let refreshStateOnErrorTimeoutInMinutes = 10 } + + struct User { + /// Floor for the gap between two user-state syncs. Guards against a device clock + /// running ahead of the server, where every `expiresAt` the server returns is already + /// in the device's past and an unclamped timer would sync in a tight loop. + /// A `var` only so tests can shorten it; the SDK never writes to it. + static var minimumSyncIntervalInSeconds: TimeInterval = 60 + } } diff --git a/Sources/FormbricksSDK/Manager/SurveyManager.swift b/Sources/FormbricksSDK/Manager/SurveyManager.swift index face2c12..e0965f4c 100644 --- a/Sources/FormbricksSDK/Manager/SurveyManager.swift +++ b/Sources/FormbricksSDK/Manager/SurveyManager.swift @@ -182,6 +182,15 @@ extension SurveyManager { func onNewDisplay(surveyId: String) { userManager.onDisplay(surveyId: surveyId) } + + /// Forwards an in-survey interaction so the user manager can pull fresh `segments` when + /// the server flagged this survey/source pair as able to change segment membership. + func onSurveyInteraction(surveyId: String, source: InteractionSource) { + guard let survey = workspaceResponse?.data.data.surveys?.first(where: { $0.id == surveyId }) else { + return + } + userManager.refreshSegmentsAfterInteraction(survey: survey, source: source) + } } // MARK: - Present and dismiss survey window - diff --git a/Sources/FormbricksSDK/Manager/UserManager.swift b/Sources/FormbricksSDK/Manager/UserManager.swift index 5904cd55..4c02b5e8 100644 --- a/Sources/FormbricksSDK/Manager/UserManager.swift +++ b/Sources/FormbricksSDK/Manager/UserManager.swift @@ -69,6 +69,28 @@ final class UserManager: UserManagerSyncable { responses = newResponses surveyManager?.filterSurveys() } + + /// Pulls fresh server-computed `segments` after an interaction that can flip segment + /// membership, instead of waiting for the state to expire. + /// + /// A `surveyInteraction` segment filter ("have seen X", "have completed X", ...) can change + /// who a contact is the moment they interact with a survey. The local bookkeeping in + /// `onDisplay` / `onResponse` keeps display caps and recontact days correct on device, but + /// segment membership is only ever computed by the server, so it has to be refetched. + /// + /// The refresh is deliberately gated twice, because a `/user` sync is not cheap: + /// - no-op for anonymous users, who never receive segments in the first place; + /// - no-op unless the server set the bit for this survey and this event. + /// + /// It is routed through the `UpdateQueue` rather than calling `syncUser` directly, so a + /// display -> response -> finish burst is debounced into a single request. + func refreshSegmentsAfterInteraction(survey: Survey, source: InteractionSource) { + guard let userId = userId else { return } + guard survey.interactionRefresh?.shouldRefresh(on: source) == true else { return } + + Formbricks.logger?.debug("Refreshing segments after \(source.rawValue) on survey \(survey.id)") + updateQueue?.requestUserStateRefresh(userId: userId) + } /// Syncs the user state with the server if the user id is set and the expiration date has passed. func syncUserStateIfNeeded() { @@ -80,6 +102,11 @@ final class UserManager: UserManagerSyncable { backingSegments = nil backingDisplays = nil backingResponses = nil + + // The state is still valid, but nothing has been scheduled to refresh it when it + // does expire — `startSyncTimer()` is otherwise only reached from a successful sync, + // so a launch that finds a warm cache would never refresh segments again. + startSyncTimer() return } @@ -120,6 +147,9 @@ final class UserManager: UserManagerSyncable { self?.surveyManager?.filterSurveys() self?.startSyncTimer() case .failure(let error): + // Release the in-flight lock so a later refresh nudge isn't swallowed. + // `reset()` already does this on the success path. + self?.updateQueue?.syncDidFinish() Formbricks.logger?.error(error) } } @@ -145,10 +175,9 @@ final class UserManager: UserManagerSyncable { backingExpiresAt = nil Formbricks.language = "default" - syncTimer?.invalidate() - syncTimer = nil + stopSyncTimer() updateQueue?.cleanup() - + // Re-filter surveys for logged out user surveyManager?.filterSurveys() } @@ -165,14 +194,53 @@ final class UserManager: UserManagerSyncable { // MARK: - Timer - private extension UserManager { + /// Schedules the next user-state sync for when the cached state expires. + /// + /// This runs inside `syncUser`'s completion, which `APIClient` delivers on URLSession's + /// background delegate queue — a pooled thread with no run loop. `Timer.scheduledTimer` + /// installs on `RunLoop.current`, so scheduling it there produced a timer that could never + /// fire, and the user state was in practice only ever refreshed by the lazy check inside + /// `Formbricks.setup()`. Build the timer unscheduled and add it to the main run loop + /// instead, the same way `UpdateQueue` hops to main for its debounce timer. func startSyncTimer() { - guard let expiresAt = expiresAt, let id = userId else { return } syncTimer?.invalidate() - syncTimer = Timer.scheduledTimer(withTimeInterval: expiresAt.timeIntervalSinceNow, repeats: false) { [weak self] _ in - self?.syncUser(withId: id) + syncTimer = nil + + guard let expiresAt = expiresAt, let id = userId else { return } + + // A device clock running ahead of the server makes every `expiresAt` we receive + // already in the past, which would otherwise sync in a tight loop. + let interval = max(expiresAt.timeIntervalSinceNow, Config.User.minimumSyncIntervalInSeconds) + + let timer = Timer(timeInterval: interval, repeats: false) { [weak self] _ in + // The user may have been logged out or swapped while this was pending. + guard let self = self, self.userId == id else { return } + self.syncUser(withId: id) } + syncTimer = timer + + // `.common` so an expiry that lands mid-scroll isn't postponed until the gesture ends. + onMain { RunLoop.main.add(timer, forMode: .common) } } + /// Cancels a pending user-state sync. Safe to call from any thread. + func stopSyncTimer() { + let timer = syncTimer + syncTimer = nil + guard let timer = timer else { return } + onMain { timer.invalidate() } + } + + /// Runs `work` on the main thread, immediately if we are already there. `Timer` and + /// `RunLoop` are bound to the thread that scheduled them, so all timer bookkeeping has to + /// funnel through the main run loop. + func onMain(_ work: @escaping () -> Void) { + if Thread.isMainThread { + work() + } else { + DispatchQueue.main.async(execute: work) + } + } } // MARK: - Getters - diff --git a/Sources/FormbricksSDK/Model/Javascript/EventType.swift b/Sources/FormbricksSDK/Model/Javascript/EventType.swift index 0bbb580f..ed494120 100644 --- a/Sources/FormbricksSDK/Model/Javascript/EventType.swift +++ b/Sources/FormbricksSDK/Model/Javascript/EventType.swift @@ -2,6 +2,7 @@ enum EventType: String, Codable { case onClose = "onClose" case onDisplayCreated = "onDisplayCreated" case onResponseCreated = "onResponseCreated" + case onFinished = "onFinished" case onOpenExternalURL = "onOpenExternalURL" case onSurveyLibraryLoadError = "onSurveyLibraryLoadError" } diff --git a/Sources/FormbricksSDK/Model/Workspace/Survey.swift b/Sources/FormbricksSDK/Model/Workspace/Survey.swift index 10f83739..9892cc9d 100644 --- a/Sources/FormbricksSDK/Model/Workspace/Survey.swift +++ b/Sources/FormbricksSDK/Model/Workspace/Survey.swift @@ -65,4 +65,7 @@ struct Survey: Codable { let styling: Styling? let languages: [SurveyLanguage]? let projectOverwrites: ProjectOverwrites? + /// Whether interacting with this survey can change some live survey's segment + /// membership. Absent unless the workspace uses survey-interaction targeting. + let interactionRefresh: InteractionRefresh? } diff --git a/Sources/FormbricksSDK/Model/Workspace/Surveys/InteractionRefresh.swift b/Sources/FormbricksSDK/Model/Workspace/Surveys/InteractionRefresh.swift new file mode 100644 index 00000000..d339d84a --- /dev/null +++ b/Sources/FormbricksSDK/Model/Workspace/Surveys/InteractionRefresh.swift @@ -0,0 +1,55 @@ +import Foundation + +/// The three survey-lifecycle moments that can flip interaction-based segment membership. +/// Raw values match the source names used by the JS SDK so the gate below can be keyed +/// off the same vocabulary on both platforms. +enum InteractionSource: String { + case onDisplay + case onResponse + case onFinished +} + +/// Per-survey gate for the post-interaction segment refresh. +/// +/// Each flag says whether interacting with *this* survey via that event can change some +/// live survey's segment membership — e.g. a survey referenced only by a "have seen" +/// filter refreshes on display but not on response or finish, and a survey no interaction +/// filter references never refreshes at all. +/// +/// The client API attaches this only for workspaces that use survey-interaction targeting, +/// so it is absent for everyone else, and present-but-all-false for surveys in such a +/// workspace that no interaction filter points at. +struct InteractionRefresh: Codable, Equatable { + let onDisplay: Bool + let onResponse: Bool + let onFinished: Bool + + private enum CodingKeys: String, CodingKey { + case onDisplay, onResponse, onFinished + } + + init(onDisplay: Bool = false, onResponse: Bool = false, onFinished: Bool = false) { + self.onDisplay = onDisplay + self.onResponse = onResponse + self.onFinished = onFinished + } + + /// Missing sub-keys decode as `false`, mirroring the tolerant `Segment` decoder. A strict + /// model would turn a partial object into a `keyNotFound` on the whole workspace-state + /// decode, which blanks out every survey — so never require these keys. + init(from decoder: Decoder) throws { + let container = try decoder.container(keyedBy: CodingKeys.self) + onDisplay = try container.decodeIfPresent(Bool.self, forKey: .onDisplay) ?? false + onResponse = try container.decodeIfPresent(Bool.self, forKey: .onResponse) ?? false + onFinished = try container.decodeIfPresent(Bool.self, forKey: .onFinished) ?? false + } + + /// Whether an interaction of this kind should trigger a user-state refresh. + func shouldRefresh(on source: InteractionSource) -> Bool { + switch source { + case .onDisplay: return onDisplay + case .onResponse: return onResponse + case .onFinished: return onFinished + } + } +} diff --git a/Sources/FormbricksSDK/Networking/Queue/UpdateQueue.swift b/Sources/FormbricksSDK/Networking/Queue/UpdateQueue.swift index f7b10e86..97c6ae99 100644 --- a/Sources/FormbricksSDK/Networking/Queue/UpdateQueue.swift +++ b/Sources/FormbricksSDK/Networking/Queue/UpdateQueue.swift @@ -15,7 +15,12 @@ final class UpdateQueue { private var attributes: [String : AttributeValue]? private var language: String? private var timer: Timer? - + /// True while a commit-triggered sync is airborne. A repeat nudge joins that request + /// instead of starting a second one: `APIClient` does not serialise requests, so two + /// concurrent `POST /user` calls would race and whichever response landed last would + /// overwrite `segments` / `displays` / `responses` wholesale. + private var isSyncInFlight = false + private weak var userManager: UserManagerSyncable? init(userManager: UserManagerSyncable) { @@ -68,11 +73,34 @@ final class UpdateQueue { } } + /// Asks for the user state to be re-read from the server. Carries no new data — it exists + /// so an interaction that can change segment membership doesn't have to wait for the + /// state to expire. Dropped while a sync is already in flight, because that sync's + /// response already brings fresh segments. + func requestUserStateRefresh(userId: String) { + syncQueue.sync { + guard !isSyncInFlight else { + Formbricks.logger?.debug("UpdateQueue - refresh skipped, a sync is already in flight") + return + } + self.userId = userId + startDebounceTimer() + } + } + + /// Called by the user manager once a sync finishes, so the next nudge can start a request. + func syncDidFinish() { + syncQueue.sync { + isSyncInFlight = false + } + } + func reset() { syncQueue.sync { userId = nil attributes = nil language = nil + isSyncInFlight = false } } @@ -103,16 +131,29 @@ private extension UpdateQueue { syncQueue.sync { effectiveUserId = self.userId ?? Formbricks.userManager?.userId effectiveAttributes = self.attributes + // Only mark a sync in flight when one is actually about to be sent. The guard + // below decides that, so mirror its condition here — otherwise an anonymous + // commit would leave the flag stuck and swallow every later refresh nudge. + if effectiveUserId != nil { + isSyncInFlight = true + } } - + guard let userId = effectiveUserId else { let error = FormbricksSDKError(type: .userIdIsNotSetYet) Formbricks.logger?.error(error.message) return } + // Nothing will call `syncDidFinish()` if there is no user manager left to run the + // request, so clear the flag here rather than leaving it stuck. + guard let userManager = userManager else { + syncQueue.sync { isSyncInFlight = false } + return + } + Formbricks.logger?.debug("UpdateQueue - commit() called on UpdateQueue with \(userId) and \(effectiveAttributes ?? [:])") - userManager?.syncUser(withId: userId, attributes: effectiveAttributes) + userManager.syncUser(withId: userId, attributes: effectiveAttributes) } } @@ -125,6 +166,7 @@ extension UpdateQueue { userId = nil attributes = nil language = nil + isSyncInFlight = false } } } diff --git a/Sources/FormbricksSDK/WebView/FormbricksViewModel.swift b/Sources/FormbricksSDK/WebView/FormbricksViewModel.swift index 3728a2b0..7c867285 100644 --- a/Sources/FormbricksSDK/WebView/FormbricksViewModel.swift +++ b/Sources/FormbricksSDK/WebView/FormbricksViewModel.swift @@ -55,6 +55,13 @@ private extension FormbricksViewModel { window.webkit.messageHandlers.jsMessage.postMessage(JSON.stringify({ event: "onResponseCreated" })); }; + // Fires once the finished response has been accepted by the backend — the + // surveys library gates this on `isResponseSendingFinished`, and we supply + // `getSetIsResponseSendingFinished` below, so it starts out false. + function onFinished() { + window.webkit.messageHandlers.jsMessage.postMessage(JSON.stringify({ event: "onFinished" })); + }; + function onOpenExternalURL(url) { window.webkit.messageHandlers.jsMessage.postMessage(JSON.stringify({ event: "onOpenExternalURL", onOpenExternalURLParams: { url: url } })); }; @@ -71,6 +78,7 @@ private extension FormbricksViewModel { getSetIsResponseSendingFinished, onDisplayCreated, onResponseCreated, + onFinished, onClose, onOpenExternalURL, }; diff --git a/Sources/FormbricksSDK/WebView/SurveyWebView.swift b/Sources/FormbricksSDK/WebView/SurveyWebView.swift index 730f9f34..7892686e 100644 --- a/Sources/FormbricksSDK/WebView/SurveyWebView.swift +++ b/Sources/FormbricksSDK/WebView/SurveyWebView.swift @@ -130,6 +130,13 @@ final class JsMessageHandler: NSObject, WKScriptMessageHandler { let surveyId: String + /// Interaction sources already refreshed during this presentation. One handler is created + /// per WebView, so this is scoped to a single survey showing. The surveys library guards + /// `onResponseCreated` itself, but `onFinished` is not guarded there, and a self-hosted + /// server may serve an older bundle — so gate the refresh on our side too. Only the + /// refresh is gated; the existing displays/responses bookkeeping keeps its behaviour. + private var refreshedSources: Set = [] + init(surveyId: String) { self.surveyId = surveyId } @@ -170,11 +177,19 @@ final class JsMessageHandler: NSObject, WKScriptMessageHandler { /// Happens when the user submits an answer. case .onResponseCreated: Formbricks.surveyManager?.postResponse(surveyId: surveyId) + refreshSegmentsOnce(for: .onResponse) /// Happens when a survey is shown. case .onDisplayCreated: Formbricks.surveyManager?.onNewDisplay(surveyId: surveyId) - + refreshSegmentsOnce(for: .onDisplay) + + /// Happens when the survey is completed and the finished response has been + /// accepted by the backend. Only used to refresh interaction-based segments — + /// the survey window is still closed by `onClose`. + case .onFinished: + refreshSegmentsOnce(for: .onFinished) + /// Happens when the user closes the survey view with the close button. case .onClose: Formbricks.surveyManager?.dismissSurveyWebView() @@ -196,6 +211,15 @@ final class JsMessageHandler: NSObject, WKScriptMessageHandler { Formbricks.logger?.error("\(error.message): \(message.body)") } } + + /// Forwards an interaction to the survey manager at most once per source per showing. + /// `WKScriptMessageHandler` callbacks arrive on the main thread, so the unsynchronised + /// set is safe here. + private func refreshSegmentsOnce(for source: InteractionSource) { + guard !refreshedSources.contains(source) else { return } + refreshedSources.insert(source) + Formbricks.surveyManager?.onSurveyInteraction(surveyId: surveyId, source: source) + } } // MARK: - Handle Javascript console.log - diff --git a/Tests/FormbricksSDKTests/FormbricksSDKTests.swift b/Tests/FormbricksSDKTests/FormbricksSDKTests.swift index 96a2116c..14edbb86 100644 --- a/Tests/FormbricksSDKTests/FormbricksSDKTests.swift +++ b/Tests/FormbricksSDKTests/FormbricksSDKTests.swift @@ -258,7 +258,8 @@ final class FormbricksSDKTests: XCTestCase { SurveyLanguage(enabled: true, isDefault: false, language: LanguageDetail(id: "2", code: "de", alias: "german", projectId: "p1")), SurveyLanguage(enabled: false, isDefault: false, language: LanguageDetail(id: "3", code: "fr", alias: nil, projectId: "p1")) ], - projectOverwrites: nil + projectOverwrites: nil, + interactionRefresh: nil ) // No language provided XCTAssertEqual(manager.getLanguageCode(survey: survey, language: nil), "default") diff --git a/Tests/FormbricksSDKTests/Mock/Environment.json b/Tests/FormbricksSDKTests/Mock/Environment.json index fa8ec1dc..805d4d03 100644 --- a/Tests/FormbricksSDKTests/Mock/Environment.json +++ b/Tests/FormbricksSDKTests/Mock/Environment.json @@ -372,6 +372,11 @@ }, "showResponseCount": false, "timeToFinish": false + }, + "interactionRefresh": { + "onDisplay": true, + "onResponse": true, + "onFinished": true } } ] diff --git a/Tests/FormbricksSDKTests/SurveyInteractionRefreshTests.swift b/Tests/FormbricksSDKTests/SurveyInteractionRefreshTests.swift new file mode 100644 index 00000000..60ee8412 --- /dev/null +++ b/Tests/FormbricksSDKTests/SurveyInteractionRefreshTests.swift @@ -0,0 +1,501 @@ +import XCTest +import WebKit +@testable import FormbricksSDK + +/// `WKScriptMessage` cannot be constructed with a payload, but `body` is overridable, so the +/// real `JsMessageHandler` can be driven end to end without a live WebView. +private final class FakeScriptMessage: WKScriptMessage { + private let payload: Any + + init(payload: Any) { + self.payload = payload + super.init() + } + + override var body: Any { payload } +} + +/// Counts `postUser` calls so the interaction gate can be asserted end to end. +private final class CountingMockService: MockFormbricksService { + var postUserCallCount = 0 + + override func postUser(id: String, attributes: [String: AttributeValue]?, completion: @escaping (ResultType) -> Void) { + postUserCallCount += 1 + super.postUser(id: id, attributes: attributes, completion: completion) + } +} + +final class SurveyInteractionRefreshTests: XCTestCase { + + private let userDefaultsKeys = [ + "userIdKey", "contactIdKey", "segmentsKey", + "displaysKey", "responsesKey", "lastDisplayedAtKey", "expiresAtKey" + ] + + override func setUp() { + super.setUp() + Formbricks.cleanup() + clearUserState() + } + + override func tearDown() { + Formbricks.cleanup() + clearUserState() + super.tearDown() + } + + private func clearUserState() { + userDefaultsKeys.forEach { UserDefaults.standard.removeObject(forKey: $0) } + // The cached workspace blob outlives `Formbricks.cleanup()` on purpose (real apps rely + // on it across launches), and its fixture `expiresAt` is years out — so without this a + // blob written by an earlier run is reused and never refetched. + UserDefaults.standard.removeObject(forKey: SurveyManager.workspaceResponseObjectKey) + UserDefaults.standard.removeObject(forKey: SurveyManager.legacyEnvironmentResponseObjectKey) + } + + private func decodeSurvey(_ json: String) throws -> Survey { + try JSONDecoder().decode(Survey.self, from: Data(json.utf8)) + } + + private func survey(id: String = "survey-a", refresh: InteractionRefresh?) -> Survey { + Survey( + id: id, + triggers: nil, + recontactDays: nil, + displayLimit: nil, + delay: nil, + displayPercentage: nil, + displayOption: .respondMultiple, + segment: nil, + styling: nil, + languages: nil, + projectOverwrites: nil, + interactionRefresh: refresh + ) + } + + // MARK: - Decoding + + /// Workspaces without interaction targeting get no `interactionRefresh` at all. + func testSurveyDecodesWithoutInteractionRefresh() throws { + let decoded = try decodeSurvey(#"{"id":"survey-a"}"#) + XCTAssertNil(decoded.interactionRefresh) + } + + func testSurveyDecodesFullInteractionRefresh() throws { + let decoded = try decodeSurvey(#""" + {"id":"survey-a","interactionRefresh":{"onDisplay":true,"onResponse":false,"onFinished":true}} + """#) + XCTAssertEqual(decoded.interactionRefresh, InteractionRefresh(onDisplay: true, onResponse: false, onFinished: true)) + } + + /// A partial object must not fail the decode — otherwise one malformed survey blanks out + /// the whole workspace payload and the user sees no surveys at all. + func testPartialInteractionRefreshDefaultsMissingKeysToFalse() throws { + let decoded = try decodeSurvey(#"{"id":"survey-a","interactionRefresh":{"onDisplay":true}}"#) + XCTAssertEqual(decoded.interactionRefresh, InteractionRefresh(onDisplay: true)) + } + + func testUnknownKeyInsideInteractionRefreshIsIgnored() throws { + let decoded = try decodeSurvey(#""" + {"id":"survey-a","interactionRefresh":{"onDisplay":true,"onSomethingNew":true}} + """#) + XCTAssertEqual(decoded.interactionRefresh?.onDisplay, true) + } + + /// The server attaches an all-false object to every survey in an interaction-targeting + /// workspace, so this is a real payload and must be distinguishable from absent. + func testAllFalseInteractionRefreshIsPresentButNeverRefreshes() throws { + let decoded = try decodeSurvey(#""" + {"id":"survey-a","interactionRefresh":{"onDisplay":false,"onResponse":false,"onFinished":false}} + """#) + XCTAssertNotNil(decoded.interactionRefresh) + for source in [InteractionSource.onDisplay, .onResponse, .onFinished] { + XCTAssertFalse(decoded.interactionRefresh?.shouldRefresh(on: source) ?? true) + } + } + + /// The cached workspace blob is re-encoded from the typed model, so the field has to + /// survive a round trip or it is silently lost until the cache expires. + func testInteractionRefreshSurvivesEncodeDecodeRoundTrip() throws { + let original = survey(refresh: InteractionRefresh(onDisplay: true, onResponse: true, onFinished: true)) + let data = try JSONEncoder().encode(original) + let decoded = try JSONDecoder().decode(Survey.self, from: data) + XCTAssertEqual(decoded.interactionRefresh, original.interactionRefresh) + } + + func testShouldRefreshMapsEachSourceToItsOwnFlag() { + let refresh = InteractionRefresh(onDisplay: true, onResponse: false, onFinished: true) + XCTAssertTrue(refresh.shouldRefresh(on: .onDisplay)) + XCTAssertFalse(refresh.shouldRefresh(on: .onResponse)) + XCTAssertTrue(refresh.shouldRefresh(on: .onFinished)) + } + + // MARK: - JS bridge + + func testEventTypeDecodesOnFinished() throws { + let data = Data(#"{"event":"onFinished"}"#.utf8) + let message = try JSONDecoder().decode(JsMessageData.self, from: data) + XCTAssertEqual(message.event, .onFinished) + } + + /// The event vocabulary stays closed: an unrecognised event must still fail to decode so + /// it is logged rather than silently mapped onto a known case. + func testUnknownEventStillFailsToDecode() { + let data = Data(#"{"event":"onSomethingElse"}"#.utf8) + XCTAssertNil(try? JSONDecoder().decode(JsMessageData.self, from: data)) + } + + func testHtmlTemplatePassesOnFinishedToRenderSurvey() throws { + Formbricks.setup(with: FormbricksConfig.Builder(appUrl: "https://example.com", workspaceId: "workspaceId") + .service(MockFormbricksService()) + .build()) + + guard let url = Bundle.module.url(forResource: "Environment", withExtension: "json"), + let data = try? Data(contentsOf: url) else { + return XCTFail("Missing Environment.json fixture") + } + let workspaceResponse = try JSONDecoder.iso8601Full.decode(GetWorkspaceRequest.Response.self, from: data) + guard let surveyId = workspaceResponse.data.data.surveys?.first?.id else { + return XCTFail("Fixture has no surveys") + } + + let html = FormbricksViewModel(workspaceResponse: workspaceResponse, surveyId: surveyId).htmlString + XCTAssertEqual(html?.contains(#"event: "onFinished""#), true) + XCTAssertEqual(html?.contains("function onFinished()"), true) + // Must be listed in the props object, not merely defined. + XCTAssertEqual(html?.contains("onFinished,"), true) + } + + // MARK: - The gate + + func testAnonymousUserNeverRefreshes() { + let service = CountingMockService() + let userManager = UserManager(service: service) + + userManager.refreshSegmentsAfterInteraction( + survey: survey(refresh: InteractionRefresh(onDisplay: true)), + source: .onDisplay + ) + + let exp = expectation(description: "no sync for anonymous user") + DispatchQueue.main.asyncAfter(deadline: .now() + 0.6) { + XCTAssertEqual(service.postUserCallCount, 0) + exp.fulfill() + } + wait(for: [exp], timeout: 2.0) + } + + func testGateBlocksWhenFieldAbsentOrFlagFalse() { + let service = CountingMockService() + let userManager = UserManager(service: service) + UserDefaults.standard.set("user-1", forKey: "userIdKey") + + // Absent — workspace does not use interaction targeting. + userManager.refreshSegmentsAfterInteraction(survey: survey(refresh: nil), source: .onDisplay) + // Present but all false — no interaction filter references this survey. + userManager.refreshSegmentsAfterInteraction(survey: survey(refresh: InteractionRefresh()), source: .onDisplay) + // Present, but a different source than the one that fired. + userManager.refreshSegmentsAfterInteraction( + survey: survey(refresh: InteractionRefresh(onDisplay: true)), + source: .onResponse + ) + + let exp = expectation(description: "gate blocks all three") + DispatchQueue.main.asyncAfter(deadline: .now() + 0.6) { + XCTAssertEqual(service.postUserCallCount, 0) + exp.fulfill() + } + wait(for: [exp], timeout: 2.0) + } + + func testMatchingFlagTriggersExactlyOneSync() { + let service = CountingMockService() + let userManager = UserManager(service: service) + UserDefaults.standard.set("user-1", forKey: "userIdKey") + + userManager.refreshSegmentsAfterInteraction( + survey: survey(refresh: InteractionRefresh(onDisplay: true)), + source: .onDisplay + ) + + let exp = expectation(description: "one sync") + DispatchQueue.main.asyncAfter(deadline: .now() + 0.6) { + XCTAssertEqual(service.postUserCallCount, 1) + exp.fulfill() + } + wait(for: [exp], timeout: 2.0) + } + + /// A display -> response -> finish burst must cost one request, not three. + func testInteractionBurstCoalescesIntoOneSync() { + let service = CountingMockService() + let userManager = UserManager(service: service) + UserDefaults.standard.set("user-1", forKey: "userIdKey") + + let allOn = survey(refresh: InteractionRefresh(onDisplay: true, onResponse: true, onFinished: true)) + userManager.refreshSegmentsAfterInteraction(survey: allOn, source: .onDisplay) + userManager.refreshSegmentsAfterInteraction(survey: allOn, source: .onResponse) + userManager.refreshSegmentsAfterInteraction(survey: allOn, source: .onFinished) + + let exp = expectation(description: "burst coalesces") + DispatchQueue.main.asyncAfter(deadline: .now() + 0.8) { + XCTAssertEqual(service.postUserCallCount, 1) + exp.fulfill() + } + wait(for: [exp], timeout: 2.0) + } + + // MARK: - UpdateQueue in-flight join + + func testRefreshIsDroppedWhileSyncIsInFlightAndResumesAfter() { + let mockUserManager = MockUserManager() + let queue = UpdateQueue(userManager: mockUserManager) + defer { queue.cleanup() } + + queue.requestUserStateRefresh(userId: "user-1") + + let firstCommit = expectation(description: "first commit") + DispatchQueue.main.asyncAfter(deadline: .now() + 0.6) { + // MockUserManager never reports completion, so the queue is still "in flight". + XCTAssertEqual(mockUserManager.syncCallCount, 1) + + queue.requestUserStateRefresh(userId: "user-1") + DispatchQueue.main.asyncAfter(deadline: .now() + 0.6) { + XCTAssertEqual(mockUserManager.syncCallCount, 1, "Nudge during an in-flight sync must be dropped") + + queue.syncDidFinish() + queue.requestUserStateRefresh(userId: "user-1") + DispatchQueue.main.asyncAfter(deadline: .now() + 0.6) { + XCTAssertEqual(mockUserManager.syncCallCount, 2, "Nudge after completion must sync again") + firstCommit.fulfill() + } + } + } + wait(for: [firstCommit], timeout: 5.0) + } + + /// A commit with no user id must not leave the in-flight flag stuck, or every later + /// refresh nudge would be swallowed for the lifetime of the queue. + func testAnonymousCommitDoesNotWedgeTheQueue() { + let mockUserManager = MockUserManager() + let queue = UpdateQueue(userManager: mockUserManager) + defer { queue.cleanup() } + + // No user id anywhere: commit bails out early. + queue.set(attributes: ["foo": "bar"]) + + let exp = expectation(description: "queue still usable") + DispatchQueue.main.asyncAfter(deadline: .now() + 0.6) { + XCTAssertEqual(mockUserManager.syncCallCount, 0) + queue.requestUserStateRefresh(userId: "user-1") + DispatchQueue.main.asyncAfter(deadline: .now() + 0.6) { + XCTAssertEqual(mockUserManager.syncCallCount, 1) + exp.fulfill() + } + } + wait(for: [exp], timeout: 3.0) + } + + // MARK: - End-to-end wiring: JS event -> SurveyManager -> UserManager -> queue + + /// Drives the real `JsMessageHandler` with a real `WKScriptMessage`, so the whole delivery + /// path is covered — not just the pieces. Without this, the `.onFinished` switch case can be + /// replaced with `break` and every other test still passes. + private func setUpSdkWithFixture(_ service: FormbricksServiceProtocol, userId: String? = "user-1") -> String { + if let userId = userId { + UserDefaults.standard.set(userId, forKey: "userIdKey") + } + Formbricks.setup(with: FormbricksConfig.Builder(appUrl: "https://example.com", workspaceId: "workspaceId") + .service(service) + .build()) + return "cm6ovw6j7000gsf0kduf4oo4i" // the fixture survey, which carries interactionRefresh + } + + private func send(_ event: String, to handler: JsMessageHandler) { + handler.userContentController( + WKUserContentController(), + didReceive: FakeScriptMessage(payload: #"{"event":"\#(event)"}"#) + ) + } + + func testOnFinishedEventReachesTheUserStateRefresh() { + let service = CountingMockService() + let surveyId = setUpSdkWithFixture(service) + let baseline = service.postUserCallCount + + send("onFinished", to: JsMessageHandler(surveyId: surveyId)) + + let exp = expectation(description: "onFinished triggers a refresh") + DispatchQueue.main.asyncAfter(deadline: .now() + 0.8) { + XCTAssertEqual(service.postUserCallCount - baseline, 1) + exp.fulfill() + } + wait(for: [exp], timeout: 3.0) + } + + func testDisplayEventReachesTheUserStateRefresh() { + let service = CountingMockService() + let surveyId = setUpSdkWithFixture(service) + let baseline = service.postUserCallCount + + send("onDisplayCreated", to: JsMessageHandler(surveyId: surveyId)) + + let exp = expectation(description: "onDisplayCreated triggers a refresh") + DispatchQueue.main.asyncAfter(deadline: .now() + 0.8) { + XCTAssertEqual(service.postUserCallCount - baseline, 1) + exp.fulfill() + } + wait(for: [exp], timeout: 3.0) + } + + /// The one-shot guard: the surveys library does not guard `onFinished`, and a self-hosted + /// server may serve an older bundle, so a repeated event must not cost a second request + /// once the first sync has already completed. + func testRepeatedOnFinishedRefreshesOnlyOnceForTheSameShowing() { + let service = CountingMockService() + let surveyId = setUpSdkWithFixture(service) + let baseline = service.postUserCallCount + let handler = JsMessageHandler(surveyId: surveyId) + + send("onFinished", to: handler) + + let exp = expectation(description: "second onFinished is ignored") + // Wait past the debounce and the sync so the in-flight lock is released; only the + // handler's own guard can suppress the second event by then. + DispatchQueue.main.asyncAfter(deadline: .now() + 0.9) { + self.send("onFinished", to: handler) + DispatchQueue.main.asyncAfter(deadline: .now() + 0.9) { + XCTAssertEqual(service.postUserCallCount - baseline, 1) + exp.fulfill() + } + } + wait(for: [exp], timeout: 5.0) + } + + /// A fresh handler means a fresh showing, so it is allowed to refresh again. + func testNewShowingCanRefreshAgain() { + let service = CountingMockService() + let surveyId = setUpSdkWithFixture(service) + let baseline = service.postUserCallCount + + send("onFinished", to: JsMessageHandler(surveyId: surveyId)) + + let exp = expectation(description: "second showing refreshes again") + DispatchQueue.main.asyncAfter(deadline: .now() + 0.9) { + self.send("onFinished", to: JsMessageHandler(surveyId: surveyId)) + DispatchQueue.main.asyncAfter(deadline: .now() + 0.9) { + XCTAssertEqual(service.postUserCallCount - baseline, 2) + exp.fulfill() + } + } + wait(for: [exp], timeout: 5.0) + } + + /// The survey id must actually be matched. If the lookup degraded to "just take the first + /// survey", an unknown id would wrongly consult another survey's flags. + func testUnknownSurveyIdDoesNotRefresh() { + let service = CountingMockService() + _ = setUpSdkWithFixture(service) + let baseline = service.postUserCallCount + + Formbricks.surveyManager?.onSurveyInteraction(surveyId: "no-such-survey", source: .onFinished) + + let exp = expectation(description: "unknown survey id is a no-op") + DispatchQueue.main.asyncAfter(deadline: .now() + 0.8) { + XCTAssertEqual(service.postUserCallCount - baseline, 0) + exp.fulfill() + } + wait(for: [exp], timeout: 3.0) + } + + /// Proves the fixture — i.e. a real server-shaped payload — decodes the gate correctly. + func testFixtureSurveyCarriesInteractionRefresh() { + let service = CountingMockService() + let surveyId = setUpSdkWithFixture(service) + + let survey = Formbricks.surveyManager?.workspaceResponse?.data.data.surveys? + .first(where: { $0.id == surveyId }) + XCTAssertEqual(survey?.interactionRefresh?.onDisplay, true) + XCTAssertEqual(survey?.interactionRefresh?.onResponse, true) + XCTAssertEqual(survey?.interactionRefresh?.onFinished, true) + } + + // MARK: - Sync timer + + /// The bug this covers: `startSyncTimer` ran inside `syncUser`'s completion, which + /// `APIClient` delivers on URLSession's background delegate queue. `Timer.scheduledTimer` + /// installs on `RunLoop.current`, and that pooled thread has no run loop, so the timer was + /// created and never fired. This mock reproduces the same threading, so the second + /// `postUser` only happens if the timer is genuinely live. + func testSyncTimerFiresWhenScheduledFromABackgroundCompletion() { + let originalFloor = Config.User.minimumSyncIntervalInSeconds + Config.User.minimumSyncIntervalInSeconds = 0.2 + defer { Config.User.minimumSyncIntervalInSeconds = originalFloor } + + let service = BackgroundCompletionMockService() + service.expiresIn = 0.3 + let userManager = UserManager(service: service) + + let exp = expectation(description: "timer fires and re-syncs") + service.onPostUser = { count in + if count == 2 { exp.fulfill() } + } + + userManager.syncUser(withId: "user-1") + + wait(for: [exp], timeout: 5.0) + XCTAssertGreaterThanOrEqual(service.postUserCallCount, 2) + } + + /// A device clock ahead of the server makes every `expiresAt` land in the past. Without + /// the floor the timer would fire immediately, re-sync, and loop. + func testSkewedClockDoesNotCauseASyncLoop() { + let originalFloor = Config.User.minimumSyncIntervalInSeconds + Config.User.minimumSyncIntervalInSeconds = 10 + defer { Config.User.minimumSyncIntervalInSeconds = originalFloor } + + let service = BackgroundCompletionMockService() + service.expiresIn = -3600 // server expiry already an hour in the device's past + let userManager = UserManager(service: service) + + userManager.syncUser(withId: "user-1") + + let exp = expectation(description: "no loop") + DispatchQueue.main.asyncAfter(deadline: .now() + 1.0) { + XCTAssertEqual(service.postUserCallCount, 1, "Clamp should hold the next sync off") + XCTAssertEqual(userManager.syncTimer?.isValid, true) + exp.fulfill() + } + wait(for: [exp], timeout: 3.0) + } +} + +/// Delivers completions on a background queue, the way `APIClient` does via URLSession's +/// delegate queue, and returns a user state whose expiry is controllable. Used to prove the +/// sync timer is not scheduled onto a dead run loop. +private final class BackgroundCompletionMockService: MockFormbricksService { + var postUserCallCount = 0 + /// Seconds from now for the returned `expiresAt`. Negative simulates clock skew. + var expiresIn: TimeInterval = 0.3 + var onPostUser: ((Int) -> Void)? + + override func postUser(id: String, attributes: [String: AttributeValue]?, completion: @escaping (ResultType) -> Void) { + postUserCallCount += 1 + onPostUser?(postUserCallCount) + + let expiresAt = DateFormatter.isoFormatter.string(from: Date().addingTimeInterval(expiresIn)) + let json = """ + {"data":{"state":{"data":{"contactId":"contact-1","displays":[],"lastDisplayAt":null,"responses":[],"segments":["segment-1"],"userId":"\(id)"},"expiresAt":"\(expiresAt)"}}} + """ + + DispatchQueue.global().async { + do { + let response = try JSONDecoder.iso8601Full.decode(PostUserRequest.Response.self, from: Data(json.utf8)) + completion(.success(response)) + } catch { + completion(.failure(error)) + } + } + } +} From 7acc9c740ad2bca7b3ef6c733e77c436b8593a0c Mon Sep 17 00:00:00 2001 From: pandeymangg Date: Wed, 5 Aug 2026 18:25:58 +0530 Subject: [PATCH 2/2] =?UTF-8?q?fix:=20address=20review=20=E2=80=94=20timer?= =?UTF-8?q?=20races,=20lost=20refreshes,=20flaky=20test=20waits?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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`. --- Sources/FormbricksSDK/Config.swift | 5 + .../FormbricksSDK/Manager/UserManager.swift | 67 ++++++++--- .../Networking/Queue/UpdateQueue.swift | 29 ++++- .../SurveyInteractionRefreshTests.swift | 109 ++++++++++++++---- 4 files changed, 167 insertions(+), 43 deletions(-) diff --git a/Sources/FormbricksSDK/Config.swift b/Sources/FormbricksSDK/Config.swift index d9c0d5b1..8b299bfc 100644 --- a/Sources/FormbricksSDK/Config.swift +++ b/Sources/FormbricksSDK/Config.swift @@ -12,5 +12,10 @@ struct Config { /// in the device's past and an unclamped timer would sync in a tight loop. /// A `var` only so tests can shorten it; the SDK never writes to it. static var minimumSyncIntervalInSeconds: TimeInterval = 60 + + /// How long to wait before retrying a user-state sync that failed. Deliberately much + /// longer than the minimum interval so a sustained outage doesn't turn into a + /// fixed-rate request stream. Mirrors `Environment.refreshStateOnErrorTimeoutInMinutes`. + static var retryAfterFailureInMinutes = 10 } } diff --git a/Sources/FormbricksSDK/Manager/UserManager.swift b/Sources/FormbricksSDK/Manager/UserManager.swift index 4c02b5e8..9f8e880a 100644 --- a/Sources/FormbricksSDK/Manager/UserManager.swift +++ b/Sources/FormbricksSDK/Manager/UserManager.swift @@ -144,13 +144,20 @@ final class UserManager: UserManagerSyncable { } self?.updateQueue?.reset() + // `reset()` clears the in-flight lock, but only this drains a refresh that + // arrived while the request was out — that interaction happened after this + // response was computed, so it still needs its own sync. + self?.updateQueue?.syncDidFinish() self?.surveyManager?.filterSurveys() self?.startSyncTimer() case .failure(let error): - // Release the in-flight lock so a later refresh nudge isn't swallowed. - // `reset()` already does this on the success path. + // Release the in-flight lock so a later refresh nudge isn't swallowed, and + // replay one that arrived mid-sync. `reset()` clears the lock on the success + // path, but only this call drains a queued refresh. self?.updateQueue?.syncDidFinish() Formbricks.logger?.error(error) + // Re-arm, otherwise the refresh cycle ends here for the whole process. + self?.scheduleSyncRetry() } } } @@ -203,32 +210,56 @@ private extension UserManager { /// `Formbricks.setup()`. Build the timer unscheduled and add it to the main run loop /// instead, the same way `UpdateQueue` hops to main for its debounce timer. func startSyncTimer() { - syncTimer?.invalidate() - syncTimer = nil - guard let expiresAt = expiresAt, let id = userId else { return } // A device clock running ahead of the server makes every `expiresAt` we receive // already in the past, which would otherwise sync in a tight loop. let interval = max(expiresAt.timeIntervalSinceNow, Config.User.minimumSyncIntervalInSeconds) + scheduleSync(after: interval, for: id) + } - let timer = Timer(timeInterval: interval, repeats: false) { [weak self] _ in - // The user may have been logged out or swapped while this was pending. - guard let self = self, self.userId == id else { return } - self.syncUser(withId: id) - } - syncTimer = timer - - // `.common` so an expiry that lands mid-scroll isn't postponed until the gesture ends. - onMain { RunLoop.main.add(timer, forMode: .common) } + /// Re-arms the sync after a failed request. + /// + /// Without this, one transient network failure ends the refresh cycle for the rest of the + /// process: the timer that fired is spent, and `startSyncTimer()` is otherwise only reached + /// from a successful sync. `expiresAt` still holds the value from the last success, so it is + /// not a usable cadence here — back off by the same interval the workspace-state path uses + /// for its errors instead of retrying at the minimum sync interval. + func scheduleSyncRetry() { + guard let id = userId else { return } + scheduleSync(after: Double(Config.User.retryAfterFailureInMinutes) * 60.0, for: id) } /// Cancels a pending user-state sync. Safe to call from any thread. func stopSyncTimer() { - let timer = syncTimer - syncTimer = nil - guard let timer = timer else { return } - onMain { timer.invalidate() } + onMain { [weak self] in + self?.syncTimer?.invalidate() + self?.syncTimer = nil + } + } + + /// Replaces any pending sync with one scheduled `interval` from now. + /// + /// Every read and write of `syncTimer` happens inside `onMain`. `startSyncTimer()` is called + /// from `syncUser`'s completion on URLSession's background delegate queue while + /// `stopSyncTimer()` can run from the main thread, so leaving the property unsynchronised + /// let the two writes race — and a lost `nil` write strands a live timer that nothing can + /// cancel afterwards. + func scheduleSync(after interval: TimeInterval, for id: String) { + onMain { [weak self] in + guard let self = self else { return } + self.syncTimer?.invalidate() + + let timer = Timer(timeInterval: interval, repeats: false) { [weak self] _ in + // The user may have been logged out or swapped while this was pending. + guard let self = self, self.userId == id else { return } + self.syncUser(withId: id) + } + self.syncTimer = timer + + // `.common` so an expiry that lands mid-scroll isn't postponed until the gesture ends. + RunLoop.main.add(timer, forMode: .common) + } } /// Runs `work` on the main thread, immediately if we are already there. `Timer` and diff --git a/Sources/FormbricksSDK/Networking/Queue/UpdateQueue.swift b/Sources/FormbricksSDK/Networking/Queue/UpdateQueue.swift index 97c6ae99..51128843 100644 --- a/Sources/FormbricksSDK/Networking/Queue/UpdateQueue.swift +++ b/Sources/FormbricksSDK/Networking/Queue/UpdateQueue.swift @@ -20,6 +20,10 @@ final class UpdateQueue { /// concurrent `POST /user` calls would race and whichever response landed last would /// overwrite `segments` / `displays` / `responses` wholesale. private var isSyncInFlight = false + /// A refresh that arrived while a sync was already airborne, replayed once that sync + /// finishes. The in-flight request was built *before* this interaction, so its response + /// cannot reflect it — dropping the nudge would leave segments stale until the next trigger. + private var pendingRefreshUserId: String? private weak var userManager: UserManagerSyncable? @@ -74,13 +78,17 @@ final class UpdateQueue { } /// Asks for the user state to be re-read from the server. Carries no new data — it exists - /// so an interaction that can change segment membership doesn't have to wait for the - /// state to expire. Dropped while a sync is already in flight, because that sync's - /// response already brings fresh segments. + /// so an interaction that can change segment membership doesn't have to wait for the state + /// to expire. + /// + /// While a sync is airborne the nudge is deferred rather than sent, because two concurrent + /// `POST /user` calls would race and the later response would overwrite `segments` / + /// `displays` / `responses` wholesale. It is replayed by `syncDidFinish()`. func requestUserStateRefresh(userId: String) { syncQueue.sync { guard !isSyncInFlight else { - Formbricks.logger?.debug("UpdateQueue - refresh skipped, a sync is already in flight") + Formbricks.logger?.debug("UpdateQueue - refresh deferred, a sync is already in flight") + pendingRefreshUserId = userId return } self.userId = userId @@ -88,11 +96,20 @@ final class UpdateQueue { } } - /// Called by the user manager once a sync finishes, so the next nudge can start a request. + /// Called by the user manager once a sync finishes. Releases the in-flight lock and replays + /// a refresh that arrived while the request was out. func syncDidFinish() { + var deferredUserId: String? syncQueue.sync { isSyncInFlight = false + deferredUserId = pendingRefreshUserId + pendingRefreshUserId = nil } + + guard let deferredUserId = deferredUserId else { return } + Formbricks.logger?.debug("UpdateQueue - replaying a refresh that arrived mid-sync") + // Outside the block above: `requestUserStateRefresh` takes the same queue. + requestUserStateRefresh(userId: deferredUserId) } func reset() { @@ -167,6 +184,8 @@ extension UpdateQueue { attributes = nil language = nil isSyncInFlight = false + // Teardown, unlike `reset()`: drop the deferred refresh instead of replaying it. + pendingRefreshUserId = nil } } } diff --git a/Tests/FormbricksSDKTests/SurveyInteractionRefreshTests.swift b/Tests/FormbricksSDKTests/SurveyInteractionRefreshTests.swift index 60ee8412..db554edd 100644 --- a/Tests/FormbricksSDKTests/SurveyInteractionRefreshTests.swift +++ b/Tests/FormbricksSDKTests/SurveyInteractionRefreshTests.swift @@ -18,9 +18,12 @@ private final class FakeScriptMessage: WKScriptMessage { /// Counts `postUser` calls so the interaction gate can be asserted end to end. private final class CountingMockService: MockFormbricksService { var postUserCallCount = 0 + /// Lets a test wait on the real request instead of guessing how long the debounce takes. + var onPostUser: ((Int) -> Void)? override func postUser(id: String, attributes: [String: AttributeValue]?, completion: @escaping (ResultType) -> Void) { postUserCallCount += 1 + onPostUser?(postUserCallCount) super.postUser(id: id, attributes: attributes, completion: completion) } } @@ -214,17 +217,19 @@ final class SurveyInteractionRefreshTests: XCTestCase { let userManager = UserManager(service: service) UserDefaults.standard.set("user-1", forKey: "userIdKey") + // Driven by the request itself rather than a fixed sleep, so a loaded machine can't + // make this flake. Over-fulfilment fails the test, which is the "exactly one" half. + let synced = expectation(description: "the gate lets one sync through") + synced.assertForOverFulfill = true + service.onPostUser = { _ in synced.fulfill() } + userManager.refreshSegmentsAfterInteraction( survey: survey(refresh: InteractionRefresh(onDisplay: true)), source: .onDisplay ) - let exp = expectation(description: "one sync") - DispatchQueue.main.asyncAfter(deadline: .now() + 0.6) { - XCTAssertEqual(service.postUserCallCount, 1) - exp.fulfill() - } - wait(for: [exp], timeout: 2.0) + wait(for: [synced], timeout: 5.0) + XCTAssertEqual(service.postUserCallCount, 1) } /// A display -> response -> finish burst must cost one request, not three. @@ -233,46 +238,81 @@ final class SurveyInteractionRefreshTests: XCTestCase { let userManager = UserManager(service: service) UserDefaults.standard.set("user-1", forKey: "userIdKey") + let synced = expectation(description: "burst coalesces into one sync") + synced.assertForOverFulfill = true + service.onPostUser = { _ in synced.fulfill() } + let allOn = survey(refresh: InteractionRefresh(onDisplay: true, onResponse: true, onFinished: true)) userManager.refreshSegmentsAfterInteraction(survey: allOn, source: .onDisplay) userManager.refreshSegmentsAfterInteraction(survey: allOn, source: .onResponse) userManager.refreshSegmentsAfterInteraction(survey: allOn, source: .onFinished) - let exp = expectation(description: "burst coalesces") - DispatchQueue.main.asyncAfter(deadline: .now() + 0.8) { - XCTAssertEqual(service.postUserCallCount, 1) - exp.fulfill() - } - wait(for: [exp], timeout: 2.0) + wait(for: [synced], timeout: 5.0) + XCTAssertEqual(service.postUserCallCount, 1) } - // MARK: - UpdateQueue in-flight join + // MARK: - UpdateQueue in-flight handling - func testRefreshIsDroppedWhileSyncIsInFlightAndResumesAfter() { + /// A nudge that lands mid-sync must be deferred and then replayed — not dropped. The + /// in-flight request was built before that interaction, so its response cannot reflect it. + func testRefreshDuringAnInFlightSyncIsDeferredThenReplayed() { let mockUserManager = MockUserManager() let queue = UpdateQueue(userManager: mockUserManager) defer { queue.cleanup() } queue.requestUserStateRefresh(userId: "user-1") - let firstCommit = expectation(description: "first commit") + let done = expectation(description: "deferred refresh is replayed") DispatchQueue.main.asyncAfter(deadline: .now() + 0.6) { // MockUserManager never reports completion, so the queue is still "in flight". XCTAssertEqual(mockUserManager.syncCallCount, 1) queue.requestUserStateRefresh(userId: "user-1") DispatchQueue.main.asyncAfter(deadline: .now() + 0.6) { - XCTAssertEqual(mockUserManager.syncCallCount, 1, "Nudge during an in-flight sync must be dropped") + XCTAssertEqual( + mockUserManager.syncCallCount, 1, + "A nudge during an in-flight sync must not start a second request" + ) + // Completing the sync must replay the deferred nudge on its own — without any + // further interaction. queue.syncDidFinish() - queue.requestUserStateRefresh(userId: "user-1") DispatchQueue.main.asyncAfter(deadline: .now() + 0.6) { - XCTAssertEqual(mockUserManager.syncCallCount, 2, "Nudge after completion must sync again") - firstCommit.fulfill() + XCTAssertEqual( + mockUserManager.syncCallCount, 2, + "The deferred refresh must be replayed once the sync finishes" + ) + done.fulfill() } } } - wait(for: [firstCommit], timeout: 5.0) + wait(for: [done], timeout: 5.0) + } + + /// Only one deferred refresh is kept, so a long sync with many interactions behind it + /// still costs a single follow-up request. + func testMultipleDeferredRefreshesCollapseIntoOneReplay() { + let mockUserManager = MockUserManager() + let queue = UpdateQueue(userManager: mockUserManager) + defer { queue.cleanup() } + + queue.requestUserStateRefresh(userId: "user-1") + + let done = expectation(description: "one replay") + DispatchQueue.main.asyncAfter(deadline: .now() + 0.6) { + XCTAssertEqual(mockUserManager.syncCallCount, 1) + + queue.requestUserStateRefresh(userId: "user-1") + queue.requestUserStateRefresh(userId: "user-1") + queue.requestUserStateRefresh(userId: "user-1") + + queue.syncDidFinish() + DispatchQueue.main.asyncAfter(deadline: .now() + 0.6) { + XCTAssertEqual(mockUserManager.syncCallCount, 2) + done.fulfill() + } + } + wait(for: [done], timeout: 5.0) } /// A commit with no user id must not leave the in-flight flag stuck, or every later @@ -448,6 +488,35 @@ final class SurveyInteractionRefreshTests: XCTestCase { XCTAssertGreaterThanOrEqual(service.postUserCallCount, 2) } + /// A failed sync must still leave a timer armed. Otherwise one transient network error ends + /// the refresh cycle for the rest of the process — the timer that fired is spent, and + /// `startSyncTimer()` is only reached from a successful sync. + func testFailedSyncReArmsTheTimer() { + let service = MockFormbricksService() + service.isErrorResponseNeeded = true + let userManager = UserManager(service: service) + UserDefaults.standard.set("user-1", forKey: "userIdKey") + + userManager.syncUser(withId: "user-1") + + let exp = expectation(description: "a retry is armed after the failure") + DispatchQueue.main.asyncAfter(deadline: .now() + 0.5) { + XCTAssertNotNil(userManager.syncTimer, "A failed sync must leave a retry scheduled") + XCTAssertEqual(userManager.syncTimer?.isValid, true) + exp.fulfill() + } + wait(for: [exp], timeout: 3.0) + } + + /// The retry backs off rather than hammering at the minimum sync interval, so a sustained + /// outage doesn't turn into a fixed-rate request stream. + func testFailureRetryBacksOffFurtherThanTheMinimumInterval() { + XCTAssertGreaterThan( + Double(Config.User.retryAfterFailureInMinutes) * 60.0, + Config.User.minimumSyncIntervalInSeconds + ) + } + /// A device clock ahead of the server makes every `expiresAt` land in the past. Without /// the floor the timer would fire immediately, re-sync, and loop. func testSkewedClockDoesNotCauseASyncLoop() {