ReviewOS

also looking at this

craft-native/craft

fix(ios): fall back to the bundle only when the remote is out of reach, and come back

#253
Merged glennmichael123 wants to merge fix/ios-fallback-only-for-connectivity into main
4 files +206 -1
9 days ago

Closes #252.

What was wrong

func webView(_ webView: WKWebView, didFailProvisionalNavigation navigation: WKNavigation!, withError error: Error) {
    loadBundledFallback(in: webView)
}

With a remote devServerURL, both failure callbacks sent the app to craft://app/index.html without looking at the error, and loadedBundledFallback only ever went one way. Two consequences:

  • A cancelled load was treated as the network being gone. WebKit reports NSURLErrorCancelled (-999) whenever a new navigation starts before the previous one commits: two quick taps, a redirect, a location.assign while a load is in flight. Each one ended the session on the bundled copy. This is what happened in WildLoop after quick tab taps on a slow dev server.
  • There was no way back. The bundle is served from craft://app, so a frontend calling its API by relative path had no API until the app was killed and relaunched, and showed whatever web build was last copied in.

What it does now

Fall back only when the remote is out of reach

CraftLoadFailure.isUnreachable decides. It answers yes only for connectivity: not connected, cannot find the host, cannot connect to it, timed out, connection lost, DNS lookup failed, data not allowed, roaming off.

Everything else stays on the remote:

  • -999 and WebKit's frame-load-interrupted (WebKitErrorDomain 102): a load a newer one replaced did not fail.
  • TLS and bad-response errors too, which the issue did not ask about. In those the server was reached. Swapping in a local copy would hide a real fault under a page that half-works, which is worse than showing the error.

It is a whitelist on purpose. An error code nobody thought about keeps the app on the remote origin rather than quietly switching it to a stale bundle.

And come back

Two triggers, both calling returnFromBundledFallback:

  1. The network comes back. This hooks into the NWPathMonitor the Coordinator already runs for craftNetworkChange, rather than starting a second one.
  2. The app returns to the foreground, via willEnterForegroundNotification.

The network trigger fires on a false → true transition only, and this is the part worth reviewing. The tempting version, retrying whenever the path is satisfied, loops. A server that is down on a network that is up leaves the path satisfied the whole time, so every path update would retry, fail with cannotConnectToHost, fall back, and retry again. With the transition guard, that case sits on the bundle until the next foreground, which is a person deciding to try again.

isConnected already defaulted to true, so the monitor's first update at launch is true → true when online and true → false when offline. Neither triggers a spurious retry.

If the remote is still out of reach when a trigger fires, the load fails with a connectivity error and falls back again: one round trip per trigger.

Tests, including one that runs the Swift

scripts/load-failure.ts lifts CraftLoadFailure out of the template, compiles it with swiftc on the host, and runs it against real error codes. It is part of test:templates, so it runs on the macOS CI job, not only locally. The classifier is written against NSError domain and code rather than WebKit's types so it compiles with Foundation alone. Output:

ok: NSURLErrorCancelled (-999) -> stays on the remote
ok: WebKitErrorDomain 102, frame load interrupted -> stays on the remote
ok: NSURLErrorSecureConnectionFailed -> stays on the remote
ok: NSURLErrorServerCertificateUntrusted -> stays on the remote
ok: NSURLErrorBadServerResponse -> stays on the remote
ok: NSURLErrorNotConnectedToInternet -> falls back to the bundle
ok: NSURLErrorCannotFindHost -> falls back to the bundle
ok: NSURLErrorCannotConnectToHost -> falls back to the bundle
ok: NSURLErrorTimedOut -> falls back to the bundle
ok: NSURLErrorNetworkConnectionLost -> falls back to the bundle
ok: NSURLErrorDNSLookupFailed -> falls back to the bundle

It is also runnable on its own: bun packages/ios/scripts/load-failure.ts.

index.test.ts pins the wiring. Both callbacks route through the classifier and neither reaches the fallback directly, both recovery triggers exist, and the transition guard is in place.

mutationcaught by
-999 treated as unreachable (the #252 bug)the Swift run: FAIL: NSURLErrorCancelled (-999): isUnreachable = true
any non-URL error falls backthe Swift run, naming WebKit 102
every URL error falls back (the old behaviour)the Swift run
retry on every satisfied path (the loop)the wiring test

Not covered

  • No automated repro of the navigation itself. The issue said the same. The E2E probe loads its bundle, not a remote devServerURL, so it never takes this path. The classifier and the wiring are tested; the WebKit timing is not.
  • One narrow race is left. If connectivity returns in the moment between a load failing and the fallback being loaded, the false → true transition has already passed when the app lands on the bundle. It then waits for the next foreground. The window is small, and closing it would mean retrying on a satisfied path, which is the loop above.
  • Android has the same one-way latch: onReceivedError in MainActivity falls back on any main-frame error and never comes back. #252 is iOS-only, so it is not fixed here. It is worth its own issue.

Verification

test:templates' new step locally, packages/ios/scripts/typecheck-template.sh, bun run typecheck, bun run verify:packages, Pickier (0 errors), and the iOS package tests (52).

4 changed files on the files tab, with 0 review threads.

Sign in to comment on this pull request.