-
Notifications
You must be signed in to change notification settings - Fork 28
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
fix: reusing PagingData crash [WPB-15055][WPB-15079][WPB-15064] 🍒 #3778
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3778 +/- ##
===========================================
+ Coverage 45.80% 45.82% +0.02%
===========================================
Files 483 483
Lines 16513 16522 +9
Branches 2783 2783
===========================================
+ Hits 7563 7571 +8
- Misses 8170 8171 +1
Partials 780 780
Continue to review full report in Codecov by Sentry.
|
Built wire-android-staging-compat-pr-3778.apk is available for download |
Built wire-android-dev-debug-pr-3778.apk is available for download |
Quality Gate passedIssues Measures |
Built wire-android-staging-compat-pr-3778.apk is available for download |
Built wire-android-dev-debug-pr-3778.apk is available for download |
This PR was manually cherry-picked based on the following PR:
Original PR description:
PR Submission Checklist for internal contributors
The PR Title
SQPIT-764
The PR Description
What's new in this PR?
Issues
There are crashes related to reusing
PagingData
flow when doing some actions on paginated conversation list screen, like accepting legal hold request, answering 1:1 call or opening pending connection request which can fetch user data.Causes (Optional)
Each action that modifies (or even puts the same exact data but just requires executing insert, update or delete query) a table that then affects paginated conversation list query, makes this query dirty and executes it again, which reloads the list.
When using
combine
, it creates a new instance of flow, so if thecombine
is used afterPagingData
is created then if the data is changed and emitted by the flow that's combined, it will re-use the samePagingData
and put it into a new flow, socachedIn
won't cache the flow properly and it will crash becausePagingData
cannot be used twice.Solutions
Move
combine
"above" thePagingData
flow so that each time a new value is emitted, it will always create a newPagingData
for the new flow and alsocachedIn
will be used correctly.Simplify
showLoading
parameter to be a simple logical equation rather than a state. Make use of mockedpreviewConversationFoldersFlow
with properLoadStates
to represent loaded data instead of using specificinitiallyLoaded
parameter.Fix
rememberSearchbarState
so that it actually saves thesearchQueryTextState
.Testing
Test Coverage (Optional)
How to Test
The crash was happening when opening pending connection request (most frequently), answering 1:1 call or accepting legal hold request, so these actions shouldn't crash the app. Also, after opening connection request or 1:1 conversation, if your or that user's data hasn't changed since the last fetch, then it shouldn't refresh the conversation list when navigating back.
Attachments (Optional)
refreshing_when_going_back.mp4
no_refreshing_when_going_back.mp4
pagination_crash.mp4
PR Post Submission Checklist for internal contributors (Optional)
PR Post Merge Checklist for internal contributors
References
feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764
.