-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Reader Bugfixes #25016
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 Bugfixes #25016
Conversation
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 29968 | |
| Version | PR #25016 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | dbecfbe | |
| Installation URL | 50bfpp4v498q8 |
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 29968 | |
| Version | PR #25016 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | dbecfbe | |
| Installation URL | 12bg6i2f1efg0 |
cd46b6b to
1709330
Compare
| case .blogsPost: | ||
| return "/read/blogs/:blog_id/posts/:post_id" | ||
| case .wpcomPost: | ||
| return "/:post_year/:post_month/:post_day/:post_name" |
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.
This can't be removed without more consideration as it's the top universal link in the app.
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've reworked this to unconditionally resolve the URL against WP.com without making any local assumptions.
1709330 to
4d56923
Compare
| success(ResolvedReaderPost(postId: postId, siteId: siteId)) | ||
| } failure: { error, response in | ||
| failure(error) | ||
| } |
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.
Nit: you can use the async API, which supports Decodable, in wordPressComRestApi.perform(...) (which is different from wordPressComRESTAPI)
|
Does this PR fix the issue where the app can't handle the post link from the web reader? Simulator.Screen.Recording.-.iPhone.17.-.2025-11-27.at.10.23.23.mov |
|
I probably pressed some unknown key combination that accidentally closed the PR. 🤦♂️ |
|
BTW, do you want to target the 26.5 release branch? |
Rebased, thanks! |
|
The base branch is set to |
Oh man, one day I'll learn to read 🤦 |
Generated by 🚫 Danger |
|
@jkmassel You may need to rebase this PR to remove some commits that were merged into trunk after the RC was cut. |
fa08f72 to
bcbf5dc
Compare
- Remove unused `Decodable` conformance from `ResolvedReaderPost` - Add `[weak self]` to closures in `fetch(_ url:)` to prevent retain cycles - Simplify nested conditionals in `contentIsAlreadyDisplayed` - Add debug logging when skipping duplicate content presentation - Make `self.` usage consistent 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Simplify nested conditionals in `contentIsAlreadyDisplayed(_:in nav:)` to use guard-let pattern - Add clarifying comment for UInt64 type requirement in resolveUrl 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
bcbf5dc to
3040de4
Compare
523028f addresses this :) |
ca3e5c9 to
523028f
Compare
|
@jkmassel The WordPress.com News link in the Reader can't be opened in the app. I have fixed that in the commit above. Feel free to make additional changes if needed. |
|
kean
left a comment
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.
It looks good. I tested the banner and some other scenarios as well. I found one issue and will send it separately.
Left one small comment about release notes.
| * [*] Add support for editing custom taxonomy terms from "Post Settings" [#24964] | ||
| * [*] Fix overly long related post titles in Reader [#25011] | ||
| * [*] Increase number of lines for post tiles in Reader to three [#25019] | ||
| * [*] Fix horizontal insets in Reader article view [#25010] |
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.
These entries are now duplicated.





Description
Fixes a few reported reader issues:
Opening a Reader link multiple times doesn't cause the view to reload on top of the old one. Try the above links, then go back to the browser and try them again. None should be replaced inline.
Fixes an issue where where going to https://wordpress.com/read after https://wordpress.com/reader/feeds/138734090 wouldn't load the reader root. It is added to the top of the nav stack instead of popping the stack down to the root to more closely imitate how a browser works.
Removes a bunch of code causing problems – there is no good way to load a post into the reader by its URL. Thus, we'll use WP.com to resolve all reader URLs to a site ID and post ID and load them that way.
Testing instructions
See above
Consider testing the following yourself before requesting a review:
-->