Skip to content

Unify site provisioning + capability detection into one per-site source - #22944

Merged
nbradbury merged 34 commits into
trunkfrom
jkmassel/editorcapabilitystate-visibility
Sep 2, 2026
Merged

Unify site provisioning + capability detection into one per-site source#22944
nbradbury merged 34 commits into
trunkfrom
jkmassel/editorcapabilitystate-visibility

Conversation

@jkmassel

@jkmassel jkmassel commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

TL;DR

Getting a site ready — mint an application password, recover the REST root, recover XML-RPC, detect editor capabilities, heal a revoked credential — was spread across three consumers that each triggered their own slice and raced the others. One per-site, single-flight pipeline (SiteProvisioningSource) now does it; the connectivity banner, application-password card, and editor preloader render slices of its StateFlow<SiteReadiness>.

PR 1 of #22942.


What this fixes

  • First-login race — auth is awaited and the capability probe is downstream, so the race is impossible rather than mitigated.
  • Stale-model writes (Recovered wpApiRestUrl doesn't survive across launches on Atomic sites #22905) — no stage holds a SiteModel; each reads fresh by siteLocalId and writes only its own column, backed by Make wpApiRestUrl, xmlRpcUrl, and app-password creds single-writer #22947's single-writer exclusions.
  • Removes CredentialsChangedNotifier — the event bus and getSelectedSite() staleness race are gone.
  • Revoked credentials heal in-pipeline — WP.com-connected sites re-mint silently. An unrecoverable one settles Unprovisionable: a 401-triggered run escalates to interactive re-auth, a routine run leaves it to the application-password card. Either way a successful re-mint no longer flashes the re-auth screen.
  • Private site left the user with nothing to read — a private site reported "does not support Application Password authentication" at login (the real reason was computed, then overwritten by a generic message), and on My Site showed no card at all once its credentials were revoked, because the re-auth card needs an authorization URL the Privacy gate won't serve. Both now say the site is private.

Worth a reviewer's eye

  • Unreachable has two producers: the capability probe failed, or the site was unreachable at the auth stage while the device was online. Both show the banner; the second is new (that state previously showed nothing).
  • XML-RPC-disabled card needs a definitive negative (XmlRpcRecovery.Unavailable), not just a missing xmlRpcUrl — otherwise a 429'd site gets a false warning.
  • Heal loop containment: the pipeline's own requests can 401 (validation and the capability probe both use notifier-wired clients), which relaunched it forever. Heals are now bounded by a per-site budget: confirming an unchanged password exhausts it outright, a re-mint costs one, and reaching Ready without needing a heal restores it. A run that already re-minted stands its own deferred heal down — otherwise validation's 401 schedules a redundant re-run that suppresses healing after a successful heal.
  • fluxc: NotifierListener.onRequestedWithInvalidAuthentication now takes SiteModel, not siteUrl — URL isn't unique (constraint is SITE_ID + URL). SiteProvisioningSource is the only implementer.

Out of scope

  • Richer terminal-vs-transient mint semantics and the Allow headless application-password creation on Atomic sites #22884 private-host card.
  • SiteStore.persistXmlRpcUrl is now orphaned — small fluxc cleanup.
  • The preloader no longer recovers the REST root when auth doesn't settle, falling back to a guessed ${site.url}/wp-json/. Only bites a non-standard REST root and failed auth.

Testing

  • ./gradlew :WordPress:testJetpackDebugUnitTest (provisioning source, XML-RPC recoverer, both slices, preloader, login helper + ViewModel, editor settings, reauth notifier) and ./gradlew detekt.

Site unreachable (new state)

  • Connect to a self-hosted site, then ask Claude to simulate it being down
  • Device online → banner appears.
  • With the banner showing, turn the device offline → the banner hides, leaving only the global "no connection" bar. (Losing the network doesn't re-run the pipeline, so the banner has to react to connectivity itself.)
  • Site down and device offline from a cold start → no banner; auth settles Provisioning, not Unreachable.

Application-password card

  • Connect to a self-hosted site, then revoke the app password server-side → re-authentication card.
  • Try to connect to a self-hosted site with XML-RPC disabled (again, ask Claude to simulate) → XML-RPC-disabled card. (Note the XML-RPC step can’t use an Atomic or Jurassic Ninja site, because the Automattic edge returns 429 for xmlrpc.php before any plugin is reached).

Credential revocation

  • WP.com-connected site, password revoked → silently re-minted, no re-auth screen.
  • Self-hosted that can't be re-minted → interactive re-auth appears once.
  • Atomic site with a rejected WP.com bearer token → provisioning settles and stops; check logcat for repeating A_P: Validating application password.

Private-site login message

  • Log in to a private Atomic site → "This site is private, so we can't read its settings to sign you in."
screens-grid

@dangermattic

dangermattic commented Jun 4, 2026

Copy link
Copy Markdown
Collaborator
2 Warnings
⚠️ This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
⚠️ PR is not assigned to a milestone.

Generated by 🚫 Danger

@wpmobilebot

wpmobilebot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

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

App NameWordPress Android
Build TypeDebug
Versionpr22944-3fba8a0
Build Number1498
Application IDorg.wordpress.android.prealpha
Commit3fba8a0
Installation URL1mgm2lgpscc6o
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@wpmobilebot

wpmobilebot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

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

App NameJetpack Android
Build TypeDebug
Versionpr22944-3fba8a0
Build Number1498
Application IDcom.jetpack.android.prealpha
Commit3fba8a0
Installation URL7gtdd0k5dsqcg
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@codecov

codecov Bot commented Jun 4, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.31373% with 56 lines in your changes missing coverage. Please review.
✅ Project coverage is 37.91%. Comparing base (c45e08e) to head (3fba8a0).
⚠️ Report is 1 commits behind head on trunk.

Files with missing lines Patch % Lines
...ess/android/repositories/SiteProvisioningSource.kt 91.70% 1 Missing and 15 partials ⚠️
...i/jetpackrestconnection/JetpackConnectionHelper.kt 0.00% 12 Missing ⚠️
...ationpassword/ApplicationPasswordViewModelSlice.kt 76.31% 4 Missing and 5 partials ⚠️
...i/accounts/login/ApplicationPasswordLoginHelper.kt 69.56% 5 Missing and 2 partials ⚠️
...ndroid/ui/accounts/login/SiteXmlRpcUrlRecoverer.kt 92.68% 0 Missing and 3 partials ⚠️
.../main/java/org/wordpress/android/AppInitializer.kt 0.00% 2 Missing ⚠️
...nnectivity/SiteConnectivityBannerViewModelSlice.kt 89.47% 0 Missing and 2 partials ⚠️
...s/android/repositories/EditorSettingsRepository.kt 0.00% 0 Missing and 1 partial ⚠️
...ccounts/login/ApplicationPasswordReauthNotifier.kt 88.88% 0 Missing and 1 partial ⚠️
...droid/ui/jetpackrestconnection/JetpackInstaller.kt 0.00% 1 Missing ⚠️
... and 2 more
Additional details and impacted files
@@            Coverage Diff             @@
##            trunk   #22944      +/-   ##
==========================================
+ Coverage   37.82%   37.91%   +0.09%     
==========================================
  Files        2350     2352       +2     
  Lines      128587   128795     +208     
  Branches    17844    17898      +54     
==========================================
+ Hits        48641    48836     +195     
+ Misses      75965    75964       -1     
- Partials     3981     3995      +14     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jkmassel
jkmassel force-pushed the jkmassel/editorcapabilitystate-visibility branch from 79c6afa to 50b2b88 Compare June 4, 2026 18:21
@jkmassel
jkmassel changed the base branch from trunk to merge/release-26.8-into-trunk June 4, 2026 18:21
@jkmassel jkmassel changed the title Unify editor capability detection into a per-site detector Unify site provisioning + capability detection into one per-site source Jun 4, 2026
@wpmobilebot

wpmobilebot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

🤖 Build Failure Analysis

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

Base automatically changed from merge/release-26.8-into-trunk to trunk June 4, 2026 20:22
@jkmassel
jkmassel force-pushed the jkmassel/editorcapabilitystate-visibility branch from 2ad8edf to 85497da Compare June 4, 2026 20:31
jkmassel added 12 commits June 13, 2026 09:55
Introduces EditorCapabilityDetector — one app-scoped owner of editor REST
capability state, exposed as a per-site StateFlow — so the connectivity
banner and editor preloader share a single, deduplicated probe instead of
each re-deriving the state and racing the same async preconditions. Folds
in the authenticated direct-host probe fallback so private Atomic sites
detect correctly on trunk. Part of #22942.
#22926's first-login hardening signalled credential establishment through a
process-global CredentialsChangedNotifier that the banner collected against a
re-read of getSelectedSite() — the staleness race called out in #22942.
EditorCapabilityDetector.refresh(storedSite), called from the mint path on the
exact mutated SiteModel, replaces that coordination, so drop the notifier and
its wiring in ApplicationPasswordLoginHelper.
…Source

Promotes the EditorCapabilityDetector into SiteProvisioningSource: one
per-site, single-flight pipeline that ensures application-password
credentials, recovers the REST root, and detects editor capabilities —
each stage awaited before the next. Because the capability probe is now
structurally downstream of credential provisioning, it can never run
before the mint, so the first-login race is gone by construction rather
than mitigated. The application-password card, connectivity banner, and
editor preloader all render slices of the one SiteReadiness state.

The mint/validate mechanics move out of ApplicationPasswordViewModelSlice
(now a renderer) into the source's ensureAuth stage; the per-site
single-flight subsumes the card's old 409-safety guard. The duplicate
wpApiRestUrl heal collapses into the source's recoverRestUrl stage.
Removes the threaded, mutated SiteModel from the provisioning pipeline: each
stage now reads the site fresh by local id and writes back only the column it
changed (persistApiRootUrl / a new persistXmlRpcUrl), so the parallel branches
can't clobber one another and there's no stale-model write (#22905).

With writes targeted, XML-RPC endpoint recovery moves out of the
application-password card into the pipeline as a parallel branch off ensureAuth
-- independent of the REST capability probe, since it needs only the
credentials. The card becomes a pure renderer that reads the recovered
xmlRpcUrl fresh. Adds SiteSqlUtils.updateXmlRpcUrl and a SiteXmlRpcUrlRecoverer
mirroring SiteApiRestUrlRecoverer.
- @Suppress("ReturnCount") on ensureAuth — its five returns are each a distinct
  auth outcome; a single-return rewrite would read worse.
- Drop a stray blank line before a brace left from removing a test region.
The IfNeeded suffix makes the short-circuit (skip when the field is already
present / not applicable) clear at the call site.
WP.com Simple sites are proxy-served and OAuth-authed; the application-password
mint returns NotSupported for them, so the pipeline was returning Unprovisionable
and never reaching capability detection (which works fine through the proxy) -- a
regression vs. the old ungated probe. ensureAuth now short-circuits them to a new
SiteAuthState.NotApplicable (treated like Provisioned), and recoverRestUrlIfNeeded
skips them too.
The route-support probe only sent Atomic sites to the direct host; Jetpack
WPCom-REST sites fell through to the WP.com proxy. Since the proxy and the
direct host advertise different route lists (the #22879 premise), Jetpack sites
got the wrong answer. Broaden the predicate to isUsingWpComRestApi &&
!isWPComSimpleSite (Atomic + Jetpack); the proxy is only for minting the
application password.
On a first provision of an Atomic site, the My Site screen showed a false
"Unable to connect to your site" banner. `ensureAuth` minted an application
password, but `detectCapabilities` re-read the `SiteModel` fresh and a concurrent
whole-row site write (`insertOrUpdateSite` uses `UpdateAllExceptId`) had clobbered
the just-encrypted credential columns before that read (#22905). With no
credentials present, the authenticated direct-host probe was skipped, the
unauthenticated discovery failed against the private host, and the probe reported
the site unreachable.

`ensureAuth` now returns the credentials it obtained in an internal `AuthResult`,
and `detectCapabilities` overlays that immutable value onto its own
coroutine-local `SiteModel` copy. The stages still read fresh and write only their
own column — no `SiteModel` is shared or mutated across the parallel
detect/`recoverXmlRpc` branches, so there's no race.

Verified on-device against a private Atomic site: the authenticated direct-host
probe runs and the banner is gone.
discoverAndVerifyXmlRpcUrl caught only DiscoveryException, so a RuntimeException from the
discovery/verify path would escape the async, cancel the whole provisioning coroutine, and reach
appScope (no SupervisorJob/handler). Catch CancellationException + generic Exception like the sibling
SiteApiRestUrlRecoverer, so any failure degrades to the XML-RPC-disabled card and retries next run.
…riter

#22947 made the app-password columns single-writer — excluded from the generic full-row update,
written only by updateApplicationPasswordCredentials — so a fresh getSiteByLocalId after a mint
reliably carries the credentials and a concurrent site write can't clobber them (#22905). The pipeline
no longer needs to thread them as a value: remove ProvisionedCredentials, AuthResult.credentials, and
the detectCapabilities overlay. ensureAuth returns a bare SiteAuthState; detectCapabilities and
recoverXmlRpcIfNeeded read the credentials off the fresh read.
@jkmassel
jkmassel force-pushed the jkmassel/editorcapabilitystate-visibility branch from 26b93d5 to 3f20dc4 Compare June 13, 2026 16:34
jkmassel added 4 commits June 14, 2026 15:01
SiteApiRestUrlRecoverer and SiteXmlRpcUrlRecoverer already contain their own
throws, but launchPipeline runs the whole pipeline inside appScope.launch with
no guard -- so any other escaping throw (a SQLiteException from a stage's WellSql
write, a failure in detectCapabilities) still reaches appScope. appScope is a
plain Job (no SupervisorJob/handler), so it cancels the scope, takes down every
other app-scoped coroutine, and leaves the readiness flow stuck on Probing.

Wrap the launch body like the recoverers: rethrow CancellationException so
invalidate/clear stay clean, contain anything else as Unreachable so the flow
settles and the next run retries.

Adds a regression test that fails without the guard.
On a persistent 401, WPMainActivity and MediaBrowserActivity navigated straight to interactive
re-auth, even for WP.com-connected sites whose application password can be re-minted headlessly -- and
once a site latched Ready nothing re-validated its credentials, so a server-side revocation went
unnoticed until pull-to-refresh.

Route invalid-auth through the provisioning pipeline. SiteProvisioningSource listens to the raw
wordpress-rs 401 (WpAppNotifierHandler) and re-runs ensureAuth: a WP.com-connected site re-mints
silently; one that can't be settles Unprovisionable. Only that terminal failure escalates to
interactive re-auth -- via a new app-scoped ApplicationPasswordReauthNotifier the two activities now
observe instead of the raw 401. So a successful re-mint no longer flashes the re-auth screen, and
bearer-only WP.com Simple sites no longer mis-trigger an application-password prompt.

Churn guards: skip sites already Unprovisionable and WP.com Simple sites; the no-op-while-active
invalidate keeps the heal's own validate-401 from looping. A 401-triggered run is flagged so only it,
never a routine onResume run, can escalate to re-auth. Also documents why invalidate no-ops during an
in-flight run.
…nPassword

Two review cleanups, no behavior change:

- GutenbergEditorPreloader re-read the provisioned site with
  siteStore.sites.firstOrNull { it.id == siteId } -- a full-table read plus a
  per-row credential decrypt to fetch one row by id. Use getSiteByLocalId(siteId),
  the single-row lookup the rest of SiteProvisioningSource already uses.

- EditorSettingsRepository.isAwaitingApplicationPassword is dead: its only caller
  (the connectivity banner's pending-auth suppression) was removed when detection
  moved behind the pipeline. Delete it.

(SiteStore.persistXmlRpcUrl is also orphaned by this stack but lives in fluxc code
this PR doesn't touch -- left for a fluxc follow-up.)
- launchPipeline now contains the post-runPipeline tail. `maybeRequestReauth`
  reads the DB (`getSiteByLocalId`), and an escaping throw there would cancel the
  non-supervisor `appScope` and wedge provisioning for every site — wrap it so
  only `CancellationException` propagates.
- `detectCapabilities` returns a private `PipelineResult(readiness, latch)`; the
  per-site dedup gate latches only on a live probe, so a `Ready` served from stale
  cache re-probes on the next run instead of sticking for the process lifetime.
- A 401 that arrives while a run is in flight is deferred (`healForInvalidAuth`)
  instead of letting `invalidate` no-op and an unrelated run consume the re-auth
  flag — which could drop a revoked credential's interactive re-auth escalation.
- `WpAppNotifierHandler.NotifierListener` hands listeners the `SiteModel`, not just
  the URL — URL is not unique (the constraint is `SITE_ID+URL`), so the heal now
  targets the exact row that 401'd. Drops the per-401 full site-table load+decrypt.
Three conflicts, all in the app-password area this branch reworked.

MediaBrowserActivity: trunk swapped the CONNECTIVITY_ACTION receiver for
LiveData<ConnectionStatus> while this branch swapped WpAppNotifierHandler for
ApplicationPasswordReauthNotifier. Kept both.

ApplicationPasswordViewModelSliceTest: took ours. Trunk's new tests all target
slice methods this branch moved into SiteProvisioningSource.

ApplicationPasswordViewModelSlice: trunk landed a fix in the block this branch
deletes — the "XML-RPC Disabled" card now only shows on a *definitive* discovery
negative, never on a transient failure (e.g. a 429), so throttled sites don't get
a false warning. Carried that into the pipeline architecture rather than dropping
it: discoverAndVerifyXmlRpcUrl returns XmlRpcRecovery (Recovered / Unavailable /
Inconclusive), SiteProvisioningSource records definitive negatives and exposes
isXmlRpcUnavailable, and the card is gated on that verdict.

Also fixed the branch's SiteXmlRpcUrlRecovererTest stubs for the defaulted
retryOnRateLimit param trunk added to SiteXMLRPCClient.fetchSites — it compiles
either way but leaves Mockito a matcher short at runtime.
…re-auth

Three defects in the new provisioning pipeline, found reviewing #22944.

1. The pipeline's own capability detection can raise the 401 that feeds back
   into healForInvalidAuth. getWpComApiClient installs the same invalid-auth
   notifier as the application-password client, so a rejected WP.com bearer
   token on an Atomic site 401s the theme fetch every run while the app
   password validates fine. Nothing broke the cycle: the run settles Ready or
   Unreachable, so neither the Unprovisionable guard nor the "already
   escalated" check fires, and the deferred heal relaunches forever.

   ensureAuth now reports whether it actually replaced the credentials, and
   PipelineResult carries whether auth was confirmed. A heal that confirmed a
   working password without replacing it cannot be the fix for a 401, so the
   site is recorded in healFutile and its 401s are dropped until an explicit
   retry. A heal that re-minted, or one that never reached the site, is not
   marked futile — so genuine revocation still heals.

2. The validator maps every ambiguous failure (DNS, timeout, refused, 5xx) to
   NetworkUnavailable so it never wipes credentials on a guess, and ensureAuth
   turned that into Provisioning. Since the banner renders only Unreachable and
   the card hides on Provisioning, a site that was simply down showed nothing at
   all — the exact case "Unable to connect to your site" exists for. ensureAuth
   now distinguishes device-offline from site-unreachable via
   SiteAuthState.SiteUnreachable, which maps to SiteReadiness.Unreachable.

3. A routine run never arms reauthOnFailure, so when a 401 arrived mid-run and
   that run settled Unprovisionable, the deferred handler returned "already
   escalated" when nothing had escalated. It now escalates instead of
   relaunching (a relaunch would only re-fail the mint), through a single
   escalateReauth funnel that is idempotent per Unprovisionable episode.

Each fix has a test that fails when the fix is reverted; the loop test exhausts
the test JVM without the guard. Also renames the transient-validation test,
which was passing on a defaulted mock rather than a stated offline precondition.
@nbradbury nbradbury self-assigned this Aug 31, 2026
Follow-up cleanup on the provisioning pipeline; no behaviour change.

SiteAuthState had five variants but only two — Provisioning and Unprovisionable
— were ever wrapped in NeedsAuth and observed by a consumer. The other three
were ensureAuth control-flow signals that happened to live in a public sealed
interface, which forced the application-password card into two dead `when`
arms. ensureAuth now returns a private AuthStage (Proceed / SiteUnreachable /
Stop), so runPipeline is a three-way when with no else and SiteAuthState means
exactly what the card renders.

PipelineResult carried credentialsChanged and authConfirmed, two booleans
encoding a three-state fact with one combination that was unrepresentable in
practice but perfectly constructible. They collapse into a HealEvidence enum
(Inconclusive / ConfirmedUnchanged / Replaced) derived entirely in ensureAuth,
so the futile-heal test is a single equality and runPipeline no longer sets
authConfirmed on the side. This also drops a dead credentialsChanged
pass-through that was always false on the NeedsAuth branch.

No test file needed changing: nothing referenced the three removed variants.
The loop test only passes when healFutile is populated, so it exercises the new
evidence wiring end to end — verified by emitting Replaced on the Valid path,
which reproduces the 401 loop and hangs the suite.

One unreachable semantic shift: a WP.com Simple site used to satisfy the futile
condition and now reports Inconclusive, which is the honest encoding since no
application password applies. Simple sites return early from
onRequestedWithInvalidAuthentication, so they can never be a heal.
The healFutile guard added a third early return to
onRequestedWithInvalidAuthentication, one past detekt's limit of 2. The three
skip conditions collapse cleanly into a conjunction, so extract them as
canHeal() rather than suppressing the rule — the per-condition rationale reads
better as KDoc on the predicate than as inline comments between guards.

Also fixes a stale [maybeRequestReauth] KDoc link left by the rename to
settleHealState.

No behaviour change: canHeal reads the states map before the Simple-site check
instead of after, which is a side-effect-free lookup.
The note was filed under 26.9, which shipped on 1 July. Trunk's top section is
still labelled 27.0 even though versionName is 27.2 and both 27.0 and 27.1 have
released — that top section is the accumulating bucket everything lands in, so
append there rather than adding a new 27.2 heading.

Left under 26.9 the note would never have reached a release.
Logging in to a private WordPress.com site reported "The provided site does not
support Application Password authentication." The site supports them fine — the
Privacy gate sits in front of WordPress and answers the anonymous discovery
request with 403 private_site, so discovery never reaches the REST API.

Two things were wrong.

The reason was thrown away. The ViewModel signalled failure by sending an empty
discoveryURL, and the fragment read that empty string as "unsupported" and
overwrote the specific message it had just been given. Every discovery failure
— DNS, timeout, TLS, 403, malformed URL — reported the same wrong cause. Fixed
structurally rather than by patching the message: discoveryURL now only ever
carries a URL to navigate to, and failure travels through errorMessage alone, so
no future failure path can clobber it either. setError() went with its only
caller.

The reason wasn't named. FailureFetchAndParseApiRoot carries a WpError with the
error code, message and status, so DiscoveryResult.Failed now carries a
FailureReason and the UI can explain a private site rather than guess. Verified
against a live private Atomic site rather than assumed: the library surfaces the
code as WpErrorCode.CustomException("private_site") with status 403.

Non-private failures now surface the library's own message, which is still
better than the previous blanket claim.

Leaves application_password_not_supported_error unreferenced. It is tagged
a8c-src-lib="module:login", so removing it means every values-*/strings.xml plus
whatever the string sync does — worth its own change.
Six review findings, four of them in the heal containment added earlier on this
branch.

The heal could disable itself on success. validate() talks through a
notifier-wired client, so a revoked credential's 401 fires during the very heal
that is fixing it. That scheduled a redundant second run, which re-validated the
fresh password, recorded "confirmed unchanged", and permanently suppressed
healing for the site. The deferred heal now stands down when the run it waited
on already re-minted.

Keying suppression on evidence type also missed the opposite case: a host that
mints happily but never accepts the result produced Replaced every time, which
never suppressed anything and re-minted on every 401 — accumulating application
passwords on the user's account. healFutile is replaced by a spend counter:
confirming an unchanged password exhausts the budget outright, a re-mint costs
one, and reaching Ready without needing a heal restores it.

invalidate() cleared that suppression below its in-flight guard, so it never ran
for the sites that needed it — MySiteViewModel.refresh builds the
application-password card first, and buildCard starts a run synchronously on
Main.immediate. The clears move above the guard; only the relaunch has to
respect an in-flight mint.

escalateReauth read the DB and started an Activity while holding the class
monitor, which the main thread takes on every stateFor / invalidate. The
deferred handler now decides under the lock and acts outside it.

Removing the fragment's blanket error overwrite took with it the only producer
of application_password_not_supported_error — a site that advertises no
application-passwords endpoint then showed raw untranslated exception text.
That case gets its own FailureReason and the string back.

The Jetpack install client lost buildUrl's empty-string guard when it moved off
getWpApiClient, so an empty wpApiRestUrl reached ParsedUrl.parse instead of
falling back.

Tests use a validator stub that fires the notifier the way the real one does;
plain thenReturn stubs hid the feedback loop that caused the first bug.
Three tests asserting each card emits the right SiteNavigationAction when
tapped: the create and re-authentication cards open auto-authentication, the
XML-RPC disabled card opens the bottom sheet. The slice's rendering was covered
already; its click wiring was not.
trunk c45e08e made the WP.com bearer client a no-op notifier, on the same
reasoning the heal containment was written for: application-password
reauthentication can't fix a bearer-token 401.

That removes one of the two self-feeding 401 sources, so the comments no longer
cite it. The containment still applies: getApplicationPasswordClient — the
client validation uses — and the self-hosted capability probe both still report
to the handler, which is what makes a heal schedule a redundant re-run of
itself.
Revoke a private site's application passwords and pull to refresh, and the app
said nothing at all. The pipeline was right — validate, wipe, mint, settle
Unprovisionable — but building the re-authentication card needs an
authorization URL, discovery can't get one through the Privacy gate, and the
Failed branch posted null. The user is left with broken credentials and no
explanation.

The two discovery-failure branches (create card and re-auth banner) were
identical, so they share one handler. It names the one cause this branch can
recognise; everything else still hides pending #22884.

Its own string rather than the login screen's: there the user is signing in,
here they are already signed in and the site can't be reconnected.
The message sat off-centre and wrapped onto two lines because the layout
constrains the text to start after the icon, and the card was passing
ic_notice_white_24dp — a white icon on a white card. Nothing was drawn, but the
space was still reserved.

SingleActionCard gains a centerText flag: the image is hidden and the text spans
the whole card. Reclaiming that space also fits the message on one line. The
other four call sites take the default and are unaffected.

Both branches of each layout decision are written out because view holders are
recycled — a centred card must not leave the next one centred. bind() split into
bindText / bindImage to stay under detekt's LongMethod limit.
The three tests asserting each card emits the right SiteNavigationAction covered
wiring this PR never touches: neither OpenApplicationPasswordAutoAuthentication
nor OpenXmlRpcDisabledBottomSheet appears as a changed line in the slice's diff,
and they replaced nothing — the pre-PR test file had no click assertions at all.

Good tests, wrong PR. This one is already flagged oversized, so they can land
separately against the code they actually cover.

Audited the rest and kept it: every other added test either belongs to a new
class, replaces one removed in the same rewrite, or covers behaviour this PR
changed.
…mpts

Three review findings.

SiteXmlRpcUrlRecoverer ran verifyOrDiscoverXMLRPCEndpoint — a blocking chain of
HTTP calls that can take tens of seconds against a dead host — on BG_THREAD,
which is Dispatchers.Default. That is the same CPU-sized pool APPLICATION_SCOPE
runs every site's pipeline on, so a few slow self-hosted sites could park most
of it. The code this replaced used IO_THREAD; restore that.

escalateReauth marked the once-per-episode flag before knowing whether anyone
took the prompt. Activities register their listener in onResume, so a heal that
settles while the app is backgrounded notified an empty map and lost the prompt
for good: later 401s are blocked by the Unprovisionable state, and routine runs
settle with wasHeal = false. notifyReauthRequired now reports whether a live
listener took it, and an undelivered prompt is held and retried by the next run
— in practice the onResume run, once an activity has registered.

The pipeline's catch-all mapped any throw to Unreachable without the offline
check that ensureAuth and detectCapabilities both make, stacking the site
banner on the global no-connection one. It now makes the same distinction.

The first attempt at the second fix only stopped burning the flag, which left
nothing to retry — the test caught it.
checkstyle's RegexpMultilineCheck runs over Kotlin too, and deleting the last
test in the file left its trailing blank line sitting before the closing brace.
@nbradbury
nbradbury marked this pull request as ready for review September 1, 2026 19:32
@nbradbury
nbradbury requested a review from adalpari September 1, 2026 19:32
@adalpari

adalpari commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Device offline instead → no banner (the global offline banner covers it).

If I'm in the "your site is not reachable" state, and I set the device offline. Is that expected?

Screenshot_20260902-111620

@adalpari

adalpari commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Testing look good. I just left a questions, and here is a point I do believe it is worth to have a look:

DB-write failure masks a successful capability probe as Unreachable
SiteProvisioningSource.kt:441
recoverXmlRpcIfNeeded → persistXmlRpcUrl → siteSqlUtils.updateXmlRpcUrl is not wrapped in try/catch (only the discovery step is).

Failure path: Self-hosted site whose XML-RPC endpoint was Recovered → the single-column DB write throws (e.g. SQLiteException) → the xmlRpc async throws → coroutineScope cancels the already-successful capabilities branch → the outer catch settles the whole site as SiteReadiness.Unreachable. Connectivity banner shows and ready never latches, even though capability detection succeeded.

Fix: Wrap the persist step (or the whole XML-RPC branch’s side effects) so a DB write failure can’t cancel the successful capability branch.

Reported on the PR: with "Unable to connect to your site" showing, turning the
device offline leaves it stacked on the global "no connection" bar.

Losing the network doesn't re-run the pipeline, so the readiness the banner
collects stays Unreachable and nothing re-evaluates. The old code had the same
hole — its suppressForOffline was computed once inside fetchCapabilities — but
this PR added a second route into Unreachable (site unreachable at the auth
stage), so the state is easier to be sitting in when the network drops.

The banner now combines readiness with connectivity through a MediatorLiveData.
ConnectionStatusLiveData only emits on transitions and swallows its initial
value, so it serves as a change trigger and the decision reads
isNetworkAvailable() for the live answer.

Starting offline already behaved correctly — ensureAuth settles Provisioning,
not Unreachable. This is only about losing the network while the banner is up.
Reported on the PR. The two recovery stages end in an unguarded WellSql update.
Only the discovery half of XML-RPC recovery had a try/catch, so a throwing write
— SQLiteException, database locked — escaped the async, cancelled the sibling
through the enclosing coroutineScope, and settled the whole site Unreachable.
The connectivity banner appeared and Ready never latched even though capability
detection had already succeeded.

The outer catch in launchPipeline anticipated a DB throw but contains it too
coarsely: by then the successful result is gone. Both stages are best effort, so
contain them individually and let the capability probe alone decide readiness.

The report named the XML-RPC branch; persistApiRootUrl has the same hole, and
worse placement — it runs before detectCapabilities in the capability branch, so
a throw there kills the probe before it starts. Both are covered.
@nbradbury

Copy link
Copy Markdown
Contributor

If I'm in the "your site is not reachable" state, and I set the device offline. Is that expected?

Nope, that's a regression. Fixed in 0f39bb1.

untitled.webm

@nbradbury

Copy link
Copy Markdown
Contributor

DB-write failure masks a successful capability probe as Unreachable

Good catch - fixed in 3fba8a0.

@adalpari adalpari left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢 it!

@nbradbury
nbradbury merged commit 925f6c4 into trunk Sep 2, 2026
24 of 25 checks passed
@nbradbury
nbradbury deleted the jkmassel/editorcapabilitystate-visibility branch September 2, 2026 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants