-
Notifications
You must be signed in to change notification settings - Fork 22
fix: use fetchRequest instead of in memory predicate - WPB-17064 #2809
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: develop
Are you sure you want to change the base?
Conversation
Test Results2 425 tests 2 420 ✅ 4m 18s ⏱️ For more details on these failures, see this check. Results for commit c657f4d. ♻️ This comment has been updated with latest results. |
Datadog ReportBranch report: ❌ 5 Failed (0 Known Flaky), 2420 Passed, 0 Skipped, 4m 17.91s Total Time ❌ Failed Tests (5)
|
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.
Looks good 👍
Only concern, is it covered by any test, implicitly or explicitly?
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.
I'm actually very surprised this is an improvement. Any idea what specifically was the bottleneck before?
415243c
to
ee3f660
Compare
@dmitrysimkin yes actually I forgot to update the tests |
This PR is stale because it has been open 30 days with no activity. Please update it or close it in case is not relevant anymore. |
Issue
When resuming the app, the conversation lists are recreated and conversations filtered and sort again. This is done in memory and causes a hang when applying the filtering (touching conversationType).
Use fetchRequests for all filters instead.
Note: Eventually, we could replace ConversationList and ConversationListSnapshot with just a fetchrequestController that would be applied a predicate each time.
Testing
Observe no hang is produced
Checklist
[WPB-XXX]
.UI accessibility checklist
If your PR includes UI changes, please utilize this checklist: