Basic support of custom post types

[crazytonyli]

Description

This PR adds basic support for custom post types, which is behind a local feature flag (on by default in debug builds).

Please note that, during development, I found a few parsing issues. I suspect there are more. We'll need to try a few more plugins that register custom post types (see 1 below).

Supported features:

1. View custom post types that are declared as publicly viewable. If you can open the custom posts in the post editor on the web, then you should be able to see them in the app.
2. View existing posts in the custom post types. No support for creating new posts.
3. Filtering by post status.
4. Search.
5. Edit and save the posts in the GBK editor.

Plugins

Here are a few plugins that I tested. You can install them, create some test data, and view them in the app.

• WooCommerce
• Sensei LMS
• WP Job Manager
• Seriously Simple Podcasting
• Easy Digital Downloads

Testing instructions

You can use cpt.wpmt.co to view the books on the site. Please note that not all books can be loaded (see p1768466053646389-slack-C06S1QS55K9).

You can install WP Job Manager on test sites, which registers a "job listing" custom post type.

[dangermattic]

	2 Warnings
⚠️	View files have been modified, but no screenshot or video is included in the pull request. Consider adding some for clarity.
⚠️	This PR is larger than 500 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

[wpmobilebot]

📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.

	App Name	Jetpack
	Configuration	Release-Alpha
	Build Number	30634
	Version	PR #25157
	Bundle ID	com.jetpack.alpha
	Commit	4bec4e1
	Installation URL	241qvovlg0m10

Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

[wpmobilebot]

📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.

	App Name	WordPress
	Configuration	Release-Alpha
	Build Number	30634
	Version	PR #25157
	Bundle ID	org.wordpress.alpha
	Commit	4bec4e1
	Installation URL	659ahvsmoaug8

Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

[kean]

Is the editor used for custom posts? Would it work to parameterize the current editor (NewGutenbergViewController) to work with these posts?

[crazytonyli]

Yes, that'd be the next step. The SimpleGBKViewController was created to quickly test editing custom posts in GBK editor.

[kean]

Hey, in addition to the inline comments, I have two more high-level suggestions/questions:

1. Navigation

On the web, the CPTs are added directly to the menu (under "Comments" and everything else). The "Custom Post Types" menu looks out of place. It also shouldn't be shown if you have no CPTs (default for WordPress sites). If the app does the same as the web, it solves both issues.

"Custom Post Type" is a bit of a more technical term. I don't think it should be prevalent in the UI. The user only cares to know that their type of content is available with the names they provided.

2. Post Status & Filtering

It's not clear posts in which status it shows. It's probably a good idea to match the web's UX and show "All" by default, but the list then needs to clearly communicate the status of the posts for each individual post.

The safer option is to keep the same UX as in the current post list. It's convenient that you can switch between tabs in one tab.

[kean]

Let's use locks – just like in WordPressClientFactory – to ensure thread-safety when accessing .service. It's inconvenient to use async methods when they aren't really needed, and it can lead to the same screen being pushed multiple times if you tap it multiple times.

[crazytonyli]

The WordPressClient is an actor, so we can't lock it. There is an open PR that also changes WordPressClient, so I don't want to change it too much at this stage. I'll keep a note to myself to see if it can be improved.

[kean]

You can simply make the property nonisolated without introducing too many other changes.

[crazytonyli]

It's not nonisolated, though. The service uses cache, which is expensive to create (it may create database tables) and should be created only once.

[kean]

Would it work if WordPressClient creates cache and service in its initializer, and store them in as let properties?

public final class WordPressClient: Sendable {
    public let siteURL: URL
    public let api: WordPressAPI

    public let cache: WordPressApiCache?
    public let service: WpSelfHostedService?

    public init(api: WordPressAPI, siteURL: URL) {
        self.api = api
        self.siteURL = siteURL

        self.cache = WordPressApiCache.bootstrap()
        self.service = try api.createSelfHostedService(cache: cache)
    }
}

It will have more predictable performance as it will perform the bootstrap on init as opposed to non-deremined point in time when cache or service are needed/accessed. It'll likely once when the user doesn't need to notice anything – e.g. they are switching sites.

It looks like it's also missing error handling. It would be easier to implement if errors are handled once during init as opposed to individual property accessors.

[crazytonyli]

We don't have any control over which thread WordPressClient.init is called on, but we don't want to create WordPressApiCache on the main thread.

[kean]

If you take the current Core Data implementation, it loads the database on app launch when the app has control over it. It seems like the best place to instantiate expensive subsystems.

Just to clarify, I think it's OK to change later. I just don't think it's acceptable to put it in Task when you try a push a new screen. This seems like the least optimal option.

[crazytonyli]

Yes, I'm planning to look into removing the Task. But I do want to keep creating the cache instance out of the main thread, though. ContextManager, being legacy code and a shared instance, prevents us from keeping it out of the main thread. But since we are starting new here, I just want to try do the right thing.

[kean]

It's not clear what these states mean and why they are exposed to the view layer (CustomPostList): stale, fetching, missing, error, ready? It doesn't seem to make much sense.

From the business logic, you either have a post or you don't. In addition the app might be in the process of sending a request to update one of the posts and it could fail – that's about it.

[crazytonyli]

These states are part of the mechanism by which the Rust library syncs posts and keeps them up to date with the server.

I reused the states in the Swift code for simplicity's sake. I will add a TODO here and address it in a separate PR.

[jkmassel]

From the business logic, you either have a post or you don't.

Just for additional context – the underlying layer works based on post IDs instead of the entire object – this makes differential updates dramatically faster. This also allows for accurate skeleton-loading states – if there are 3 posts, we can represent them loading in the UI accurately (instead of showing a full page of posts then culling it down to 3 when they actually load, for instance).

There's a bunch of benefits to this (faster time-to-interactive with lower server load, for instance) – it's how Android has worked for many years.

[kean]

if there are 3 posts, we can represent them loading in the UI accurately (instead of showing a full page of posts then culling it down to 3 when they actually load, for instance).

It doesn't seem like a substantial benefit, and it probably won't work because you still need to show something while you are fetching the list of IDs. The app only shows it on the first sync, so it's really irrelevant how it looks.

It would be nice to have a minified model of post that has enough information to display a post in a list. There is nothing useful you can really do with an ID, but it means that app has to deal with the incidental complexity it creates.

[kean]

It shows raw content in the place of the excerpt. You can use GutenbergExcerptGenerator to quickly create an excerpt.

[kean]

(nit) It couldn't probably use DataViewPaginatedForEach – doesn't have to, but I think it should work.

[crazytonyli]

I took the same load more mechanism in DataViewPaginatedForEach here. I don't want to just use it here though. Because I don't want to mix the data view loading mechanism and the one in Rust together, also the reused part is pretty simple and straightforward.

[kean]

Let's extract it to a separate file.

[kean]

(nit) It's more idiomatic to implement these as properties (filterMenu). It also doesn't need @ViewBuilder.
(nit) Passing a binding to each individual item is a bit odd: FilterMenuItem(filter: $filter. Can it be implemented as a simple picker?

                Picker("", selection: viewModel.makeSortFieldBinding()) {
                    ForEach([SortField.dateSubscribed, .email, .name], id: \.self) { item in
                        Text(item.localizedTitle).tag(item)
                    }
                }
                .pickerStyle(.inline)

[kean]

I can't say it's clear what' going on in the subscriber. I tried to debug it, but it wasn't really possible with Rust.

When you open the screen it seems to receive two "relevant" updates (what does that mean?).

WpApiCache update: update to listMetadataState at row 1
List info: Optional(WordPressAPIInternal.ListInfo(state: WordPressAPIInternal.ListState.fetchingFirstPage, errorMessage: nil, currentPage: Optional(1), totalPages: Optional(1), totalItems: Optional(1), perPage: 20))
WpApiCache update: update to listMetadataState at row 1
List info: Optional(WordPressAPIInternal.ListInfo(state: WordPressAPIInternal.ListState.idle, errorMessage: nil, currentPage: Optional(1), totalPages: Optional(1), totalItems: Optional(1), perPage: 20))

For each updates, it seems to reload and re-map all the items: collection.loadItems().map(ListItem.init) and update the list with animation. Is that how it's supposed to work? Are we planning to add more granular updates? It seems like it would be inefficient for any large collection of items.

(nit) map(ListItem.init) – this happens on the main thread; possible potential performance improvement.

[crazytonyli]

It's probably easier to look into the Rust library directly to understand its syncing mechanism.

From the client's point of view, we need to do two things:

1. Call refresh() or loadNextPage() to trigger the syncing.
2. Observe the "updates", fetch the most updated data (listInfo() & loadData() results), and display them on screen.

When you open the screen it seems to receive two "relevant" updates (what does that mean?).

Internally, the Rust library stores the syncing data (metadata + post data) in a database. The Rust code sends an update whenever the database is updated. My expectation is that, regardless of the internal mechanism, the client should receive updates only when it's absolutely necessary to refresh the data displayed on screen. That is not the case at the moment. But I think the Rust library should be improved to reduce the number of updates sent to the app.

For each updates, it seems to reload and re-map all the items. [...] Is that how it's supposed to work? Are we planning to add more granular updates?

By "glandular updates", do you mean like only mapping new items and appending them to the list?

[kean]

By "glandular updates", do you mean like only mapping new items and appending them to the list?

Yes, and updated to individual items. This is not strictly necessary with SwiftUI's List diff algorithm, but it might be for Android or for other components or parts of the app.

Core Data has pretty good APIs for observing changes with direct SwiftUI binding for collections (FetchRequest) and for individual items (ObservedObject) where you can dive into any level of granularity if needed. I don't know if there is a similar ORM-ish library for Rust, but I would pick something existing, stable, and documented. This is extremely hard to design and implement well.

collection.isRelevantUpdate(hook: hook)

This whole invocation seems like it should be a one-liner on the view level:

cache.databaseUpdatesPublisher()
            .debounce(for: .milliseconds(50), scheduler: DispatchQueue.main)
            .values
        for await hook in updates {
            guard let collection, collection.isRelevantUpdate(hook: hook) else { continue }

Maybe something like:

for await update in cache.updates(for: collection)

The fact that it needs debounce also points to something problematic. The fact that you need to call collection.loadItems() on every update also is. I seems it leaves a lot to be desired right now – too low-level.

[crazytonyli]

Yes, and updated to individual items. [...] Core Data has pretty good APIs for observing changes with direct SwiftUI binding for collections (FetchRequest) and for individual items (ObservedObject) where you can dive into any level of granularity if needed.

I think there two things in play here:

1. The Rust library sends minimal updates, similar to the NSFetchedResultsController as you mentioned.
2. The Swift observer code only map-and-update the @State var items if absolutely necessary, even if the Rust library does not do the above.

I don't think we intend to make the Rust library do 1. It'd be nice for sure, but, as you mentioned, that can be complicated.

As for 2, I think we can try that in the Swift code. I'm not sure how much performance it'd gain. We can definitely try and see.

The fact that it needs debounce also points to something problematic.

Yes. It is. As I mentioned before, at the moment, the Rust library sends updates that are not necessary to update the UI. It's not too frequently, like it may send two events one immediately after another, even though only one is needed. The frequency of updates affects the list UI, but not the performance. The debounce here is to avoid UI glitches (like list flashes), rather than unblocking the main thread

[jkmassel]

Core Data has pretty good APIs for observing changes with direct SwiftUI binding for collections (FetchRequest) and for individual items (ObservedObject) where you can dive into any level of granularity if needed. I don't know if there is a similar ORM-ish library for Rust, but I would pick something existing, stable, and documented. This is extremely hard to design and implement well.

From a high level we don't want an ORM or really any "magic" here – we update a @State var as needed and the Swift diff algorithm deals with the rest.

That said, we don't want the diff to run more often than it has to – we can probably tighten that up on the Rust side. I know that the collection is separate from the DB observer, but I wonder if the collection could be registered with the observer so we don't need to call collection.isRelevantUpdate at all?

[crazytonyli]

I wonder if the collection could be registered with the observer so we don't need to call collection.isRelevantUpdate at all?

Yep, it could. This would only be a syntactical change: wrapping the WpApiCache update in another helper layer. There is no change to how many updates are sent to the app.

BTW, I created Automattic/wordpress-rs#1128 to document two issues related to sending updates. I don't have a fix for them yet.

[kean]

Needs to be DDLog.

[kean]

(nit) There seem to be a potential to warp it in a Service or a ViewModel to reduce the amount of complexity the view needs to know about.

[crazytonyli]

I have extracted a view model from the previous file. And the code is split into multiple files, which should help with navigating the code.

[crazytonyli]

@kean Thanks for the review. I have created a couple of issues to track your suggestions, and I'll address them in separate PRs.

• https://linear.app/a8c/issue/CMM-1192
• https://linear.app/a8c/issue/CMM-1193

[crazytonyli]

⬆️ Rebased without any code changes.

[wpmobilebot]

🤖 Build Failure Analysis

This build has failures. Claude has analyzed them - check the build annotations for details.

[kean]

The PR seems good enough for merge as a start, but there seems to be quite a few major challenges:

• How to incorporate CPTs into the app UX-wise in a reasonable way
• How to make Rust-based data layer work with million of different parts of the app that use Core Data (AbstractPost) and keeping it maintainable
• A bit questionable API design of the data model for posts (especially statuses) and for data observations exposed by the Rust layer

I'd also try and simplify the following, which could be made more scalable (not require many updates when you add new fields):

public var siteURL: URL {
        switch self {
        case let .dotCom(siteURL, _, _):
            return siteURL
        case let .selfHosted(_, siteURL, _, _, _):
            return siteURL
        }
     }

The enum is not the best way to model it. I'd suggest looking into alternative options, such as protocols:

protocol SiteProtocol {
    var siteURL: URL
}

struct DotComSite: SiteProtocol {
   // Standard + doctom-specific fields
}

struct SelfHostedSite: SiteProtocol {
   / Standard +self-hosted fields
}

The implementation might still have an enum for places that need to deal separately with each type of site, but for most scenarios, you would use either SiteProtocol, polymorphism, or specific types for screens that only work with one particular type of site.

[crazytonyli]

How do we incorporate CPTs into the app UX-wise in a reasonable way

I plan to address this as part of https://linear.app/a8c/issue/CMM-1192

How do we make Rust-based data layer work with million of different parts of the app that use Core Data (AbstractPost) without making it unmaintainable

I'm not sure how to answer this. We definitely don't want to make any code unmaintainable. Just so that we have something specific to discuss, in the context of this PR, is there any code that you think is unmaintainable?

A bit questionable API design of the data model for posts (especially statuses) and for data observations exposed by the Rust layer

I actually think the new syncing mechanism in the Rust layer works pretty well, both in terms of API and how it syncs posts internally.

The statuses API seems a bit noisy. But it has its benefits. For example, the app can handle different statuses differently, like proactively fetching the latest post if it's stale, manually refreshing a single post if it's error, etc. This PR hasn't done any of that, but we may implement that later. I haven't put much thought into handling those statuses, to be honest. That's why I mapped the Rust status to Swift, to keep the option open to using them. Maybe the Rust implementation can reduce that noise a little bit. But I don't think it's a major concern. As for why are there so many statuses, this doc explains it in a high level.

---

Yes, I will address the problem with the site enum in a separate PR. The tuple is getting out of hand.

[jkmassel]

How do we make Rust-based data layer work with million of different parts of the app that use Core Data (AbstractPost) without making it unmaintainable

Core Data and AbstractPost should be considered deprecated.

[kean]

Core Data and AbstractPost should be considered deprecated.

I was looking for something that would help to keep the codebase maintainable short-term and avoid potential regressions when making changes to the existing code. There was an example with the tags screen being reworked in a similar way to support both Core Data and Rust, and it did lead to a couple of regressions. These types of reworks are associated with risk.

[sonarqubecloud]

Quality Gate passed

Issues
42 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud