Evict WordPress clients during account and site removal - #25852
Conversation
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 33987 | |
| Version | PR #25852 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 80f5646 | |
| Installation URL | 78cf1mhaujnq0 |
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 33987 | |
| Version | PR #25852 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 80f5646 | |
| Installation URL | 2mpd154p4gh4o |
🤖 Build Failure AnalysisThis build has failures. Claude has analyzed them - check the build annotations for details. |
11a7eff to
d0f2aba
Compare
| for (Blog *blog in account.blogs) { | ||
| if ([toDelete containsObject:blog.dotComID]) { | ||
| [self unscheduleBloggingRemindersFor:blog]; | ||
| [self evictWordPressClientForBlog:blog]; |
There was a problem hiding this comment.
This clears the site's saved connection a moment too soon.
The sync deletes the site in the background, but that delete isn't written to disk until the whole sync finishes. In that gap the site is still on screen. If you tap it right then, the app builds a fresh connection for a site that's about to disappear — undoing the cleanup this line just did. The new connection then sits unused until the next logout.
Steps to see it:
- Sign in to WordPress.com with two sites, A and B.
- On the web, remove your access to site B.
- In the app, pull to refresh. It starts removing B in the background.
- Before it finishes, tap B — it's still showing in your list.
- The app spins up a brand-new connection for B and starts loading a site that's being deleted.
Fix: do this cleanup after the sync has finished saving the deletion, not before. Once B is really gone, tapping it can't rebuild the connection. (This is a bit more than a one-line change — the cleanup needs to move to where the sync reports it's done — so I've left it as a note rather than a code suggestion.)
There was a problem hiding this comment.
Based on the steps, the race between step 3 and 4 specifically, it looks like an acceptable edge case? I think the app in general would struggle with this kind of simultaneously deleting from the web and accessing from the app issue. What do you think?
There was a problem hiding this comment.
Could we speculatively remove the site from the UI if it's pending deletion? I think that would be a workaround.
The repro steps are just a way to demonstrate the issue without actually deleting the site.
There was a problem hiding this comment.
I don't see a "pending deletion" state on WP.com. I tried deleting a couple of simple sites and a premium sites. They are all gone pretty quickly. I only have one atomic test site, so I did not try that...
I believe once you delete the site from the app, the site's response says is_deleted is true, which means the site won't show up in the site list.
There was a problem hiding this comment.
Sorry, I mean "pending deletion" locally – the network request is in-flight to delete it, so the site is hidden from view immediately. It makes it so the user can't interact with it.
We could deal with this as a future consideration if needed?
d0f2aba to
a28cfa7
Compare
jkmassel
left a comment
jkmassel
left a comment
There was a problem hiding this comment.
We shouldn't be able to interact with the app while a site is being deleted, so this is good to merge.
3 participants
WordPressClientFactorycaches clients by blog ID. Removing an account or site currently leaves its client cached for the rest of the app session.This PR evicts clients when removing the default WordPress.com account, explicitly removing a site, or deleting a site that is no longer returned during account sync.