Repository navigation
Show Reader profiles for post mentions - #25793
crazytonyli wants to merge 14 commits into
Conversation
Generated by 🚫 Danger |
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 35033 | |
| Version | PR #25793 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | fbb25b2 | |
| Installation URL | 2msi3oc5939do |
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 35033 | |
| Version | PR #25793 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | fbb25b2 | |
| Installation URL | 6rq9n1003vmog |
|
Version |
14b2ee0 to
26cb5f1
Compare
|
Version |
| // Posts typically have few mentions, so resolving them sequentially keeps this simple without meaningful delay. | ||
| for mention in mentions { | ||
| let key = Self.normalizedMentionHandle(mention.handle) | ||
| guard mentionProfiles[key] == nil, requestedHandles.insert(key).inserted else { | ||
| continue | ||
| } | ||
|
|
||
| if let profile = await readerUserProfileService.fetchProfile(handle: mention.handle) { | ||
| mentionProfiles[key] = profile | ||
| } |
There was a problem hiding this comment.
Pre-fetch in bulk, with the post. This makes one request per handle and awaits each before starting the next. Until a handle's request has finished — and after any failed one — tapping that mention opens the web view, so the same link behaves differently depending on when it's tapped.
We want zero delay on a mention tap, which means the profiles should load along with the post instead of trickling in after render. Numbers from testing against the live API on desktop broadband:
- Each
GET /users/<handle>takes about 150 ms on a warm connection (257 ms for the first). - The worst real post we sampled has 18 distinct handles, so about 2.7 s before the last one resolves.
GET rest/v1.1/batch?urls[]=/users/a&urls[]=/users/bresolves them in one round trip: 232 ms for 10 handles, 301 ms for 30. The response is keyed by the requested path, and an unknown user comes back as{"status_code": 404, …}in its own slot without failing the rest, somapProfilecan run per entry.
HTTPRequestBuilder already encodes an array parameter as urls[]=…, so perform(.get, URLString: "rest/v1.1/batch", parameters: ["urls": […]]) should be all it takes — we have not compiled or run that in the app. The handle allow-list encoding and the user_login comparison are worth keeping, since each sub-URL is interpreted server-side.
IMHO we could probably store the mentions alongside the post content – anytime post content changes, we can reload the mentions data if needed? I think that'd mean we store:
post_content -> derived_mention_names -> mention_data. If post_content changes but derived_mention_names doesn't, no need to reload anything?
There was a problem hiding this comment.
As I mentioned in the PR description, I deliberately chose to resolve the mention links separately. I don't want to mix too much logic into the main use case of fetching and displaying post content. The "tapping mention links to show a native UI" is like an add-on, which should be available in most cases, because, as you mentioned, it should be pretty quick to resolve them.
| Task { @MainActor [weak coordinator] in | ||
| await coordinator?.resolveMentionProfiles(for: post) | ||
| } |
There was a problem hiding this comment.
If any of this stays as a Task started from the detail screen, it should be stored and cancelled in deinit like translationAvailabilityTask, with a cap on the number of handles — the post author controls how many there are.
As written, the loop keeps running after the screen is dismissed. The in-flight call holds the coordinator, so the postInUse(false) in its deinit waits for the loop to finish.
| private final class ReaderUserProfileServiceMock: ReaderUserProfileService { | ||
| private(set) var requestedHandles: [String] = [] | ||
|
|
||
| func fetchProfile(handle: String) async -> ReaderUserProfile? { | ||
| requestedHandles.append(handle) | ||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
Should this always return nil?
| private static func parseMention(from anchor: Element) -> Mention? { | ||
| guard let handle = try? anchor.attr("data-username").trimmingCharacters(in: .whitespacesAndNewlines), | ||
| !handle.isEmpty, | ||
| let href = try? anchor.attr("href"), | ||
| !href.isEmpty, | ||
| let url = URL(string: href), | ||
| let scheme = url.scheme?.lowercased(), | ||
| scheme == "http" || scheme == "https" | ||
| else { | ||
| return nil | ||
| } | ||
|
|
||
| return Mention(handle: handle, url: url) | ||
| } |
There was a problem hiding this comment.
data-username isn't tied to the href or the visible text, so an anchor can display @a, link to /mentions/a/, and open the profile sheet for b. Requiring the href path to be /mentions/<data-username>/ closes that, and it costs nothing on real content — all 105 anchors we sampled already match.
| if let profile = mentionProfiles[Self.normalizedMentionHandle(mention.handle)] { | ||
| presentMentionProfile(profile) |
There was a problem hiding this comment.
A resolved mention tap no longer records readerArticleLinkTapped — it did when every mention went through the web view — and the sheet records nothing of its own. Is that intended? The existing author and comment-avatar sheets don't track either, but it'd probably make sense for all of them to?
| } else { | ||
| WPAnalytics.trackReader(.readerArticleLinkTapped) | ||
| presentWebViewController(url) | ||
| } |
There was a problem hiding this comment.
Nit: this repeats the final else. Folding the profile lookup into the condition drops the duplicate, and an unresolved mention falls through the normal chain to the same place:
} else if let mention = findMention(matching: url),
let profile = mentionProfiles[Self.normalizedMentionHandle(mention.handle)]
{
presentMentionProfile(profile)| let result = await wordPressComRestApi.perform( | ||
| .get, | ||
| URLString: "rest/v1.1/users/\(encodedHandle)", | ||
| parameters: ["meta": "site"] |
There was a problem hiding this comment.
Nit: meta=site has no effect on this endpoint — the response is byte-identical (746 bytes) with and without it, and primary_blog is included either way.
| private extension String { | ||
| var nilIfEmpty: String? { | ||
| isEmpty ? nil : self | ||
| } | ||
| } |
There was a problem hiding this comment.
Nit: String.nonEmptyString() in WordPressShared already does this.
| self.readerUserProfileService = | ||
| readerUserProfileService | ||
| ?? WordPressComReaderUserProfileService(coreDataStack: coreDataStack) |
There was a problem hiding this comment.
Nit: this builds the service — a synchronous main-context account fetch — in every coordinator init, including for feeds and posts that can't have mentions. Resolving the API when it's first needed avoids that. It may be moot depending on where the pre-fetch ends up.
|
Version |
The markup does not tie data-username to the href, so an anchor could display one user and name another. Only accept anchors whose href path is the mention page of the named user, which keeps the native profile sheet consistent with the page the web view fallback opens.
A mention is still an article link when it opens the native profile sheet instead of the web view.
An unresolved mention matches no gallery, image, story, or Reader route, so it reaches the final branch, which already tracks the tap and opens the web view.
The users endpoint ignores meta and always includes primary_blog.
* Size the Reader profile sheet to its content The sheet used a fixed `.medium()` detent, so a profile with an avatar, a name, and one row left the lower part of a half-screen sheet empty. Replace the `List` with a `ScrollView` so the content has a measurable height, and size the sheet to that height plus the navigation bar, up to half the screen. A taller profile scrolls, and the sheet can be pulled up to show all of it. The height is measured inside the detent resolver, at the width the sheet gives the profile. On iPad the sheet is narrower than the view that presents it, and its width changes with rotation and window resizing, so a height measured once before presenting cut off the last line of a profile or left empty space below it. The profile reports when its height changes, so the detents are resolved again. The navigation bar only reaches its final height once it is in the sheet, and iOS 17 does not resolve the detent again when it does, so the hosting controller also invalidates the detents when its top safe area inset changes. `showAuthorProfile()` built the same sheet inline. It now uses `ReaderUserProfilePresenter`, so the post author, comment author, and mention sheets are all sized the same way. The "Site" row is left out when there is no site, instead of showing a dash. Its title is a heading for VoiceOver, and is uppercased for the text's locale. Outside a `List`, a link centers a label that wraps, so the site address is explicitly leading-aligned. A name that wraps is now centered under the avatar. * Show the bio in the Reader profile sheet `GET /users/<login>` already returns the user's bio as `description`. Map it, and show it under an "About" heading below the site. WordPress.com returns `&`, `<` and `>` in the bio and in the display name as entities, so both are decoded. Only mentions have a bio. Comment and post authors come from data that does not include one, so their sheets are unchanged. * Show a decoded name and only web addresses in the Reader profile sheet The sheet showed a comment author's raw name, so an author the comment lists as "Tom & Jerry" read "Tom & Jerry" in the sheet. It now uses `authorForDisplay()`, as the comment cell does. A comment's address is whatever its author entered, and the sheet offered any of them as a link: `tel:+19005550100` was a tappable "Site", and a bare `http://` a link that opens nothing. Only an `http` or `https` address with a host is offered now, whichever of the three sources it comes from. Both problems predate this branch.
fbb25b2 to
f6d731a
Compare
| } | ||
|
|
||
| func render(_ post: ReaderPost) { | ||
| Task { @MainActor [weak coordinator] in |
There was a problem hiding this comment.
The lookups keep running after someone leaves the post. This stores the task and cancels it, the same way as translationAvailabilityTask.
| Task { @MainActor [weak coordinator] in | |
| mentionProfilesTask?.cancel() | |
| mentionProfilesTask = Task { @MainActor [weak coordinator] in |
Two lines outside this diff need adding by hand:
private var translationAvailabilityTask: Task<Void, Never>?
+ private var mentionProfilesTask: Task<Void, Never>? translationAvailabilityTask?.cancel()
+ mentionProfilesTask?.cancel()
NotificationCenter.default.removeObserver(self)This needs the check in resolveMentionProfiles from the other suggestion too.
How this was checked: Jetpack on an iOS 27.0 simulator, with a fixture post of 6 mentions and the first lookup delayed through a local proxy. The post was closed while that lookup was running.
| This branch | With this suggestion | |
|---|---|---|
| Lookups sent after closing the post | 5 | 0 |
| Coordinator released | 28 s after closing | Right away |
Not covered: this does not stop the crash when someone signs out during the lookups, tracked in #26305. That crash still happened with this suggestion applied (Task created in a session that has been invalidated), with the post still open at sign-out.
| }) | ||
| var requestedHandles = Set<String>() | ||
| // Posts typically have few mentions, so resolving them sequentially keeps this simple without meaningful delay. | ||
| for mention in mentions { |
There was a problem hiding this comment.
Part of the suggestion on render above: without this check, cancelling stops only the lookup in progress, and the loop moves on to the next one.
| for mention in mentions { | |
| for mention in mentions { | |
| guard !Task.isCancelled else { | |
| return | |
| } | |
Cancel pending lookups when the post closes so remaining requests stop and the coordinator can be released promptly.
* Look up the WordPress.com API for each mention profile request Signing out invalidates the account's API, and a request on an invalidated API crashes the app. Looking it up per request returns nil once the account is gone, so the remaining lookups are skipped. * Limit mention profile lookups to 10 per post A post's author decides how many people it mentions, so a post could otherwise trigger an unbounded run of requests. Also covers cancellation and showing a resolved profile, and adds the release note. * Explain what the mention lookup limit protects against * Revert looking up the WordPress.com API for each mention profile request The sign-out crash belongs in HTTPClient, where the fix covers every API client (#26305).
…ion-in-post-alpine
…ion-in-post-alpine
Fixes https://linear.app/a8c/issue/CMM-2158.
Tapping at-mentions now shows the
RreaderUserProfileViewsheet, which is the same as tapping comment author avatars.For the sake of keeping the implementation simple, the solution is resolving the
<a>@username</a>links to a user profile object when viewing the post. The expectations are 1) there will not be many mentions in regular posts and 2) it'd be pretty quick to resolve them, at least they should be resolved before user reading the post and tapping a mention link.