feat: support survey-interaction segment filters (ENG-1275) + fix survey-display crash (#43) - #56
Merged
Merged
Conversation
`IllegalStateException: FragmentManager has been destroyed`, reported at #43. Three defects stacked up: 1. The fragment transaction was committed on a timer thread. SurveyManager schedules the display on a `java.util.Timer`, whose task called `showSurvey` directly, which reaches `DialogFragment.show()`. androidx requires fragment transactions on the main thread. This is not delay-specific: a zero delay still schedules through the timer, so every display took that path. 2. The stored FragmentManager can outlive the Activity it came from. It is captured once at setup, so in a login -> dashboard flow it belongs to an Activity that has already finished. That is why the reporter saw the crash on first login only and not on later launches. 3. Nothing checked whether the manager was still usable. `showSurvey` now hops to the main looper, skips a destroyed manager with an error that tells the host to call `setFragmentManager` from the current Activity, skips a state-saved manager (a commit after onSaveInstanceState throws, and the host is on its way to the background anyway), and wraps the commit so a survey that fails to show can never take the host app down. As a side effect the timer task now only posts to the main looper, so it can no longer throw — a throwing TimerTask permanently cancels `java.util.Timer`, which would have stopped every later survey in that process.
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 flags are nullable on purpose: Gson does not run Kotlin
default-value initialisers, so a partial object from the server would otherwise
leave a non-nullable Boolean undefined.
- 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 UpdateQueue so a display -> response -> finish burst is
debounced into one request. The one-shot guard lives in
SurveyInteractionForwarder so it is testable outside the fragment.
- Give UpdateQueue an in-flight join and a lock. Nothing serialised its state,
and two concurrent POST /user calls could race so the later response would
overwrite segments, displays and responses wholesale.
- Make the debounce timer one-shot. It was a repeating timer that cancelled
itself from inside its own task, reading the shared `timer` field from the
timer thread — so a newer timer scheduled in the meantime could be cancelled
instead, silently dropping that update.
Also hardens the user-state sync timer, which this feature depends on:
`startSyncTimer` was only ever reached from a successful sync, so a launch that
found a warm cache never scheduled a refresh and segments stayed frozen for the
whole process. It is now also armed when the cached state is still valid. The
delay is computed relative to now and clamped, because a device clock running
ahead of the server puts every `expiresAt` in the past, which java.util.Timer
runs immediately and would loop. A pending sync is cancelled on logout and the
task re-checks the user id, so a logged-out user is never re-synced.
WalkthroughAdded survey interaction refresh configuration for display, response, and completion events. Added manager and queue logic that validates users, deduplicates refreshes, and coordinates synchronization. Added WebView completion event forwarding with per-showing source tracking. Updated survey presentation to run on the main thread and handle invalid or destroyed fragment managers. Added instrumented tests for decoding, gating, lookup, forwarding, and completion handling. 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@android/src/main/java/com/formbricks/android/Formbricks.kt`:
- Around line 335-341: Update the showSurvey flow in Formbricks.kt so
fragmentManager is read inside the Handler(Looper.getMainLooper()).post runnable
instead of being captured before posting. Keep the existing null check and
SDKError.fragmentManagerIsNotSet behavior, but use the current manager at
execution time so the fragment transaction commits against the live Activity
state rather than a potentially destroyed one.
In `@android/src/main/java/com/formbricks/android/network/queue/UpdateQueue.kt`:
- Around line 68-76: Make UpdateQueue and UserManager use one synchronized state
machine for refreshes: in
android/src/main/java/com/formbricks/android/network/queue/UpdateQueue.kt#L68-L76,
retain pending work from requestUserStateRefresh instead of dropping it; at
`#L118-L123`, have commit defer while isSyncInFlight; in
android/src/main/java/com/formbricks/android/manager/UserManager.kt#L200-L206,
schedule the pending snapshot only after the active sync completes; and at
`#L257-L278`, route expiry-driven syncs through the same guarded path. Do not
start concurrent requests, and preserve the latest pending refresh until
completion.
🪄 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: c6db0063-c45d-4566-9a11-d218e90da4a5
📒 Files selected for processing (14)
android/src/androidTest/java/com/formbricks/android/manager/SurveyInteractionRefreshInstrumentedTest.ktandroid/src/androidTest/java/com/formbricks/android/webview/WebAppInterfaceInstrumentedTest.ktandroid/src/main/java/com/formbricks/android/Formbricks.ktandroid/src/main/java/com/formbricks/android/manager/SurveyManager.ktandroid/src/main/java/com/formbricks/android/manager/UserManager.ktandroid/src/main/java/com/formbricks/android/model/error/SDKError.ktandroid/src/main/java/com/formbricks/android/model/javascript/EventType.ktandroid/src/main/java/com/formbricks/android/model/workspace/InteractionRefresh.ktandroid/src/main/java/com/formbricks/android/model/workspace/Survey.ktandroid/src/main/java/com/formbricks/android/network/queue/UpdateQueue.ktandroid/src/main/java/com/formbricks/android/webview/FormbricksFragment.ktandroid/src/main/java/com/formbricks/android/webview/FormbricksViewModel.ktandroid/src/main/java/com/formbricks/android/webview/SurveyInteractionForwarder.ktandroid/src/main/java/com/formbricks/android/webview/WebAppInterface.kt
…retry - Read `fragmentManager` inside the main-thread runnable instead of capturing it at schedule time. The host can hand us a newer one via `setFragmentManager` between the two, and the stale reference would report "destroyed" while a usable manager was available. - 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 the latest deferred refresh is kept, so many interactions behind a slow sync still cost a single follow-up. `syncDidFinish()` drains it and is now called on the success path too; `reset()` deliberately keeps it, and logout clears it via `clearPendingRefresh()`. - Re-arm the sync when the request fails. The task 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 whole process. The retry backs off by `RETRY_AFTER_FAILURE_MS` rather than the minimum sync interval, so a sustained outage doesn't become a fixed-rate request stream. Not addressed, and worth its own ticket: `commit()` can still start a request while a sync is in flight, and the expiry task calls `syncUser` directly rather than through the queue. Both predate this PR — before it there was no in-flight flag at all, so every commit raced. Making attribute-carrying commits wait on a sync changes when user data reaches the backend, which needs its own design rather than a drive-by change here. 4 more instrumented tests, bringing the file to 19.
|
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.
Two commits, reviewable separately. The crash fix is unrelated to the feature but small, so it rides along.
1.
feat: survey-interaction segment filters (ENG-1275)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. Android 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.
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:
Absent for workspaces that don't use interaction targeting, and present-but-all-false for surveys no interaction filter references — both handled.
The flags are
Boolean?rather thanBoolean = falseon purpose: Gson does not run Kotlin default-value initialisers, so a partial object from the server would leave a non-nullableBooleanin an undefined state. Nullable plus== truetreats anything missing or non-boolean as "do not refresh".The missing completion signal —
onFinishedonFinishedhas always been a prop of the surveys library, but Android never passed it in, sohaveCompleted/haveNotCompletedhad no client-side trigger at all. Added toEventType, theWebAppCallbackinterface, the fragment, and the JS harness.Because we pass
getSetIsResponseSendingFinished,isResponseSendingFinishedstartsfalse, so on app surveysonFinishedgenuinely means the finished response was accepted by the backend — not merely "the UI finished".The refresh
Gated twice, because a
/usersync is not cheap:Routed through
UpdateQueueso a display → response → finish burst debounces into one request. The one-shot guard lives in a smallSurveyInteractionForwarderrather than inside the fragment, so it can actually be tested.Two
UpdateQueuebugs found on the wayPOST /usercalls could race and whichever response landed last would overwritesegments/displays/responseswholesale. Added a lock and a drop-while-in-flight rule.timerfield from the timer thread. A newer timer scheduled in the meantime could be cancelled instead of the intended one, silently dropping that update. Now one-shot, so there is nothing to self-cancel.Sync-timer hardening
startSyncTimerhad exactly one caller — the success path ofsyncUser. So a launch that found a still-valid cached state scheduled nothing, and segments stayed frozen for the whole process. It is now also armed fromsyncUserStateIfNeeded's else branch.Two more issues on that path:
Date. A device clock running ahead of the server puts everyexpiresAtin the device's past, whichjava.util.Timerruns immediately → sync → past again → tight loop. Now a relative delay with a floor.Verification
compileDebugKotlinandcompileDebugAndroidTestKotlinboth pass.15 new instrumented tests in
SurveyInteractionRefreshInstrumentedTestcover decoding (absent / full / partial / all-false / non-boolean / unknown key), the gate (anonymous, absent, all-false, mismatched source, matching source), the survey lookup (unknown id, null id, and that the matching survey's flags are used rather than the first), and one-shot forwarding. PlustestMessage_onFinishedin the existing bridge test.They observe
UpdateQueue's pending user id via reflection — the pattern this repo already uses — so the assertions are deterministic: no debounce waits and no network.Important
I have not been able to run the instrumented tests — no emulator or device was available. They compile, but that is not the same as passing. Worth running before merge.
2.
fix: survey display crashing the host app (#43)IllegalStateException: FragmentManager has been destroyed. The reported stack trace ends injava.util.TimerThread.run, which is the tell. Three defects stacked:SurveyManagerschedules the display on ajava.util.Timer, whose task calledshowSurveydirectly →DialogFragment.show(). androidx requires fragment transactions on the main thread. Not delay-specific either:Timer.schedule(task, Date(now))still runs on the timer thread, so every display took that path.FragmentManagercan outlive its Activity. It is captured once at setup, so in a login → dashboard flow it belongs to an Activity that has already finished. That explains the reporter's "crashes on first login, fine on later launches".showSurveynow hops to the main looper, then:setFragmentManagerfrom the Activity currently on screenonSaveInstanceStatethrows, and the host is heading to the background anywaySide benefit: the timer task now only posts to the main looper, so it can no longer throw. A throwing
TimerTaskpermanently cancelsjava.util.Timer, which would have killed every later survey in that process.Note
No automated test for this one. Producing a genuinely destroyed
FragmentManagerneeds an Activity harness (fragment-testingor a test Activity), neither of which exists in this repo, and I could not run it to verify. Happy to add the harness in a follow-up if you'd like it covered.Notes for reviewers
interactionRefresh— identical decoded object graph. 2.0.0 is Gson (ignores unknown keys, no strict mode configured); older tags setignoreUnknownKeys = true. Old apps are unaffected.SurveyInteractionForwarderis a new internal class. It exists purely to move the one-shot guard out ofFormbricksFragment, which is untestable without instrumentation. Happy to inline it if you'd rather.onSurveyInteractionuses?: returninstead of theguardhelper.guard's fallback isT::class.java.newInstance(), which would throw for a data class likeSurvey.isShowingSurveyis left alone. It is set atSurveyManager.kt:208and never read or reset in production — only in tests. It guards nothing and never clears, unlike iOS. Existing tests depend on its current value, so it deserves its own ticket rather than a drive-by change here.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. The web SDK has the same characteristic, so this matches it rather than diverging. The real fix is upstream.