fix: scope in-app polling to process lifecycle instead of per-activity#746
fix: scope in-app polling to process lifecycle instead of per-activity#746mahmoud-elmorabea wants to merge 3 commits into
Conversation
In-app message polling was started/stopped from per-activity ON_RESUME/ON_PAUSE callbacks. Because a modal in-app message runs in its own GistModalActivity, dismissing it resumed the host activity and triggered an immediate refetch that re-displayed a message which had failed to load — a tight retry loop. The same coupling also re-triggered polling on ordinary activity navigation. Scope polling to the process foreground lifecycle (ProcessLifecycleOwner), mirroring SseLifecycleManager, so a single polling timer survives activity navigation and modal display. A message that fails to load is now retried on the normal poll cadence (matching iOS and the web SDK) rather than in a tight loop. Added verification logs under the [Polling] tag. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sample app builds 📱Below you will find the list of the latest versions of the sample apps. It's recommended to always download the latest builds of the sample apps to accurately test the pull request. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #746 +/- ##
============================================
- Coverage 69.07% 68.09% -0.98%
- Complexity 838 890 +52
============================================
Files 149 158 +9
Lines 4601 4912 +311
Branches 628 660 +32
============================================
+ Hits 3178 3345 +167
- Misses 1189 1323 +134
- Partials 234 244 +10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
📏 SDK Binary Size Comparison Report
|
|
Build available to test |
Move the process-lifecycle polling logic out of GistSdk into a dedicated PollingLifecycleManager, parallel to SseLifecycleManager. GistSdk becomes a thin GistProvider facade again (no lifecycle/timer/subscription state). Both transports are now singletons scoped to the same process lifecycle, which also removes a latent hazard: the GistProvider- and GistSdk-typed DI singletons each constructed a separate GistSdk (the middleware used the latter), so a second instance with its own polling timer could be created. Polling now lives in a single PollingLifecycleManager singleton; the middleware and the dismissal fetch path call it directly, and the duplicate gistSdk singleton is removed. No behavior change. Polling unit coverage moves from GistSDKTest into PollingLifecycleManagerTest. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 41abdcb. Configure here.
|
|
mrehan27
left a comment
mrehan27
left a comment
There was a problem hiding this comment.
Changes look good overall. As I mentioned in slack earlier, it would be good to call out the slight behavior change so everyone is aware of it and aligned with expectations.
| resetTimer() | ||
| } else { | ||
| logger.debug("[Polling] $reason - SSE not active, ensuring polling is running") | ||
| startPolling(duration = currentState.pollInterval) |
There was a problem hiding this comment.
Copying feedback from Claude, would be nice to validate this:
This starts polling with initialDelay=0 (immediate fetch). On an anonymous→identified transition while SSE is disabled, this fires fetchUserMessages() at the same time as ModuleMessagingInApp's UserChangedEvent handler, which also calls fetchInAppMessages() — so we get two near-simultaneous fetches.
The old code deliberately avoided this: its isUserIdentified/sseEnabled subscriptions only stopped polling ("Starting polling is handled by … event handlers"). Same redundant restart happens on an sseEnabled false→true flip while the user is anonymous (shouldUseSse stays false → pointless timer reset + fetch).
Suggest restarting here without the immediate fetch (use initialDelay = currentState.pollInterval, letting the event handlers own the catch-up fetch), or guarding startPolling so it doesn't restart when a timer with the same interval is already running.
There was a problem hiding this comment.
Good callout, fixed!
Two guards so the attribute subscriptions only act on genuine transitions: - Skip the pollInterval subscription's initial replay emission. The initial/ foreground start is owned by handleForegrounded()/fetchInAppMessages(); reacting to the replay could cancel that in-flight catch-up fetch and reschedule a full interval out (addresses the Bugbot finding). - In the SSE-availability handler, only (re)start polling when the timer isn't already running, i.e. when transitioning away from SSE. Previously every identification or sseEnabled flip that left shouldUseSse=false caused a redundant timer reset + duplicate fetch (e.g. anon->identified with SSE off, where ModuleMessagingInApp already fetches; or an sseEnabled flip while anonymous). Restores the original "subscriptions only stop polling, event handlers own starting" behavior while still resuming on a real SSE->off flip. timer is now @volatile for cross-thread visibility. Adds unit tests for both guards. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
|
2 participants
Summary
Scopes in-app message queue polling to the process foreground lifecycle (
ProcessLifecycleOwner) instead of per-activityON_RESUME/ON_PAUSEcallbacks, mirroring howSseLifecycleManageralready works.Previously, because a modal in-app runs in its own
GistModalActivity, dismissing it resumed the host activity and triggered an immediate refetch — which re-displayed a message that had failed to load, in a tight retry loop. The same coupling also restarted polling on ordinary activity navigation. With this change a single polling timer survives activity navigation and modal display, and a failed-to-load message is retried only on the normal poll cadence (matching iOS and the web SDK).Added
[Polling]verification logs for foreground/background/poll-tick transitions.Test plan
:messaginginapp:testDebugUnitTestgreen; newGistSDKTestcases for foreground-starts-polling, SSE-active-skips-polling, and not-foregrounded-skips-polling.🤖 Generated with Claude Code
Note
Medium Risk
Changes core in-app fetch timing and lifecycle wiring; behavior is well covered by new unit tests but affects message delivery edge cases (foreground, SSE handoff, dismiss).
Overview
In-app queue polling is extracted from
GistSdkinto a newPollingLifecycleManagerregistered onProcessLifecycleOwner(onStart/onStop), aligned withSseLifecycleManager, instead of listening to each activity’sON_RESUME/ON_PAUSE(with special-casing forGistModalActivity).That keeps one timer across navigation and while a modal is shown, so closing or failing a modal no longer forces an immediate refetch and retry loop; fetches follow the normal poll cadence when polling is active. Polling still stops when SSE is active (
shouldUseSse), with guards to avoid redundant restarts on poll-interval replay, SSE flag, and identification changes.GistSdkbecomes a thin facade that holds the lifecycle managers for side-effect init and delegatesfetchInAppMessages,reset, and dismissal-driven refresh viapollingLifecycleManager. DI adds a singletonpollingLifecycleManager; tests addPollingLifecycleManagerTestand update mocks across the module.Reviewed by Cursor Bugbot for commit 6c6fa01. Bugbot is set up for automated code reviews on this repo. Configure here.