Skip to content

Conversation

@crazytonyli
Copy link
Contributor

Issue

In the Reader tab, when filter by a topic, only first page of posts are loaded. When scroll to the bottom of the posts list, the new posts are not displayed.

Root cause

ReaderStreamViewController was refactored in my previous PR. Before this refactor, the view controller hold a ReaderPostSerivce instance as a stored property. The PR removed this store property, used temporary instances when needed instead (see this example).

Creating a service instance and discarding it when done is a common pattern throughout the app, I thought it should be okay to do the same here. But I didn't realize the posts pagination info is accessed in ReaderPostSerivce.fetchPostsV2. Since ReaderPostSerivce instances are created and discarded, the pagination info is also discarded, which is why the posts in second page are not loaded.

Fix

I've moved the fetchPostsV2 into its own ReaderPostStreamService type, so that we can keep the pagination info around in the view controller.

Reason we can't simply store ReaderPostSerivce instances as a property of the view controller is we don't want to create a new background context (which is a deprecated pattern) to initialize a ReaderPostSerivce instance.

Test Instructions

Go to Reader tab, in the "Discover" section, select a topic/tag, scroll to bottom the posts list, verify new posts can be loaded.

Regression Notes

  1. Potential unintended areas of impact
    None.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    None.

  3. What automated tests I added (or what prevented me from doing so)
    None.

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@crazytonyli crazytonyli added this to the 21.6 milestone Jan 9, 2023
@wpmobilebot
Copy link
Contributor

You can test the changes in WordPress from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr19880-ad9f779 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@wpmobilebot
Copy link
Contributor

You can test the changes in Jetpack from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr19880-ad9f779 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

Comment on lines +44 to +46
let serivce = ReaderPostService(managedObjectContext: context)
serivce.deletePostsInExcessOfMaxAllowed(for: readerTopic)
serivce.deletePostsFromBlockedSites()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it guaranteed that ReaderPostService will be retained during this operation? Do we not need to keep a reference to it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By keeping a reference, do you mean saving it a stored property? If that's the case, I don't think we need to do that. With changes in this PR, ReaderPostSerivce is now stateless, like almost all other "service" types. So, it can be used in a create-and-discard way, like almost all other "service" types.

Copy link
Contributor

@alpavanoglu alpavanoglu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue seems to have resolved. I've just left a question to highlight a possibility. If that's fine, we can go on and merge.

@crazytonyli crazytonyli merged commit fc64c0b into trunk Jan 10, 2023
@crazytonyli crazytonyli deleted the tonyli-fix-reader-topic-posts-loading-issue branch January 10, 2023 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants