Fix Mapbox crash: don't clear native access token to empty string on token clear - #97821
Fix Mapbox crash: don't clear native access token to empty string on token clear#97821MelvinBot wants to merge 2 commits into
Conversation
…y string Co-authored-by: Kevin Brian Bader <ikevin127@users.noreply.github.com>
|
I looked into the failing checks — there's no code failure to fix here. The only red check is Everything else that could reflect a real problem with the code is passing: typecheck, ESLint, Oxfmt, Bun tests, all Jest Action needed: complete the author checklist in the PR body — nothing to push. |
This comment was marked as resolved.
This comment was marked as resolved.
|
@ikevin127 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
🤖 Done,
All yours to start the review whenever you're ready. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb Chrome🟢 No GPS functionality on web. iOS: HybridAppScreen.Recording.2026-08-04.at.17.57.12.moviOS: mWeb Safari🟢 No GPS functionality on web. MacOS: Chrome / Safari🟢 No GPS functionality on web. Screen.Recording.2026-08-04.at.17.25.11.mov |
![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: 56a7423429
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // is cleared at runtime (expiry/refresh, app-foreground, reconnect) `accessToken` becomes '', so this | ||
| // returns false and consumers fall back to PendingMapView instead of leaving a map mounted / letting a | ||
| // fresh native MapView be constructed while the token is blank. | ||
| return hasSetAccessToken && !!accessToken; |
There was a problem hiding this comment.
When a token refresh clears accessToken to '' and then supplies a new token, hasSetAccessToken is still latched from the previous token, so this render returns true immediately for the new non-empty prop before the effect has run and resolved setAccessToken(newToken). Because the map was just unmounted during the empty-token interval, the recovery path can construct a fresh native MapView while the native global still contains the previous/expired token, leaving the refreshed map load using stale credentials. Track the specific token that was successfully set natively, or reset the latch until the current token's setter resolves.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@MelvinBot What do you think about this code comment ?
There was a problem hiding this comment.
My take: the observation is mechanically correct, but low severity and optional to address — it doesn't weaken the crash fix.
Codex has the ordering right. Because hasSetAccessToken never resets and the gate is hasSetAccessToken && !!accessToken, a tokenA → '' → tokenB refresh reopens the gate on the first render where accessToken === tokenB — and that's the same commit that mounts a fresh native MapView, which happens before the effect runs setAccessToken(tokenB). Since the fix deliberately never clears the global, it still holds tokenA at that instant, so the remounted map is constructed against the previous token and tokenB lands a beat later.
Why I wouldn't block on it:
- It's not a crash. The PR's actual target — the empty-token
MapboxConfigurationException— is fully fixed. Worst case in this scenario is a transient tile-load hiccup, not a throw. - The "previous" token is usually still valid. This is proactive ~25-min rotation, not hard expiry, so a map built against
tokenAtypically still loads, and the gate self-corrects on the next token/render cycle. - It's an orthogonal correctness nit, not a regression this PR introduces.
If you want to close the window cleanly — and it's arguably a touch simpler than the boolean-latch-plus-!!accessToken — gate on the specific token confirmed in the native global:
const [tokenSetNatively, setTokenSetNatively] = useState<string>();
// ...
setAccessToken(accessToken).then((token) => {
if (ignore || !token) {
return;
}
setTokenSetNatively(accessToken);
});
// ...
return tokenSetNatively === accessToken;Now the gate opens only once the current token has actually resolved into the native global, so a remount can't construct a MapView against the previous token, and '' still returns false so the crash fix is preserved.
TL;DR: worth a quick hardening if you want belt-and-suspenders, but not required to ship the crash fix.
|
🤖 I looked into the failing checks — there's nothing to fix in code. The only ❌ is PR Reviewer Checklist / checklist, which fails because the reviewer checklist isn't filled out yet ( Every actual CI check passes ✅ — |
|
🤖 Still nothing to fix in code — the situation is unchanged from my previous comment. The one remaining ❌ is PR Reviewer Checklist / checklist, which fails only because the reviewer checklist hasn't been completed yet ( All actual CI checks pass ✅ ( |
|
PR doesn’t need product input as a refactor PR. Unassigning and unsubscribing myself. |
|
@MelvinBot Two code-fixes required: 🟡 Both sites still read Someone reading Rename both to 🟡 Test coverage of the diff: 0% No test file in the diff, and there is no existing jest.mock('@rnmapbox/maps', () => ({setAccessToken: jest.fn((token: string) => Promise.resolve(token))}));
// 1. returns false until the promise resolves
// 2. setAccessToken is NEVER called with '' when the token is cleared <- the actual regression guard
// 3. returns false again once the token is clearedAssertion 2 is the one that matters: without it, the next person who "simplifies" the early return reintroduces
|
…tests Co-authored-by: Kevin Brian Bader <ikevin127@users.noreply.github.com>
|
Both fixes are in 1. Renamed 2. Added a
The Verification
|
|
@danieldoglas Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
The only failing check is Check independent approval — and it's not a code problem, so there's nothing to push a fix for. Why it's failing: the This is a governance gate that clears once a qualifying reviewer approves the PR — it can't be resolved by changing code. Every other check is either passing or still running (tests, builds, perf, typecheck); none of them are failing. Next step: get an independent Expensify employee to review and approve, and this check will pass on the next run. |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
3 participants
Explanation of Change
HybridApp Android crashes with
MapboxConfigurationException(SentryAPP-HTR) when a nativeRNMBXMapViewis (re)constructed while the per-process Mapbox token global (MapboxOptions.accessToken) is blank.Root cause is entirely in
useAccessToken:isAccessTokenSetlatched totrueand never reset, so the map stayed mounted / mountable even after the token was cleared.accessToken={mapboxAccessToken?.token ?? ''}, so a runtime token-clear (expiry/refresh every 25 min, app-foreground, or reconnect) flipped the prop to''and calledsetAccessToken('').@rnmapbox/maps@10.3.2the native setter only null-guards (accessToken?.let { MapboxOptions.accessToken = it }), so an empty string clobbers the global with"". A Fabric preallocation of a freshRNMBXMapViewover that blank global then throws atMapView.<init>.The fix, in
src/components/MapView/useAccessToken.ts:accessTokenis falsy, sosetAccessToken('')is never called and the native global only ever holds a valid token.hasSetAccessToken && !!accessTokeninstead of a one-way latch. When the token is cleared at runtime the gate re-closes and consumers fall back toPendingMapViewinstead of leaving a map mounted (or letting a fresh nativeMapViewbe constructed) while the token is blank.Deriving the gate (rather than a synchronous
setState(false)inside the effect, as the proposal originally worded it) avoids areact-hooks/set-state-in-effectviolation while achieving the same re-close. This covers bothGPSMapViewandMapView, which both consumeuseAccessToken.The complementary native empty-guard is being taken upstream to
rnmapbox/mapsrather than carried as a localpatch-packagepatch, so it is intentionally not included in this PR.Fixed Issues
$ #97474
PROPOSAL: #97474 (comment)
Tests
Regression (happy path — all platforms):
+) → Create expense → Distance → open the map view.Crash fix (HybridApp Android — the reproducing platform):
Mobile-Expensify, sign in, then FAB → Create expense → Distance → GPS tab (distance-gps) so a native map is mounted.Onyx.set(ONYXKEYS.MAPBOX_ACCESS_TOKEN, null). Optionally toggle airplane mode ON right after to widen the window by blocking the refetch.MapboxConfigurationException. The map should fall back toPendingMapViewfor the brief refetch window and then return once a valid token is refetched.Offline tests
QA Steps
Same as the Tests section above. Prioritize HybridApp Android, since the
MapboxConfigurationExceptioncrash is Android-only (iOS Mapbox tolerates a blank token and recovers).PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Regression smoke-test on dev NewDot web — with a valid token the distance map renders normally (tiles, labels, marker), confirming the token-gating change does not break the happy path: