-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Reader: Fix select interests unregistered cell #23158
Conversation
📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
|
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 comments I left were minor and not blocking this PR. I'm fine with leaving in the @MainActor
's if you want to keep them.
LGTM! 🎉
@@ -63,35 +63,41 @@ class ReaderInterestsDataSource { | |||
} | |||
|
|||
/// Fetches the interests from the topic service | |||
public func reload() { | |||
public func reload(completion: (() -> Void)? = nil) { | |||
interestsService.fetchInterests(success: { [weak self] interests in |
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 thought all our API calls already call their success and failure blocks on the main thread, so I wonder if adding @MainActor
changes anything.
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.
Good point. You're right, the API already dispatches the completion blocks to the main thread. My assumption is that the interests
variable got updated while the collection view is calling cellForItemAtIndexPath
for the indexes before it got updated, which led to the guard condition.
However, even if I'm dispatching it again to the main thread—and, even if it works—it doesn't guarantee that the indexes will be correct when cellForItemAtIndexPath
is called, since we don't know if the collection view still holds onto the "old" numberOfItemsInSection
value.
Perhaps the best way is to move the implementation to a UICollectionViewDiffableDataSource
... For now, I'll revert the dispatches to main queue and will just keep the empty cell registration.
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.
Addressed in f560543.
@@ -383,6 +385,7 @@ extension ReaderSelectInterestsViewController: UICollectionViewDelegateFlowLayou | |||
|
|||
// MARK: - ReaderInterestsDataDelegate | |||
extension ReaderSelectInterestsViewController: ReaderInterestsDataDelegate { | |||
@MainActor |
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.
Kind of the same comment here, I would think that calling reloadData
on a background thread would've caused a warning/crash. So my guess is this was already being called on the main thread.
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.
Addressed in #23158 (comment). Thanks!
59be791
to
f560543
Compare
Fixes #22932
Potentially fixes an issue where an unregistered collection view cell is returned from the
collectionView:cellForItemAtIndexPath:
.The crash is caused by the collection asking for an invalid index path, which is already guarded against and returns an empty
UICollectionViewCell
. However, this crashed because the cell is not registered... 😅 I believe the root cause of this is the invalid index path, which is likely to be a race condition due to updating theinterests
variable while the collection view is updating its cells.To fix/mitigate this, I've:
Dispatched the logic that updatesSee Reader: Fix select interests unregistered cell #23158 (comment)interests
AND reloads the collection view to the main thread.UICollectionViewCell
so that if the race condition is still happening, it doesn't immediately crash. It should be fine to show an empty cell since we should display a NoResults screen.To test
Note that I wasn't able to reproduce the crash, but these are the reproduction steps based on the logs:
Regression Notes
Potential unintended areas of impact
Should be none.
What I did to test those areas of impact (or what existing automated tests I relied on)
Manually tested the changes.
What automated tests I added (or what prevented me from doing so)
N/A.
PR submission checklist:
RELEASE-NOTES.txt
if necessary.Testing checklist: