MOBILE-341: Report what the embedded block shows and count shows where somebody is looking - #757
Merged
Merged
Conversation
added 3 commits
August 25, 2026 17:38
The mirror of the same change on iOS. A wrapper that lays the block out itself was told only whether the view is visible, and a boolean cannot tell an error screen from a shimmer: both keep the place occupied, and only the container knows which of them it settled on. A Flutter widget draws the host's screens itself while the native view holds nothing but a stand-in, so it has to know which one to draw. `MindboxEmbeddedBlockAppearance` names what is drawn — placeholder, content, error, collapsed — and `setAppearanceObserver` hands the current value out on subscribing, which the boolean observer never did: a wrapper attaching after the outcome would otherwise hold the place of a block that had already collapsed, with nothing left to say so a second time. The rules behind the choice stay here: `appearanceFor(state)` is the single place that decides, `hasCollapsed` gives way to `hasSettled` plus `shownAppearance`, and the view's own `GONE` is derived from the same answer, so it cannot drift from `COLLAPSED`. Two rules are pinned down along the way. An empty place collapses however the host draws its failures — a host cannot fill the space of a block that was never meant to be there. And the error view is obeyed when it changes: swapped mid-failure it replaces what is on screen, taken away it collapses the block. What neither does is expand a block that has already collapsed; reopening space the layout has reclaimed would make it jump. `setHostVisible(Boolean)` is a second source for the same input as window visibility: a Flutter or React Native screen never leaves the single window the app has, so without it a block spends its waiting budget on a screen nobody is looking at. The window, the host and `release()` now drive one idempotent switch — three sources that can each repeat what another already said — and a release stops the content before letting it go.
…ed off screen The backend hears about blocks the user was actually shown. A page rendering behind another screen was counted anyway: the block reported the show the moment the page said `contentRendered`, whether or not anybody could see it. The show is now counted where the block is looked at — at the render if it is on screen, otherwise by the `start()` that brings it back. `timeToDisplay` follows the same split. Measured at the show, it would have carried the user's absence from the screen on top of the wait for the page, so the render time is frozen where the render was reported. Failures are held the same way, and for the same reason: one that happened off screen is kept and sent when the block comes back. Only the first is kept — the block reports the outcome it came back to, and a silent page repeating itself adds nothing to that.
The budget only ever came from `app:mindboxTimeoutMs`, and a wrapper has no XML to put it in: the Flutter and Compose hosts both build the block programmatically and were stuck with the default 30 s, while the iOS container has taken a `timeout:` since it was written. The programmatic constructor takes one now, and the internal one prefers it over the attribute — a block built in code has no attributes to read, and one inflated from XML has no caller to ask, so neither path loses anything. Nothing new decides what a bad value means: a non-positive budget still falls back to the default and says so in the log, in the one place that already did that.
There was a problem hiding this comment.
Pull request overview
Adds wrapper-facing appearance and visibility hooks, improves show/failure accounting, and supports programmatic timeouts.
Changes:
- Reports precise block appearance and host visibility.
- Defers off-screen show/failure reporting.
- Adds programmatic View and Compose timeout configuration.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
EmbeddedBlockWebViewHolderTest.kt |
Tests deferred reporting. |
MindboxEmbeddedBlockViewWrapperHooksTest.kt |
Tests wrapper hooks and appearance. |
MindboxEmbeddedBlockViewLookupTest.kt |
Tests programmatic timeouts. |
MindboxEmbeddedBlockViewCollapseTest.kt |
Updates constructor usage. |
EmbeddedBlockWebViewHolder.kt |
Defers off-screen telemetry. |
MindboxEmbeddedBlockView.kt |
Adds appearance, visibility, and timeout APIs. |
MindboxEmbeddedBlockListener.kt |
Clarifies failure behavior. |
MindboxEmbeddedBlockAppearance.kt |
Defines appearance states. |
MindboxEmbeddedBlock.kt |
Integrates appearance and timeout with Compose. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
sergeysozinov
approved these changes
Aug 26, 2026
added 8 commits
August 26, 2026 18:12
Pages already shipped ask `checkInappsTargeting`; the name became `filterShowableInapps` in the rename, and iOS answers both. Android answered only the new one, so on a shipped page the block stayed silent: the page waited out its three seconds, drew an empty feed, reported contentRendered: 0 and collapsed for no reason of its own. A second enum constant rather than a Gson `alternate`: `alternate` is read-only, so the answer would go back named `filterShowableInapps` while the page waits for one to the name it asked under — and an answer it does not recognise is the same silence as none. The test that asserted the old name was cut now proves both spellings get the identical answer, mirroring the iOS suite.
…with The budget is settled in the AndroidView factory, so a new timeoutMs handed to a live block is dropped. The View has no such trap — its timeout is a constructor argument — but the composable takes the parameter on every recomposition, and the Flutter widget already warns about the same thing. Compose was the only wrapper that stayed silent, which leaves a host debugging a budget it thinks it set against the clock. Once per value, not per frame: the effect is keyed on timeoutMs.
iOS dropped its half of this, so the pair no longer holds: `checkInappsTargeting` was the pre-rename spelling, and answering both names on Android alone only keeps a name alive that no platform is committed to. `filterShowableInapps` is the one name the bridge speaks. The constant, its registration, and the reverted test go back to what they were before the second spelling was added: a page that still asks the old name gets no answer and the interactor is never consulted.
deliverPendingEvent wrote LOADING into deliveredEvent, so the outcome that followed always looked new — even when it was the one the host had already heard. The controller reports Loading again on every start(), and that delivery lands before the resolve answers, so a place that is not in the config called onFail on every pass: every foreground, every re-attach of a recycled row, every setHostVisible(true) from Flutter. The listener KDoc promises each outcome once, and iOS already behaves that way — it returns nil for .loading and leaves the record alone. Loading is no longer an event at all: the delivery returns early and the record keeps the last real outcome. The LOADING case of the enum and its branch are gone with it.
handlesByPlace is a process-wide map, and it held its handles strongly. The only paths that ever closed a registration were release() from a wrapper and the onDestroy of a ViewTreeLifecycleOwner, so a block in a plain Dialog, in a PopupWindow, or in an app that swaps its own views had neither: findViewTreeLifecycleOwner() is empty there, nothing was installed, and closing the host leaked the block, its view, its WebView and the Activity behind them. iOS has a third exit in deinit; Android had two. The map now keeps weak references and prunes the cleared ones whenever a place is looked up, so letting the view go is enough on Android as well. Nothing else changes: the handle is the content controller itself, which the view holds for as long as it lives.
A released block still took a listener: setListener passed the identity check, cleared the record of what was delivered and scheduled a delivery, so a dead block called the host back once. setAppearanceObserver rearmed the observer release had just dropped, and a re-attached released view registered hostDestroyObserver on the host lifecycle again — a stale registration, since the content stays stopped. iOS closes both doors with `guard !isReleased` on the delegate. Both setters and the lifecycle observation now check isReleased, and release() has the KDoc its iOS twin has: what it stops, that it is for wrappers rather than host applications, and that it is one way.
Android trimmed the name in five places — the config mapping, the selection, the place filter, the registry's host input and the operation events — while iOS trimmed only the config. A name copied from the admin panel with a trailing space therefore worked on Android and silently collapsed on iOS: the same widget, the same config. Rather than teach the other side to trim, both now compare what they were given. The only question left about a name is whether it is empty, so the view asks isNotEmpty instead of isNotBlank and the variant validator asks isNullOrEmpty instead of isNullOrBlank: a name of spaces is a name like any other, and a place nobody spelled that way simply never resolves.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three commits on top of
mission/stories, all driven by what a wrapper needs from the embedded block, plus the show-accounting fix that falls out of it.What changed
Report what the block shows instead of whether it is visible. A wrapper that lays the block out itself was told only whether the view is visible, and a boolean cannot tell an error screen from a shimmer — both keep the place occupied, and only the container knows which it settled on.
MindboxEmbeddedBlockAppearancenames what is drawn (placeholder, content, error, collapsed) andsetAppearanceObserverhands the current value out on subscribing, which the boolean observer never did.appearanceFor(state)is the single place that decides, so the view's ownGONEcannot drift fromCOLLAPSED.setHostVisible(Boolean)adds a second source for the same input as window visibility — a Flutter or React Native screen never leaves the app's single window, so without it a block spends its waiting budget on a screen nobody is looking at.Count a show where somebody is looking; hold what failed off screen. The show was reported the moment the page said
contentRendered, whether or not anybody could see it. It is now counted at the render if the block is on screen, otherwise by thestart()that brings it back.timeToDisplayis frozen where the render was reported, so it measures the wait for the page rather than the user's absence from the screen. Failures follow the same rule and are held for the return; only the first is kept.Let a wrapper set the block's timeout in code. The budget only came from
app:mindboxTimeoutMs, so the Flutter and Compose hosts — which build the block programmatically — were stuck with the default 30 s, while the iOS container has taken atimeout:since it was written. The programmatic constructor takes one, and the internal one prefers it over the attribute.Note for review: the branch was rebased onto
mission/storiesIt previously sat on
23910c9f, so this is a force-push, not a fast-forward. Two things a reviewer should know:One commit was dropped deliberately.
MOBILE-341: Answer the targeting question under both of its namesadded afilterShowableInappsenum entry and registered it as an alias forhandleCheckInappsTargetingAction, on the premise that shipped pages still askcheckInappsTargeting. On this base both halves are moot:filterShowableInappsis already the name, andMOBILE-329cutcheckInappsTargetingon purpose — "Cut hard, in sync with iOS: the action never shipped, so no installed SDK speaks it" — with a test asserting it is not answered. Taking this base's decision, the commit is gone. If the web side confirms deployed pages do sendcheckInappsTargeting, it has to come back — as a realCHECK_INAPPS_TARGETINGentry with its handler, and the test atEmbeddedBlockWebViewHolderTest.ktrewritten.Dropping it also removed
registerSuspend(FILTER_SHOWABLE_INAPPS, ::handleCheckInappsTargetingAction), which by last-wins was overriding the::handleFilterShowableInappsActionregistered on the line above it.filterShowableInappsnow reaches its own handler.Nothing else was lost in the rebase: the four tests that this branch no longer carries relative to its old tip were all removed by
MOBILE-329itself (git log -Sconfirms), and three of them exist here underfilterShowableInappsnames.Testing
./gradlew :sdk:testDebugUnitTest— 2833 tests, 0 failures, 166 classes./gradlew :sdk:ktlintCheck :sdk:detekt— clean./gradlew :sdk:compileDebugKotlin— clean🤖 Generated with Claude Code