ReviewOS

also looking at this

craft-native/craft

fix(android): tie a ready mark to its own navigation, and inject a document once

#239
Merged glennmichael123 wants to merge fix/android-injection-generation into main
5 files +159 -3
9 days ago

Closes #228.

Two gaps in how a page is marked ready, both found while fixing #215 and both worse now that deep links queue until the bridge is.

A stale completion marked the wrong page ready

evaluateJavascript's completion is posted, not immediate. If it ran after the next navigation's onPageStarted, it set bridgeReady = true for a document that had never been injected — and the queues flushed straight into it:

val links = pendingDeepLinks.toList()
pendingDeepLinks.clear()
links.forEach { (url, initial) -> dispatchDeepLink(url, initial) }

A page with no bridge and no replay listener, and the queue cleared. The link is gone.

markBridgeLoading() now bumps a navigationGeneration, the injection reads it just before handing the script over, and the completion compares before doing anything. A completion for a page that has since navigated away returns early: the new document's own injection will mark it ready.

A same-document navigation injected the bridge twice

Chromium calls onPageFinished without a matching onPageStarted for same-document navigations — a fragment change, a history.pushState — and MainActivity injects on every trusted onPageFinished. Running the whole script again replaces window.craft, fires craftReady a second time, and rejects every in-flight promise with "Android bridge reinitialized".

The script is now wrapped so a second run in the same document does nothing:

private fun onlyOnce(script: String): String =
    "if (!window.__craftBridgeInstalled) {\nwindow.__craftBridgeInstalled = true;\n$script\n}"

A block rather than a function, deliberately: var and function declarations are function-scoped, so the script behaves exactly as it does unwrapped, and everything it publishes it publishes on window regardless. A real navigation gets a new window, so the flag is absent and the bridge installs normally — which is the half worth being careful about, since a guard that got this wrong would leave every page without a bridge at all.

I chose this over the alternative of probing the page first (evaluateJavascript("!!window.craft") and injecting in its callback), which puts a round trip on the hot path before every injection, and over making each installer individually idempotent, which is the same guard written a dozen times.

The verification the issue asked for

The issue's first step was "verify on an emulator with a pushState navigation", and it noted this was not yet reproduced. That is now an Android E2E case rather than a manual check: the page pushes state, waits 2 s for onPageFinished to arrive if it is going to, and requires that it comes out with the same window.craft, the same promise runtime, and no second craftReady — then that the bridge still answers, so the case cannot pass by the guard having suppressed the bridge entirely.

Holding the promise runtime's identity rather than an in-flight call is what makes it non-flaky: nothing the bridge offers stays pending for two seconds without UI, but a second injection replaces window.__craftPromise on its way to rejecting everything in it, which is the same evidence.

So the case passes whether or not this emulator re-enters onPageFinished, and it fails if the guard regresses.

Tests that fail without the fix

The Bun page-script test now runs the script as Kotlin evaluates it, wrapper included, by slicing onlyOnce out of the template alongside the script itself. Neutering the guard fails two of its tests: the bridge is replaced on a second injection, and craftReady fires again.

Three cases cover it: a second injection leaves window.craft and the ready count alone, an in-flight promise still settles across one, and a first injection into a fresh document installs as before.

index.test.ts pins the generation: that it exists, that markBridgeLoading bumps it, that it is read before the script is handed over and compared after — either order alone leaves the race open.

Verification

bun run test, bun run typecheck, bun run verify:packages, Pickier (0 errors), the android/ios package tests (149), and 55 protocol tests. The Kotlin compile and the emulator run only in CI.

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

Sign in to comment on this pull request.