-
Notifications
You must be signed in to change notification settings - Fork 9
chore: SSE bug fixes #639
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feature/in-app-sse
Are you sure you want to change the base?
chore: SSE bug fixes #639
Conversation
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 @@
## feature/in-app-sse #639 +/- ##
========================================================
+ Coverage 67.85% 68.18% +0.32%
- Complexity 748 752 +4
========================================================
Files 142 142
Lines 4303 4303
Branches 580 580
========================================================
+ Hits 2920 2934 +14
+ Misses 1165 1147 -18
- Partials 218 222 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
|
Build available to test |
📏 SDK Binary Size Comparison Report
|
| if (!isPersistentMessage()) { | ||
| inAppManager.dispatch(InAppMessagingAction.DismissMessage(message = state.message)) | ||
|
|
||
| if (ourMessage != null && inAppManager != null && ourMessage.queueId == displayedMessage?.queueId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Null queueId comparison incorrectly matches non-displayed state
The condition ourMessage.queueId == displayedMessage?.queueId can incorrectly evaluate to true when displayedMessage is null (no message currently displayed) and ourMessage.queueId is also null, since null == null is true in Kotlin. This defeats the race condition fix because a message with a null queueId would trigger a dismiss dispatch even when no message is currently in the Displayed state. The check needs to ensure displayedMessage is non-null before comparing queueId values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Gist activity is always started with a message
|
|
The issue
This PR fixes an issue for in-app Android. Sometimes when there are multiple messages the
GistModalActivityanimation takes longer than the processor to update the state. In which case, the activity dismisses the wrong message.onDestroyof old activity runs and grabs the current message -> which is now message 2Dismisses it as per this
This is not really related to SSE but it's more apparent because SSE shows subsequent messages faster.
The fix
GistModalActivityis created, it holds on to the message it was created to showonDestroyruns, we make sure not to call dismiss unless our message is being shownNote
Addresses a race condition where
GistModalActivitycould dismiss the wrong modal during transitions.activityMessageand inonDestroyonly dismisses if it matches the currently displayed message (byqueueId); refactorsisPersistentMessage(Message?)store.state.shouldUseSse(instead ofsseEnabled) to triggerProcessMessageQueueafter dismissing a messageWritten by Cursor Bugbot for commit 1a8bb41. This will update automatically on new commits. Configure here.