Skip to content

Ambiguous request failures should degrade optional dependencies rather than fail the editor load #603

Description

@dcalhoun

Description

A transient server failure on an optional dependency currently fails the entire editor load, even though the editor has a perfectly good degraded mode for it.

On iOS this is partly addressed: networkFallbackMode == .automatic degrades to empty dependencies, but only for a fixed set of URLError codes (EditorService.swift#L211-L221). A 5xx or 429 never qualifies. On Android there is no equivalent concept at all — every failure is fatal.

Reproduction (Android, cold cache)

  1. Disable airplane mode
  2. Load the site's Editor Configuration (discovery succeeds, theme styles = ON)
  3. Tap Clear Preload Cache
  4. Enable airplane mode
  5. Open the editor

Result: Failed to load editor; unable to resolve host…

The same steps on iOS produce an error with fallback .disabled, and default styles with .automatic. Android has no way to get the second outcome.

For contrast, these already work on Android and should keep working:

  • Discovery fails first (offline from the start) → capability negotiated off → default styles
  • Warm cache + offline → cached theme styles applied

Proposed change

Replace isNetworkError with a predicate asking "is this failure non-authoritative?" rather than "did we fail to reach the server." The current name becomes inaccurate once 5xx and malformed responses qualify — neither is a network error.

Non-authoritative: URLError/IO failures (current set), 5xx, 429, malformed or undecodable body.
Authoritative, therefore excluded: 404 rest_no_route (handled in #602), 401/403.

Apply only to optional dependencies — editor settings and plugin assets, which have both a capability flag and a defined absent-value:

Mode Outcome
Disabled Fatal, as today
Automatic Cached value if present, else absent-value

For required dependencies (post data, post types, active theme) there is no absent-value, so automatic fallback may only substitute a cache hit — never proceed without them. Automatic means "use what you have," not "proceed regardless."

The cache is already consulted before any request (iOS EditorService.swift#L224, Android EditorService.kt#L260-L269), so this only affects the cache-miss path.

Tasks

  • iOS: widen the predicate beyond URLError; rename to reflect what it asks
  • Android: introduce NetworkFallbackMode and the equivalent branch in prepare()
  • Both: restrict degradation to optional dependencies
  • Decide the Android default (see below)

Open question: Android's default

Defaulting Android to disabled preserves today's behavior exactly — the offline cases that work now never reach the fallback check, so there's no regression risk either way. It's a straightforward design choice, not a compatibility constraint.

Worth noting the iOS enum documents .disabled as "current default behavior," which reads more like inherited history than a deliberate decision. Choosing Android's default is a good moment to revisit iOS's too.

Also worth noting: the iOS demo can afford optimistic capability flags offline because .automatic exists; Android's demo instead fails closed to false in SiteCapabilitiesDiscovery. Two workarounds for the same missing capability.

Auth handling

401/403 should stay fatal under both modes. It's authoritative but not a capability answer — typically an expired or broken token, where silently degrading hides a fixable problem behind a subtly-wrong editor.

To make that actionable, the host needs more than an opaque error through editor(_:didFailToLoad:) (EditorViewControllerDelegate.swift#L20). WP-iOS currently downcasts to ClientError and string-matches wpError.code to classify errors for telemetry (PostGBKEditorViewController.swift#L133) — evidence the need is real, not hypothetical. Suggested:

public enum EditorLoadFailure: Error {
    /// Credentials rejected. The host should re-authenticate and retry.
    case unauthorized(underlying: Error)
    /// A required resource was unavailable or the server failed.
    case unavailable(underlying: Error)
}

The protocol's empty default implementation (#L81) also means an unopted host gets nothing. Android has no load-failure callback at all — worth adding for parity.

Out of scope

Whether GBK should hand auth classification to the host and let it refresh-and-retry rather than deciding fatal itself. WP-iOS has a token-refresh path GBK knows nothing about, but that needs retry hooks and re-entrancy handling during prepare(). Shipping the typed failure first keeps that open without a later signature change.

Related

Depends on #602 landing first — it removes 404 from this path by construction.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions