Stop the First Look banner retrying a slot the player hid - #9
Stop the First Look banner retrying a slot the player hid#9antonurankar-moloco wants to merge 15 commits into
Conversation
…to two formats Banner readiness was tracked with two sticky booleans that were set on load and never cleared, so the first successful fill from either source froze the slot for the whole screen visit. After a CloudX no-fill the AdMob fallback owned the placement until the scene was destroyed, and CloudX was never asked again - the opposite of what First Look promises. The screen's backoff retry and the LoadBanner() inside ToggleBanner were both unreachable in that state. An inline ad has no consumption event the way a fullscreen one does, so displaying it is now what spends the pass: a load into an already-visible view renders immediately, which makes "on screen" the one honest moment to treat a fill as used. ShowSource clears both flags and raises PassSpent; the screen schedules the next pass PassCooldownSeconds later, and that pass starts at CloudX again. Reloading is in place rather than a recreate, so a visible ad is replaced only once the new one has filled and the slot never blanks. The cycle turns only while an ad is on screen - hiding cancels the pending pass - and a fill the AdMob console refreshed on its own does not count as a pass, or an AdMob unit with Automatic refresh enabled would postpone CloudX's next first look on every refresh. The screen now covers interstitial and banner only. Those are the two shapes the rule has to handle; rewarded repeats the interstitial and MREC repeats the banner, and the General screen already demonstrates all four formats. With two formats left, each family base had exactly one subclass, so the bases are gone and each controller is one self-contained file a publisher can copy on its own alongside FirstLookSource.cs. That duplicates about fifty lines of ad-unit and dispose bookkeeping between the two files, on purpose, so neither drags a shared base into someone else's project. AdScreenUi closes the portrait column when a screen hides buttons, so the two remaining ones sit together instead of leaving a hole. It is a no-op when nothing is hidden, which is the General screen. Verified on the Android emulator and the iOS simulator, on the CloudX path and the forced no-fill path: CloudX re-asked on every pass, a 30 s cadence that follows the constant rather than the SDK's own inert refresh timer, zero requests while hidden, no blank frame on swap, and no drift across rotations.
OnAdMobLoaded set _adMobLoaded unconditionally, including for a fill the AdMob console refreshed on its own. When that fill was not displayed - which happens exactly when CloudX is the shown source, because ShowIfWanted refuses to let a non-pass fill take the slot from CloudX - the flag stayed set. ReadySource then reported AdMob, the scheduled Load() returned early, and since nothing went on screen no PassSpent fired and no further pass was ever scheduled. The cycle stalled for good with CloudX on screen and a stale AdMob fill banked: the original latch, re-entered through the refresh path. A fill from a pass may sit unspent until the slot is shown - that is what banks an ad for the first tap - but a fill nobody asked for may not. ShowIfWanted now reports whether it displayed, and both load handlers keep the loaded flag only for an unshown fill that came from a pass. Nothing is lost by forgetting the rest: the native view keeps the creative and the next pass reloads that side. Found by Copilot on PR #8. Re-verified on the Android emulator and the iOS simulator, both fill paths: 30 s cadence held (Android CloudX 30.2/30.3 s, forced no-fill 30.6/30.5/30.6 s across four passes), an unsolicited AdMob refresh at 14:55:46 did not move the next pass at 14:55:50, the preloaded fill was still banked and shown on the first tap with no new load, and hide then show re-showed with zero new requests.
The docs page used to carry its own copy of both controllers and now links here instead, which makes these files the thing a publisher actually reads. The 48-line banner header was written for the other arrangement: it re-explained the whole pass cycle, which the page already does, and it cited things a publisher copying the file does not have - FirstLookScreen.ToggleBanner, GeneralScreen, FirstLookConfig.PassCooldownSeconds. What each header keeps is what someone reading this file needs and cannot get from the code: what to copy, the reading order, why an inline ad needs a pass cycle when a fullscreen one does not, the two things the host must do or the cycle stalls, and the AdMob console setting no code can apply. Everything else now points at the page. The reasoning per rule stays where it always was, in the comments at the lines it governs. The CloudX placement and custom data strings say they are this demo's and are the caller's to replace, which nothing said before. Comments only - no logic changed, verified by diffing out comment lines - so the device verification from the earlier commits still stands.
The two controllers already carried the link. The other three did not, and one of them is a file the page tells publishers to copy: FirstLookSource.cs. Anyone who lands in it from a repo browse or a copy-paste had no way back to the explanation. All five now carry it, and the round trip closes: the page links to the files, the files link to the page. FirstLookScreen.cs gets one extra line, because it is the only place that shows the half of the banner contract the controller cannot keep on its own - ScheduleNextBannerPass starting the next pass a cooldown after PassSpent, and ToggleBanner cancelling it on hide. That is what a reader is looking for when the page tells them the host owns the clock. Comments only, verified by diffing out comment lines. The URL returns 200.
FirstLookScreen had two places arming the same banner retry and only one knew about the hide. ToggleBanner cancelled the pending pass with CancelInvoke, which empties Unity's invoke queue and nothing else; the AdLoadFailed handler armed a new load unconditionally. A load already out on the network is not in that queue, so it failed after the hide and armed a fresh request against an off-screen slot. Load() never consults _wantShown, so the request went out, and with both sources no-filling the backoff climbed to its 60s cap and repeated until the scene was destroyed. Reproduced on a Pixel 6 emulator (API 35) against the unmodified parent commit: hide at 09:30:12.87, the in-flight load failed 2.5s later and armed a retry, then kept requesting at 09:30:58, 09:32:09 and 09:33:21 with the slot empty. The guard is the one the docs page already publishes in its host snippet: a flag survives a callback that arrives after the hide, where CancelInvoke cannot. It starts true on purpose - IsShown is not a substitute, because it is also false during the preload before the first Show(), where a retry is still wanted. The demo now matches its own documented snippet. A fill arriving while hidden was never part of the defect: KeepsUnspentFill banks it, ReadySource goes non-null and Load() early-returns, so that path already terminated. Also removes ForceCloudXNoFill and its ad-unit helper. It is an internal test switch and does not belong in the public sample; the README now points at DemoConfig for the same effect, as the docs page already does. Verified after the fix: "not retrying while hidden" then zero ad requests for 3m17s; preload retry intact (5 retries with the banner never shown); pass cadence 30.17/30.43s on Android and 31/30s on iOS; hide for 120s gives zero requests on both platforms and re-show returns the banked ad with no new load; interstitial unchanged. The loop itself was not reproduced on the iOS simulator, which has no network lever - iOS covers the fixed behaviour only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01441wbqMjaRA3DKBBZp52tC
…-latch # Conflicts: # Assets/Scripts/FirstLook/FirstLookConfig.cs # Assets/Scripts/FirstLook/FirstLookScreen.cs # README.md
The docs page had to paste a host snippet, because the half of the banner
contract the controller cannot keep - the clock - only existed inside
FirstLookScreen, mixed in with SDK init, the ATT gate, the interstitial and
UI plumbing. A pasted snippet is a second copy, and it drifted: it declared
its handler as FirstLookBannerController.Source, a type that does not exist,
because nothing ever compiled it.
FirstLookBannerHud is that clock as a MonoBehaviour, in one file. It owns
the cooldown, the retry backoff, the wanted flag and the show/hide toggle,
and it depends on nothing but FirstLookBannerController and FirstLookSource,
so the docs can link it instead of copying it. Its three rules are the ones
an integration gets wrong, and they are stated at the top of the file.
FirstLookScreen keeps only what a demo screen should: initialization, button
binding and status text. It drops from 390 lines to 332.
The pass cooldown moves with the cycle it paces, so FirstLookConfig is now
just the AdMob fallback ad unit ids.
Verified on a Pixel 6 emulator (API 35) and an iPhone 17 Pro simulator
(iOS 26.2). Cadence 30.17/30.13s Android and 31/30s iOS; hide gives zero ad
requests for 24 minutes on Android and 120s on iOS, and re-show returns the
banked ad with no new load; a load in flight at the hide still logs "not
retrying while hidden" and starts nothing; the preload retry still runs with
the banner never shown; interstitial unchanged on both.
One small loss: the demo used to log the retry delay ("retrying in 8s") and
now logs only whether it retried, because the backoff moved inside the hud.
The two outcomes are still distinguishable via FirstLookBannerHud.IsWanted.
Copilot's review point on the docs PR applies here too: the README said a load in flight when the player hides "fails after the hide", as if that were certain. It is not. Hiding does not invalidate a load - a fill is banked for the next show, which is exactly the behaviour the pass cycle relies on. Only a load that then fails can re-arm the retry, so the bullet now says that. NextRetryDelay took its counter by ref back when the banner and the interstitial shared it. The banner's backoff moved into FirstLookBannerHud, so both remaining call sites pass the same field; it is now an instance method over _interstitialRetries. Same arithmetic, same counter. Also names the hud in the file table row for FirstLookScreen.
There was a problem hiding this comment.
🟡 Changes recommended
Hiding during an in-flight CloudX load can still start a new AdMob fallback request while hidden.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Extracts banner retry/cooldown orchestration into a reusable HUD component and prevents terminal retries while hidden.
Changes:
- Adds
FirstLookBannerHudto own banner lifecycle timing. - Simplifies screen/config integration.
- Updates fallback and integration documentation.
File summaries
| File | Description |
|---|---|
README.md |
Documents the extracted banner cycle and fallback setup. |
Assets/Scripts/FirstLook/FirstLookScreen.cs |
Delegates banner behavior to the HUD. |
Assets/Scripts/FirstLook/FirstLookConfig.cs |
Removes cooldown and forced-no-fill configuration. |
Assets/Scripts/FirstLook/FirstLookBannerHud.cs |
Implements banner visibility, cooldown, and retry handling. |
Assets/Scripts/FirstLook/FirstLookBannerHud.cs.meta |
Registers the new Unity script asset. |
Review details
Files not reviewed (1)
- Assets/Scripts/FirstLook/FirstLookBannerHud.cs.meta: Generated file
- Files reviewed: 4/5 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
A HUD is a heads-up display. This class has no UI at all - no UnityEngine.UI, no Text, Button, Canvas or Rect - and what it does hold is the pass cooldown, the retry backoff, ScheduleNextPass and the show/hide toggle. That is the pass cycle, which is the term the README, the docs page and the controller comments already use for it. The name also collided with the file that really is the display: AdScreenUi owns the labels and buttons. Having AdScreenUi draw while something called Hud kept time was backwards. The name came from the old docs snippet, which was called BannerHud; it was carried over when the file was extracted rather than chosen. Rule 3 in the file header still said a load in flight "still fails afterwards" - the same overstatement corrected elsewhere after Copilot's review point, missed here. A fill is banked for the next show; only a failure re-arms the retry. The meta guid is unchanged, so nothing that referenced the file loses it.
…fallback Copilot's review point on #9, confirmed on device. The retry guard in FirstLookBannerCycle only runs after AdLoadFailed, and the controller raises that only when the AdMob leg fails. Hiding while the CloudX leg was still in flight left CloudXOnLoadFailed free to call LoadAdMobFallback, so one AdMob request went out on a slot that was already off screen. That contradicts the "a hidden slot never keeps requesting" claim in the README and on the docs page, and my earlier zero-request measurements did not cover it: they were taken from after the failure had resolved. _wantShown cannot be the gate, because it is also false during the preload before the first Show, and the preload has to be allowed to reach the fallback - that is what banks an ad for the first tap. So this adds _hidden, set by Hide and cleared by Show, which separates "never shown yet" from "explicitly hidden". Measured on a Pixel 6 emulator, hiding inside the CloudX leg (network off, so the leg lasts through an HttpRetry cycle rather than failing instantly): with the gate load() 21:47:00.626, hide 21:47:00.825 -> 0 AdMob requests without the gate load() 21:49:25.594, hide 21:49:25.796 -> 1 AdMob request at 21:49:27.032, 1.2s after the hide Regression on the same build: pass cadence 30.27s, hide gives zero loads and zero AdMob requests over 70s, re-show returns the banked ad with no new load, and a preload against an invalid CloudX unit still falls back to AdMob.
There was a problem hiding this comment.
🟡 Changes recommended
Re-showing before an in-flight callback completes can revive the cancelled pass and bypass the restarted cooldown.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- Assets/Scripts/FirstLook/FirstLookBannerCycle.cs.meta: Generated file
Suppressed comments (1)
Assets/Scripts/FirstLook/FirstLookBannerCycle.cs:120
- Re-enabling
_wantedhere similarly revives a cancelled AdMob pass. When the player hides during an AdMob load and re-shows the retained creative before the no-fill callback arrives, that old callback reachesOnAdLoadFailedafter_wantedbecame true and schedules a retry, so the new show's 30-second cooldown is bypassed. Keep the cancelled in-flight pass distinct from the slot's current wanted state and suppress its retry even after a re-show.
_wanted = true;
- Files reviewed: 5/6 changed files
- Comments generated: 1
- Review effort level: Balanced
Copilot's follow-up on #9, confirmed on device. The previous commit cleared the flag in Show(), which tracks whether the slot is currently visible rather than whether the running pass was cancelled. Hide then a quick re-show while a CloudX load is still in flight therefore revived it: the stale terminal callback arrived with the flag already cleared, started the fallback at once, and jumped the 30s cooldown the re-show had just restarted. The flag now belongs to the pass. Hide sets it; only the start of a new pass in Load() clears it. Show leaves it alone, so a cancelled pass stays cancelled however the slot is toggled afterwards. The same reasoning applies to the AdMob leg, which Copilot raised as a second case: its terminal failure is no longer forwarded to the host for a cancelled pass, so the host cannot schedule a retry that jumps the cooldown either. Measured on a Pixel 6 emulator, hiding and re-showing inside the CloudX leg (network off, so the leg lasts through an HttpRetry cycle): pass-scoped flag hide 22:05:41.507, show 22:05:41.588 -> 0 AdMob requests cleared on Show hide+show 22:07:31.846 -> 2 AdMob requests Regression: cadence 30.23s, hide gives zero loads and zero AdMob requests over 65s, re-show returns the banked ad with no new load. Copilot also noted that a cooldown load firing before a stale callback is dropped by the _isLoadingCloudX guard. That is the pre-existing no-per-pass- timeout gap already recorded as a follow-up; it predates this branch and is not addressed here.
There was a problem hiding this comment.
🟡 Changes recommended
Integration guidance and the declared PR scope remain inconsistent with the extracted component and controller changes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- Assets/Scripts/FirstLook/FirstLookBannerCycle.cs.meta: Generated file
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
Assets/Scripts/FirstLook/FirstLookBannerController.cs:30
- The controller’s updated integration contract still says
Show()starts the cycle, while the header at lines 8–11 tells users to copy only this controller andFirstLookSourceand calls this file “the whole flow.” After this extraction, following those instructions omitsFirstLookBannerCycle, so no cooldown/retry clock is installed and the banner stalls after its first pass. Update this class-level guidance to name all three required files and make the cycle responsible for timing.
- Files reviewed: 5/6 changed files
- Comments generated: 1
- Review effort level: Balanced
The controller header still told readers to copy this file and FirstLookSource.cs and called it "the whole flow". That was true before the timing was extracted; following it now leaves nothing driving the cycle, so the banner shows one ad and stops. It names all three files, and says what happens if you take only this one. The two host rules stay, since anyone driving the controller from their own component still needs them, but they now say FirstLookBannerCycle already does both. The Hide-ends-the-pass behaviour moves out of that list, because it is this controller's job rather than the host's. FirstLookInterstitialController keeps the same two-file wording: a fullscreen ad is consumed by being shown, so it needs no clock and the claim is accurate there.
There was a problem hiding this comment.
🟡 Changes recommended
Successful callbacks can still revive a cancelled pass after a quick re-show.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- Assets/Scripts/FirstLook/FirstLookBannerCycle.cs.meta: Generated file
- Files reviewed: 5/6 changed files
- Comments generated: 1
- Review effort level: Balanced
Copilot's third-round point, and it is a violation of the contract the header gained last round: it says a later Show does not revive a cancelled pass, and the success callbacks did exactly that. Hide, then a quick Show, then the stale CloudX or AdMob fill arrives with its loading flag still set, so it raised PassSpent and reset the cooldown from a pass the player had already dismissed. The cause is that one flag carried two meanings. spendsPass answered both "did this controller ask for the load" - which decides whether the fill is banked, and whether it may take the slot from the other source - and "should this re-time the cycle". Those come apart exactly once: a pass a Hide cancelled is still ours, but its timing is not. So they are two flags now. "ours" keeps the banking and source-replacement behaviour unchanged, and spendsPass is ours && !_passCancelled. A cancelled pass's fill is still banked while the slot is hidden, and still shows if the slot has since been shown - throwing away an ad we paid a request for would be worse - but the cooldown stays with whatever the host scheduled after the hide. Regression on a Pixel 6 emulator: cadence 30.23s, hide gives zero loads and zero AdMob requests over 60s, re-show returns the banked ad with no new load.
Begin's comment said "after both SDKs have answered", which contradicts what FirstLookScreen does and what the docs page now says. The screen starts both initializations and calls Begin on the CloudX result alone, because Google Mobile Ads queues loads issued while it is still initializing and the fallback is lazy regardless. Part of a three-way inconsistency Copilot found across this comment, the Info block on the docs page, and the usage section; the other two are fixed in cloudx-io/docs#407.
There was a problem hiding this comment.
🟢 Approval recommended
The cancellation flow is coherent, with only a non-blocking unreachable diagnostic branch remaining.
Review details
Files not reviewed (1)
- Assets/Scripts/FirstLook/FirstLookBannerCycle.cs.meta: Generated file
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
Assets/Scripts/FirstLook/FirstLookScreen.cs:263
FirstLookBannerControllerdrops cancelled AdMob failures before raising this event (lines 491–493), and cancelled CloudX failures never raise it either, so every failure reaching this handler still hasIsWanted == true. Thenot retrying while hiddenarm is therefore unreachable and cannot produce the diagnostic cited in verification; log the retry outcome unconditionally here (or add a distinct cancellation event if cancellations must be observable).
- Files reviewed: 5/6 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Copilot's non-blocking note, and it is right. Once the controller started dropping the terminal failure of a cancelled pass, nothing reaching the screen's AdLoadFailed handler can be from a hidden slot, so the "not retrying while hidden" arm became dead and IsWanted lost its only caller. The handler now logs the one outcome that can actually happen. This also corrects the record: that log line was real evidence when it was gathered, against the build at the time, but the current build cannot emit it. What replaces it as evidence is the request count, which is the thing that actually matters - zero AdMob requests after a hide, measured against a control build that produces one. The cycle keeps its _wanted guard even though the controller now makes it unreachable, because the rule is the host's to keep: anyone driving the controller from their own component needs that line. The comment says so rather than leaving it looking like live logic. Regression: cadence 30.25s, zero loads and zero AdMob requests over 55s hidden, re-show returns the banked ad with no new load.
There was a problem hiding this comment.
🟢 Approval recommended
The cancellation state consistently covers both SDK failure and success paths while preserving preload and banked-fill behavior.
Review details
Files not reviewed (1)
- Assets/Scripts/FirstLook/FirstLookBannerCycle.cs.meta: Generated file
- Files reviewed: 5/6 changed files
- Comments generated: 0 new
- Review effort level: Balanced
The defect
FirstLookScreenhad two places arming the same banner retry, and only one knew about the hide.ToggleBannerhandled the hide with_banner.Hide()thenCancelInvoke(nameof(LoadBanner)). That empties Unity's invoke queue and nothing else.AdLoadFailedhandler armedInvoke(nameof(LoadBanner), delay)unconditionally.A load already out on the network is not in that queue. So: a pass comes due and
LoadBannerfires, CloudX misses, the AdMob leg goes out, the player taps Hide,CancelInvokecancels nothing, AdMob no-fills, and the handler arms a fresh load against an off-screen slot.FirstLookBannerController.Load()gates on_isDisposed, the in-flight flags andReadySource— never on_wantShown— so the request goes out. With both sources no-filling the backoff climbs to its 60s cap and repeats until the scene is destroyed.It also broke a contract the README stated: "Hiding cancels the pending pass, so a hidden slot never keeps requesting in the background."
Reproduced on a Pixel 6 emulator (API 35) against the unmodified parent commit, network killed so both legs no-fill:
13 SDK-level request lines went out after the hide, with the slot empty.
One correction to the original report
The report claimed a fill in this state means the demo "bought an ad for a hidden banner and spent the pass on nothing". It does not.
KeepsUnspentFillreturnsspendsPass && !wentOnScreen, so a fill arriving while hidden is banked:ReadySourcegoes non-null,Load()early-returns, the loop stops, and the nextShow()puts that ad up. Only the sustained no-fill branch was broken.The fix
A flag survives a callback that arrives after the hide, where
CancelInvokecannot. It startstrueon purpose —IsShownis not a substitute, because it is also false during the preload before the firstShow(), where a retry is genuinely wanted.This is the guard the docs page already publishes in its host snippet, so page and demo now agree rather than diverge. The companion docs PR is cloudx-io/docs#407.
Also: the pass cycle moved into its own component
The half of the contract the controller cannot keep — the clock — only existed inside
FirstLookScreen, mixed in with SDK init, the ATT gate, the interstitial and UI plumbing. That is why the docs page had to paste a host snippet, and a pasted snippet is a second copy: it drifted into declaringFirstLookBannerController.Source, a type that does not exist, because nothing ever compiled it.FirstLookBannerCycle.csis that clock as aMonoBehaviour, in one file. It owns the cooldown, the retry backoff, the wanted flag and the show/hide toggle, and depends on nothing butFirstLookBannerControllerandFirstLookSource— so the docs link it instead of copying it. Its three rules are stated at the top of the file.FirstLookScreenkeeps only what a demo screen should: initialization, button binding, status text. 390 lines to 332. The pass cooldown moves with the cycle it paces, leavingFirstLookConfigas just the AdMob fallback ad unit ids.This is the smaller of two options discussed. It removes one class of drift — the sample can no longer fail to compile — but publishers still hand-write the host. Folding the cycle into the controller so there is no host contract at all is the follow-up, to be done once this squashes.
Also:
ForceCloudXNoFillremovedAn internal test switch that does not belong in the public sample.
FirstLookConfigkeeps only the AdMob ad unit ids and the pass cooldown. The README now points atDemoConfigfor the same effect, which is what the docs page already tells publishers to do.Interstitial
Not the same hole, no change. It arms retries the same unconditional way, but a fullscreen ad has no visibility state — no
Hide, nothing to cancel — and keeping one preloaded is the intent. One adjacent thing checked while there: neither handler callsCancelInvoke, so twoLoadInterstitialinvokes can stack; the second is absorbed by the guards atFirstLookInterstitialController.cs:96. Harmless.Verification
Pixel 6 emulator (API 35) and iPhone 17 Pro simulator (iOS 26.5). Full receipts with logs and screenshots: https://claude.ai/code/artifact/f8dfc741-c1fb-4c40-8689-b5c0b4a0bb21
not retrying while hidden, then 0 ad requests for 3m17serror CSRe-verified after the extraction
not retrying while hidden, nothing startedNot reproduced: the loop itself on iOS. The simulator shares host networking and has no per-device network lever, so iOS covers the fixed behaviour only. I am not arguing it from equivalence with Android — it was not observed either way. Same for the preload-retry case, which needs the network down.
Unexplained, noted not claimed: on iOS one pass landed at the moment the interstitial closed rather than 30s after the re-show. Consistent with Unity time not advancing under a native fullscreen ad, but not isolated.
Scope
FirstLookBannerCycle.csFirstLookBannerController.csFirstLookScreen.csFirstLookConfig.csREADME.mdNo scene and no
ProjectSettings.FirstLookInterstitialController.csis untouched.