fix(ci): ship real AdMob IDs in the release bundle, not just prebuild (#232) - #233
Conversation
The release workflow set the production AdMob env on the prebuild step but not on the gradle bundleRelease step. bundleRelease re-evaluates app.config.js to regenerate the embedded Constants.expoConfig.extra.adMob snapshot, so without the env it defaulted useTestIds to true and shipped Google test ad unit ids to every free-plan user (0 real requests). - Hoist EAS_BUILD_PROFILE + the four EXPO_PUBLIC_ADMOB_ANDROID vars to the job-level env so prebuild and bundleRelease evaluate app.config.js identically; drop the now-redundant per-step duplicates. - Add a post-bundleRelease guard (assert-aab-admob-config.js) that inspects the shipped AAB's JS bundle and fails on Google sample ids or missing real unit ids -- the prebuild-source guards could not catch this. - Extract the pre-build config assertion into assert-production-admob-config.js shared by both guards. - Fail-fast the local release scripts (android-release-apk/device) when the production AdMob env is missing instead of silently building a test-id release. - Lock the selection contract in use-ad-mob tests: useTestIds=false uses real ids and never TestIds.*, undefined falls back to __DEV__, and a missing prod id yields no ad request. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
There was a problem hiding this comment.
The fix is correct and well-executed. Hoisting the four EXPO_PUBLIC_ADMOB_* vars to job-level env: is exactly the right root-cause fix — it ensures gradlew bundleRelease sees the same environment that expo prebuild did when it evaluated app.config.js, closing the gap that caused test-ad delivery to free-plan users. Extracting the inline Node.js assertion into assert-production-admob-config.js plus adding the new post-bundleRelease AAB bundle inspection (assert-aab-admob-config.js) gives two independent validation layers: one on the prebuilt source, one on the actual shipped artifact. The fail-fast in production-admob-env.js for local release scripts prevents silent test-id builds locally too. The three new test cases in use-ad-mob.test.tsx meaningfully lock the selection contract (falls back to test IDs when useTestIds is absent, uses prod IDs when useTestIds=false, emits no ad request when prod IDs are null). No high-signal issues found.
There was a problem hiding this comment.
Code Review: PR #233
Scope: PR #233 — fix(ci): ship real AdMob IDs in the release bundle, not just prebuild
Recommendation: APPROVE (with one Medium to address)
Summary
This PR fixes a real revenue bug: the bundleRelease Gradle step re-evaluated app.config.js without the production AdMob env vars, so the JS bundle shipped with Google test ad unit IDs even though the manifest had real ones. The fix hoists env vars to job level so all steps share them, extracts the inline validation script into reusable Node modules, adds a post-bundleRelease AAB bundle inspection step, and closes the same env gap in the local release scripts. The approach is correct and the defence-in-depth guard layering (config validation → prebuild validation → AAB bundle scan) is well designed.
Findings
Critical
None.
High
None.
Medium
[MEDIUM] assert-aab-admob-config.js does not verify the App ID in the shipped bundle
- dimension: Correctness (#1)
- location:
apps/mobile/scripts/assert-aab-admob-config.js:30-33 - issue:
assertAabAdMobConfigchecksEXPO_PUBLIC_ADMOB_ANDROID_INTERSTITIAL_IDandEXPO_PUBLIC_ADMOB_ANDROID_REWARDED_IDappear in the bundle, but does not checkEXPO_PUBLIC_ADMOB_ANDROID_APP_ID. The App ID determines which AdMob account impressions are credited to — a mismatch here means $0 regardless of correct unit IDs. - risk: A build where the App ID reverts to the Google sample value would pass the AAB guard and ship, silently earning nothing. The pre-
bundleReleaseguard (assert-production-admob-config.js) does check the App ID, but the whole point of the AAB-level guard is to catch what that earlier guard cannot — a bundler pass that overwrites config. - fix: Add
EXPO_PUBLIC_ADMOB_ANDROID_APP_IDtoexpectedIds:
const expectedIds = [
readRequiredEnv("EXPO_PUBLIC_ADMOB_ANDROID_APP_ID"),
readRequiredEnv("EXPO_PUBLIC_ADMOB_ANDROID_INTERSTITIAL_ID"),
readRequiredEnv("EXPO_PUBLIC_ADMOB_ANDROID_REWARDED_ID"),
];- reference: CLAUDE.md rule 8; PR's stated intent to guard the shipped bundle
[MEDIUM] console.log in new production-path scripts
- dimension: No
console.log(#7) - location:
apps/mobile/scripts/assert-production-admob-config.js:94,apps/mobile/scripts/assert-aab-admob-config.js:66 - issue: Both new scripts use
console.log("... validation passed.")under theirrequire.main === moduleguards. CLAUDE.md rule 4 bansconsole.login production code; CI/release scripts count as production code. - fix: Replace with
process.stdout.write("... validation passed.\n")— idiomatic Node replacement that doesn't invoke the bannedconsoleAPI. - reference: CLAUDE.md rule 4
Low / Info
[INFO] unzip system binary assumed present on CI runner
- location:
apps/mobile/scripts/assert-aab-admob-config.js:20 - issue:
execFileSync("unzip", ...)depends onunzipbeing installed on theubuntu-latestimage. Present today, but runner image contents aren't guaranteed stable across updates. - fix: Either add
sudo apt-get install -y unzipbefore the AAB verification step, or use an npm package (adm-zip,yauzl) to eliminate the OS binary dependency.
[INFO] resolveProductionAdMobEnv returns a full env spread
- location:
apps/mobile/scripts/production-admob-env.js:20-24;apps/mobile/scripts/android-release-apk.js:9 - issue:
Object.assign(process.env, resolveProductionAdMobEnv(process.env))globally mutatesprocess.env. Correct for a top-level script, but a subtle side-effect if the function is ever reused in a testable context. - fix: No immediate change needed. Informational only.
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | PAIRED — all changes are Android-only (CI, build scripts, mobile-only ads hook test); no web mirror required or missing |
| i18n-syncer | IN SYNC — no user-facing strings introduced |
| contract-aligner | N/A — no packages/shared types or endpoints.ts changed |
| security-reviewer | N/A — no orbit-api code changed |
Validation
| Check | Result |
|---|---|
| Lint | N/A (shell policy; author reports passing) |
| Type check | N/A (shell policy; author reports passing) |
| Tests | N/A (shell policy; author reports 580 passing incl. new selection-contract cases) |
| Build (api) | N/A (mobile-only PR) |
What's good
- Root-cause fix is correct: hoisting env vars to job level is the right approach rather than repeating them per-step.
- Extracting the inline heredoc JavaScript into proper modules (
assert-production-admob-config.js,assert-aab-admob-config.js,production-admob-env.js) makes the logic independently readable and testable. - The post-
bundleReleaseAAB bundle scan closes the exact false-confidence gap described in the PR body — the pre-bundleReleaseconfig guard was verifying a snapshot thatbundleReleasecould then silently override. - New
use-ad-mobtest cases (fallback-to-test-ids whenuseTestIdsis absent, no-op when prod IDs are null) are behaviorally meaningful and cover real contract corners. resolveProductionAdMobEnv's fail-fast with a clear human-readable error message is exactly right — no ambiguous silent failures.- No dead code, no
any, no workarounds. Diff is clean.
Recommendation
Approve after addressing the substantive Medium: add EXPO_PUBLIC_ADMOB_ANDROID_APP_ID to the AAB-level check in assert-aab-admob-config.js. Without it, the App ID — the field that ties impressions to the AdMob account — is verified only by the pre-bundleRelease guard, which is precisely the guard this PR argues is insufficient in isolation. The console.log Medium can be bundled with that fix or tracked as a follow-up.
|
Combine #233's job-level AdMob env hoist with #239's QA build-target resolution: prebuild AND bundleRelease map the BUILD_* overrides, so the internal/QA track ships test IDs at bundle time (not just prebuild); gate #233's shipped-AAB real-ID guard to non-internal tracks. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>



What
Free-plan production users were served Google test ads ("Anúncio de teste") instead of real ones, so the free tier earned $0.
Root cause
android-release.ymlset the production AdMob env on theexpo prebuildstep but not on thegradlew bundleReleasestep.bundleReleasere-evaluatesapp.config.jsto regenerate the embeddedConstants.expoConfig.extra.adMobsnapshot, so without the env it defaulteduseTestIds:trueand shipped Google test ad unit IDs to every user. The manifest app ID was real (prebuild had the env) and the existing guards passed because they inspect the prebuilt source, not the shipped bundle — false confidence.Fix
EAS_BUILD_PROFILE=production,EXPO_PUBLIC_ADMOB_USE_TEST_IDS=false, and the three realEXPO_PUBLIC_ADMOB_ANDROID_*IDs to job-levelenv:so prebuild and bundleRelease agree.bundleReleaseguard that inspects the shipped AAB's JS bundle (fails on Google sample IDs / missing real unit IDs).android-release-apk.js,android-release-device.js), fail-fast if prod IDs are absent.use-ad-mobunit tests to lock the selection contract.Validation
lint ✓ · type-check ✓ · 580 mobile unit tests ✓ (incl. the new selection-contract cases).
Still required (manual — AC4)
Re-cut a build via this workflow to the internal/open track, then on a real Android device + free-plan account confirm real (non-"Test Ad") rewarded + interstitial creatives and AdMob requests/impressions climbing off 0; spot-check that a pro account shows no ads.
Closes #232
🤖 Generated with Claude Code