Make crust's Mapbox Navigation dependency opt-in (default off)#3372
Make crust's Mapbox Navigation dependency opt-in (default off)#3372PhilippeFerreiraDeSousa wants to merge 1 commit into
Conversation
Embedding crust no longer forces a MAPBOX_DOWNLOADS_TOKEN on every host.
The config plugin gains a navigation prop (default false):
- off: the Mapbox Downloads repo is NOT injected and the Nav SDK is
compileOnly (generalizing the existing China-build condition) — crust
compiles, the classes are absent from the APK, and no credential is
needed to build. protobuf exclusion + desugaring still apply.
- on: repo injected, Nav SDK is implementation (in the APK). The Mentra
app passes {navigation: true}.
The plugin writes the choice to the mentraCrustNavigation gradle
property that android/build.gradle reads. CrustModule gains a lazy
navAvailable reflection check (Class.forName on MapboxNavigation,
catching Throwable since a missing class is an Error); the seven nav
AsyncFunctions return a clean 'navigation not available in this build'
result when it's absent instead of crashing with NoClassDefFoundError.
This also hardens the pre-existing China compileOnly path, which relied
on JS never calling nav.
Verified by prebuild: Mentra app -> mapbox repo present,
mentraCrustNavigation=true; example OEM app -> no mapbox repo,
mentraCrustNavigation=false, protobuf+desugar still applied. Android
compile check (nav on) BUILD SUCCESSFUL.
PhilippeFerreiraDeSousa
requested review from
AryanFP and
aisraelov
as code owners
📋 PR Review Helper📱 Mobile App Build✅ Ready to test! (commit 🕶️ ASG Client Build⏳ Waiting for build... 🔀 Test Locallygh pr checkout 3372 |
PR Agent Orchestrator State{
"cycle": 1,
"fixRound": 0,
"totalReviewerRuns": 1,
"consecutiveNoNewReviews": 1,
"openFindings": [],
"resolvedFindings": [],
"nitFindings": [],
"phase": "discovery",
"status": "in_progress",
"stagnationFixRounds": 0,
"fingerprintReopenCounts": {},
"mutedFingerprints": [],
"lastPair": [],
"lastOpenCount": 0
} |
🤖 PR Agent Review — cycle 1✅ No blocking findings · 0 blocking · 0 nits No model reviews ran this cycle. Updated automatically by the PR Agent Orchestrator each review cycle. Nits do not block merge. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9f6602b. Configure here.
| // compiles against the SDK — the classes are just absent at runtime, and | ||
| // CrustModule's navAvailable guard returns a clean error if nav is invoked. | ||
| def navigationEnabled = (findProperty("mentraCrustNavigation") ?: "false").toString() == "true" | ||
| if (isChinaBuild || !navigationEnabled) { |
There was a problem hiding this comment.
Nav-off compile needs Mapbox repo
High Severity
When navigation is off, the crust plugin stops injecting the Mapbox Downloads Maven repo, but :mentra-crust still declares compileOnly Mapbox Navigation SDK dependencies. Gradle must still resolve those artifacts to compile NavigationManager.kt, which only exists in Mapbox’s private registry—so credential-free OEM builds fail on a clean cache.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 9f6602b. Configure here.
| AsyncFunction("requestNavigationPermission") { promise: expo.modules.kotlin.Promise -> | ||
| if (!navAvailable) { | ||
| promise.resolve(navUnavailableResult) | ||
| return@AsyncFunction |
There was a problem hiding this comment.
Permission response missing accepted
Low Severity
When navigation is unavailable, requestNavigationPermission resolves navUnavailableResult, which only has ok and error. Other failure paths include accepted: false, and NavigationService.requestPermission types the result as {ok, accepted, error?}—callers may treat accepted as undefined instead of explicitly false.
Reviewed by Cursor Bugbot for commit 9f6602b. Configure here.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f6602b613
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| compileOnly 'com.mapbox.navigationcore:navigation:3.25.0' | ||
| compileOnly 'com.mapbox.navigationcore:tripdata:3.25.0' |
There was a problem hiding this comment.
When navigation is omitted or false, the config plugin no longer injects the Mapbox Downloads Maven repo, but this branch still puts com.mapbox.navigationcore:* on crust's compileOnly classpath. compileOnly is still resolved when compiling NavigationManager.kt; it only keeps the artifacts out of the APK. In a clean OEM host with no Mapbox repo/token and no cached artifacts, Gradle fails dependency resolution instead of producing the intended credential-free build.
Useful? React with 👍 / 👎.
| "build": "expo-module build", | ||
| "clean": "expo-module clean", | ||
| "lint": "expo-module lint", | ||
| "test": "expo-module test", | ||
| "prepare": "expo-module prepare && expo-module build plugin", | ||
| "prepare": "expo-module prepare", |
There was a problem hiding this comment.
app.plugin.js requires ./plugin/build, and that directory is not tracked in the repo; it is produced by expo-module build plugin. Dropping that command from the normal build/prepare path means a clean install or published package can leave the plugin output missing, so Expo fails to load @mentra/crust before prebuild reaches any of the new Gradle logic.
Useful? React with 👍 / 👎.
1 participant
Why
Embedding crust (via island) forced
MAPBOX_DOWNLOADS_TOKENon every host, because crust unconditionallyimplementations the Mapbox Nav SDK, whose artifacts live only in Mapbox's private registry. A brand-new OEM that never navigates still couldn't build without obtaining a Mapbox credential — the follow-up teed up in #3367.What
crust's config plugin gains a
navigationprop, defaultfalse:compileOnly— crust compiles, the classes are absent from the APK, and no credential is needed to build. The protobuf exclusion + desugaring still apply. This generalizes the condition crust's gradle already used for the China build.implementation(in the APK). The Mentra app passes["@mentra/crust", {navigation: true}].The plugin writes the choice to a
mentraCrustNavigationgradle property thatandroid/build.gradlereads (isChinaBuild || !navigationEnabled→compileOnly).Runtime safety —
CrustModulegains a lazynavAvailablereflection check (Class.forNameonMapboxNavigation, catchingThrowablesince a missing class is anError, not anException). The seven navAsyncFunctions (startNavigation,stopNavigation,requestNavigationPermission,resetNavigationPermission,simulateDeviation,setWrongSidewalkOffset,setSkipCrossings) return a clean{ok: false, error: "navigation not available in this build"}when nav is absent instead of crashing withNoClassDefFoundError. This also hardens the pre-existing ChinacompileOnlypath, which until now relied on the JS layer simply never calling nav.Verification
mentraCrustNavigation=true. Example OEM app (nav off, no arg): no Mapbox repo,mentraCrustNavigation=false, protobuf + desugaring still present.check-android-compile.sh bluetooth-sdk(compiles the Mentra Android project incl. crust + the new guard, nav on): BUILD SUCCESSFUL.bun run preparebuilds both the module and the plugin (the fresh-checkout CI path); mobile tsc clean; full jest 55 suites / 429; island tsc + 220; example OEM app typechecks.Unverified — needs on-device
The nav-off runtime path (guard returning the clean error) is exercised only by an OEM-style build; the compile check covers nav-on. Worth a spot-check on the example OEM APK once built: calling a nav command should log "navigation not available" and return the error, not crash. The Mentra app (nav-on) behavior is unchanged and fully covered.
Follow-up
iOS: the Nav SDK wiring (
mapbox-nav-ios.ts,mapbox-nav-crust-link.ts) still lives in mobile's local plugins; folding it into crust's plugin (and honoring the samenavigationflag) is the iOS half, deferred until an iOS OEM consumer exists.Note
Medium Risk
Changes Android embedding contract and native nav entry points; Mentra opts in explicitly, but wrong plugin config or missing on-device nav-off testing could surprise OEM integrators.
Overview
@mentra/crust now treats Mapbox Navigation as host opt-in (
navigation, defaultfalse) instead of always pulling the Nav SDK and Mapbox Downloads Maven repo.When navigation is off, the config plugin skips the authenticated Mapbox repo, sets
mentraCrustNavigation=false, and crust’s Gradle usescompileOnlyfor the Nav artifacts (same idea as the existing China build). Protobuf-javalite exclusion and core-library desugaring still apply. The Mentra app explicitly passes["@mentra/crust", { navigation: true }]so full nav builds are unchanged.CrustModuleadds a lazynavAvailablecheck (Class.forNameon Mapbox classes) and all seven navAsyncFunctions returnnavigation not available in this buildinstead ofNoClassDefFoundErrorwhen the SDK isn’t in the APK.package.jsondrops building the plugin from the defaultbuild/preparescripts (plugin still hasbuild:plugin).Reviewed by Cursor Bugbot for commit 9f6602b. Bugbot is set up for automated code reviews on this repo. Configure here.