Fix an issue with duplicated subscriptions for Jetpack-connected sites #25026
Conversation
kean
force-pushed
the
fix/reader-subscriptions-duplicated
branch
from
dd694a3 to
875c009
Compare
Generated by 🚫 Danger |
| /// Site/Feed URL with fallback: data.site → data.feed | ||
| /// Prioritizes site URL over feed URL for canonical representation | ||
| public var url: URL? { | ||
| site?.url ?? feed?.url |
There was a problem hiding this comment.
I opted to skip reading the fields inlined in the response and use data.feed and data.site directly as these seem to be more reliable for all three scenarios.
kean
requested review from
crazytonyli and
jkmassel
and removed request for
jkmassel
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 30031 | |
| Version | PR #25026 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 211a3cb | |
| Installation URL | 2gv32dpon2q68 |
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 30031 | |
| Version | PR #25026 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 211a3cb | |
| Installation URL | 51f89vieulqug |
| let data = try? meta.nestedContainer(keyedBy: DataKeys.self, forKey: .data) { | ||
| self.feed = try? data.decode(FeedData.self, forKey: .feed) | ||
| self.site = try? data.decode(SiteData.self, forKey: .site) | ||
| } |
There was a problem hiding this comment.
I feel like manually decoding defeats the purpose of using Decodable. The code would look cleaner if we use types that match the JSON structure, like
public struct ReaderFeed: Decodable {
public init(from decoder: Decoder) throws {
let feed = try Feed(from: decoder)
self.feed = feed.meta?.data?.feed
self.site = feed.meta?.data?.site
}
}
// For parsing JSON only.
private struct Feed: Decodable {
struct Meta: Decodable {
struct Data: Decodable {
var feed: FeedData?
var site: SiteData?
}
var data: Data?
}
var meta: Meta?
}There was a problem hiding this comment.
That makes sense. I added ReaderFeedJSON just like that. It's a bit easier to follow now.
| let feedID: String? | ||
| let blogID: String? | ||
| let name: String? | ||
| let url: URL? |
There was a problem hiding this comment.
During testing the JSON parsing above, I found out by accident that this url can be an empty string, which causes JSONDecoder to throw an error, which is silenced at the moment because of the try? parsing code above.
You can search "WordPress.com News" and check out the "wpsites.net" site in the response.
There was a problem hiding this comment.
I think we can change the URL properties to be String, and turn the string to URL in the ReaderFeed public API, which are all optional anyway.
There was a problem hiding this comment.
I added a custom init(from decoder:) for FeedData (same as SiteData already has). I don't trust every field to be the correct format, so it all try?s now. It will catch the URL issue too.
…eading to duplicated subscriptions
kean
force-pushed
the
fix/reader-subscriptions-duplicated
branch
from
875c009 to
8ab5420
Compare
crazytonyli
left a comment
crazytonyli
left a comment
There was a problem hiding this comment.
The changes look good ot me, but the unit tests failed due to parsing the stubbed JSON files.
I fixed the tests by adding "inline data" parsing back. It should not be necessary in production, but it's just some defensive code. |
|
Fixes https://linear.app/a8c/issue/CMM-1007/reader-subscriptions-to-jetpack-connected-sites-made-from-search-are (see ticket for RCA).
Testing Instructions
Repeat the steps for an RSS feed, a Dotcom site, and a Jetpack-connected site. I covered all three scenarios with unit tests.
Recording
Screen.Recording.2025-11-27.at.3.35.03.PM.mov