Skip to content

Deliver the document announcement to a destination that attaches late - #262

Open
olivaresf wants to merge 2 commits into
mainfrom
bridge-handshake-for-late-attaching-destination
Open

olivaresf wants to merge 2 commits into
mainfrom
bridge-handshake-for-late-attaching-destination

Conversation

@olivaresf

Copy link
Copy Markdown
Member

No description provided.

Two of the four tests added here FAIL at this commit. That is deliberate:
they reproduce a live defect, and the fix follows in the next commit so the
reproduction can be watched failing and then passing.

A document announces itself once, at document start, and `Bridge` delivers
that announcement straight to `delegate?.bridgeDidInitialize()`. When no
destination owns the web view at that instant the announcement goes to a nil
optional and nothing records that it happened. A destination attaching
afterwards - which it must, before a user can see the screen - never learns
the page is ready, so it never registers its components, and every component
message the page has queued stays queued for the life of the document. The
screen looks and behaves normally until the user touches something that needs
native, and then nothing happens.

BridgeReadyHandshakeTests drives the real bridge.js user script in a real web
view against a faithful stand-in for @hotwired/hotwire-native-bridge 1.1.0.
BridgeHandshakeLifecycleTests drives a real Session against a real server
through the lifecycle a destination receives when something is presented over
it while its page is still loading - a sheet or a composer, an ordinary thing
to do.

Each suite pairs a control with the failing case, so a green control proves
the harness measures what it claims:

  testDestinationOwnsTheWebViewWhileTheDocumentLoads  passes
  testDestinationAttachesAfterTheDocumentLoaded       FAILS
  testDestinationStaysOnScreenWhileItsPageLoads       passes
  testDestinationIsCoveredWhileItsPageLoads           FAILS

Both failures read the same way: supportedComponents is empty rather than
naming the registered component, one message is stranded in the web bridge's
pending queue rather than none, and no component was ever initialized.

Tests/Turbo/Server.swift gains the fixture the lifecycle suite serves; the
tests do not build without it.
Turns the two failing tests from the previous commit green and leaves the
whole suite passing: 372 tests, 0 failures.

`Bridge` now records that the current document announced itself, and hands
that announcement to a destination that attaches afterwards. The delivery is
idempotent per destination per document: a destination that deactivates and
reactivates on the same document - every tab switch, every return to a screen
- is caught by the identity check and does not register twice. The control
tests assert the registered component list exactly, so a double registration
would fail them.

Nothing changes on the web side, and that is why this is small. bridge.js's
NativeBridge.setAdapter already awaits the registerCalled promise before
installing the adapter, and register resolves it. A register that arrives
late resolves the promise, the adapter installs, and the web bridge flushes
everything it had queued. The web end was always willing to recover; the
native end simply never spoke.

The rejected alternative was to have the attach path ask the page to
re-announce itself by re-dispatching web-bridge:ready. That event is not
private to the handshake - in a Basecamp page every bridge controller hangs
off page readiness - so re-dispatching re-enters page code on every
activation rather than only on the broken path, needs a JavaScript round trip
on a hot path, and depends on the web view cooperating in exactly the state
that broke it. This approach never enters JavaScript and does nothing at all
on the healthy path: the guard falls through on its first condition.

One residual, left deliberately. A different destination taking over the same
document does register again, which is correct because it genuinely needs its
components, but register on the JavaScript side concatenates rather than
merges, so supportedComponents can hold a name twice and the
data-bridge-components attribute shows the duplicate. It is cosmetic -
supportsComponent uses includes - and closing it means changing bridge.js,
which this commit deliberately does not touch.

Bridgable gains one @mainactor requirement; BridgeSpy, the only other
conformer in the tree, gets a no-op.
@olivaresf

Copy link
Copy Markdown
Member Author

Reviewer's receipt. Everything below was measured on this branch, not on the worktree it was developed in.

Watch it fail, then pass

Two commits, in that order, so the reproduction can be checked without trusting a claim. git checkout 375da23 and run the suite; take a8fc3ea and run it again.

                                                    375da23      a8fc3ea
                                                    (tests only) (with fix)
BridgeReadyHandshakeTests
  testDestinationOwnsTheWebViewWhileTheDocumentLoads  passed 0.311  passed 0.304
  testDestinationAttachesAfterTheDocumentLoaded       FAILED 5.676  passed 0.406
BridgeHandshakeLifecycleTests
  testDestinationStaysOnScreenWhileItsPageLoads       passed 0.400  passed 0.401
  testDestinationIsCoveredWhileItsPageLoads           FAILED 12.057 passed 2.701

full suite at a8fc3ea:  Executed 372 tests, 0 failures, 48.3s, ** TEST SUCCEEDED **

Both failures read the same way: supportedComponents empty rather than naming the component, one message stranded in the web bridge's pending queue rather than none, and no component initialized. The multi-second failure times are the five-second poll timing out before the assertions fire.

Each suite pairs the failing case with a control that stays green, so a green control is the evidence that the harness measures what it claims. The controls also assert the registered component list exactly, so a double registration would read "two,two" and fail them — that is the guard on the idempotency below.

The defect

A document announces itself once, at document start, and Bridge hands that announcement straight to delegate?.bridgeDidInitialize(). When no destination owns the web view at that instant it goes to a nil optional and nothing records that it happened. A destination attaching afterwards — which it must, before a user can see the screen — never learns the page is ready, never registers its components, and every component message the page queued stays queued for the life of the document. The screen looks and behaves normally until someone touches something that needs native.

BridgeHandshakeLifecycleTests is the one worth reading: a real Session against a real server, through the lifecycle a destination gets when something is presented over it while its page is still loading. A sheet, a composer. Ordinary.

The seam, and the one I rejected

Chosen: Bridge records that the current document announced itself and delivers that announcement to a destination that attaches later, once per destination per document. Idempotency is an identity check against the last-notified delegate, so a destination that deactivates and reactivates on the same document — every tab switch, every return to a screen — does not register twice.

No JavaScript changes, which is why this is 21 lines. bridge.js's NativeBridge.setAdapter() already awaits the registerCalled promise before installing the adapter, and register resolves it. A register arriving late resolves the promise, the adapter installs, and the web bridge flushes what it queued. The web end was always willing to recover; the native end simply never spoke.

Rejected: have the attach path ask the page to re-announce by re-dispatching web-bridge:ready. That event is not private to the handshake — in a Basecamp page every bridge controller hangs off page readiness — so re-dispatching re-enters page code on every activation rather than only on the broken path, needs a JavaScript round trip on a hot path, and depends on the web view cooperating in exactly the state that broke it. The chosen approach never enters JavaScript and does nothing at all on the healthy path: the guard falls through on its first condition.

Blast radius

  • Two consumers, both pinned exactly. bc3-ios pins exact: 1.3.1 and hey-ios pins exact: "1.3.1" in Shared/HEYCore/Package.swift, both resolved to dbc4fc0. Nothing reaches either app until that app deliberately bumps — no silent propagation. HEY carries the same latent defect and gets the same fix whenever it bumps.
  • One caller in the framework. webViewDidBecomeActive is called only from HotwireWebViewController.swift:52, webViewDidBecomeDeactivated only from :57. No app calls either directly.
  • Bridgable gains one @MainActor requirement. The only other conformer in the tree is BridgeSpy, which gets a no-op.
  • Base. Branched from 1.3.1 (dbc4fc0), which is what both consumers pin, rather than from any intermediate revision.

The residual, stated because nobody asked

A different destination taking over the same document does register again. That is correct — it genuinely needs its components — but register on the JavaScript side concatenates rather than merges, so supportedComponents can hold the same name twice and data-bridge-components shows the duplicate. It is cosmetic: supportsComponent uses includes, so behaviour is unaffected. Closing it means a one-line dedupe in bridge.js, and this branch deliberately touches no JavaScript.

What this does not establish

The reproduction demonstrates a real defect whose signature matches what production is reporting — bridge attached, no components registered, on ordinary non-chat screens. It does not prove this path accounts for every such report in the field. Separating that would take counters on the handshake itself, which is not in this branch.

Environment note

The two suites both run an Embassy test server on a fixed port, so two concurrent xcodebuild test invocations against this package collide with Address already in use and one gets skipped rather than failed. I hit that while measuring and re-ran serially; the numbers above are all from serial runs. Worth knowing before wiring this into a parallel CI matrix.

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant