Remove OnyxUtils.get() from useUpdateGpsTripOnReconnect (use useOnyx) - #99391
Conversation
Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
Fixed the failing Oxfmt check and pushed to the branch. Cause: The PR added Fix: Reordered The only other red check, |
Web test results — PR #99391Result: What was verified
The refactored hook powers the app-wide Note on the console checkThe web test harness drives the app through the accessibility tree and screenshots; it does not have access to the browser devtools/console, so I couldn't attach a If a hard console-error guarantee is required for sign-off, a Playwright-based run with a ScreenshotsSee the attached evidence (Home load, Concierge chat, Create-expense/Scan flow, Home after navigation) posted alongside this comment. |
|
The real failure (Oxfmt) was already fixed by my earlier push (commit The only remaining red check is |
Android test results — PR #99391Result: ✅ Pass. On standalone Android NewDot, the PR branch ( What was verified
The refactored hook powers the app-wide Note on the console checkThis PR's only written test step is "Verify that no errors appear in the JS console." The pre-started session had no active JS console log stream, so I couldn't capture a raw console read on device — the absence of console errors is inferred from the complete absence of error UI, crash screens, or broken renders across every screen transition. That's consistent with a clean console but is not a direct capture of it. The ScreenshotsSee the attached evidence (Home load, Concierge chat, Create-expense Scan/Manual, Home after navigation) posted alongside this comment. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61f388ce22
ℹ️ 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".
| import {useEffect, useRef} from 'react'; | ||
|
|
||
| function useUpdateGpsTripOnReconnect({gpsPoints}: {gpsPoints: GPSPoint[][]}) { | ||
| const [gpsDraftDetails] = useOnyx(ONYXKEYS.GPS_DRAFT_DETAILS); |
There was a problem hiding this comment.
Gate reconnect writes on draft hydration
If the app starts offline with a persisted GPS trip and reconnects before this subscription reaches loaded, both gpsDraftDetails and the parent-provided fallback are initially empty. The empty Promise.all then completes immediately and updateGpsPoints([[]]) overwrites the persisted route; the parent checker already guards its startup logic with gpsDraftDetailsMetadata.status !== 'loaded' for this reason. Read the metadata here and defer the reconnect handler until the draft has hydrated.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Edge case enough to ignore. Not reproducible.
| useEffect(() => { | ||
| latestGpsDraftDetailsRef.current = gpsDraftDetails; | ||
| }, [gpsDraftDetails]); |
There was a problem hiding this comment.
Preserve an authoritative post-geocoding draft read
When GPS_DRAFT_DETAILS changes while reverse geocoding is pending, the Onyx notification can schedule a render without this passive effect having run before the promise continuation. In that window the ref still contains the previous route, so the later updateGpsPoints() replaces points recorded during geocoding—the race the removed post-await OnyxUtils.get() avoided. The merge needs an authoritative latest-value read or an atomic state update rather than relying on effect timing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Edge case enough to ignore. Not reproducible
|
No code fix needed — all code/CI checks are green now. The Oxfmt fix I pushed earlier ( The only two remaining red checks are not code checks and can't be resolved by a push:
Both are human/approval gates that clear once the PR gets a reviewer sign-off, not something a code change addresses. |
Code review — PR #99391Verdict: LGTM. The change is correct, minimal, and does exactly what the repo's own guidance asks — it drops the unsanctioned deep import Why the refactor is sound
Minor (non-blocking)
On testing thisWorth flagging for QA: this hook is native-only. The web build's |
Regression check — PR #99391No regressions observed on Web, and one narrow native-only regression risk found in code review.
Web regression pass — clean ✅Ran the PR branch on Web: Home, the Track-distance (map/waypoints) screen, Inbox/Concierge chat, Account, Workspaces, and Spend all render normally, and Home is unchanged after full round-trip navigation. No crash boundary, blank screen, or stuck loader from the always-mounted Native-only regression risk (code review) — narrow timing raceThe refactor slightly narrows (does not remove) the exact race the original code existed to close.
Evidence & confidence
Suggested verification / mitigation: exercise the reconnect-while-tracking flow on native (start a GPS trip, go offline, keep moving so points append, come back online mid-geocoding) and confirm no appended points are dropped. If tightening is wanted, assigning the ref during render is blocked by the React Compiler check the PR notes — a |
|
@MelvinBot add Tests / QA steps |
|
Here are drafted Tests / QA steps for this PR. I've left them as a comment rather than editing the Key thing to know first: this is a native-only feature. The behavior under test: while offline, GPS-tracked waypoint addresses are stored as raw coordinates (reverse geocoding needs the network); on reconnect the hook fetches human-readable addresses and writes them back using the latest Tests (iOS / Android native — dev)
QA Steps (staging)Same as Tests above, run on iOS: Native and Android: Native.
For reference, I already ran the web build on this branch and it loads and navigates cleanly with no visible error state (see my earlier comment) — but note that only covers the no-regression check, not the functional reconnect flow, which needs a native device. |
DetailsExplanation of Change
Since this is a React hook running in the render path, this PR reads the key through the supported The ref is synchronized in a Changes:
The rest of the merge logic (fallback to Fixed Issues$ #98608 Tests
Offline testsSame as Tests QA StepsN/A
PR Author Checklist
@MelvinBot please set the PR body to the content in the details section above. And mark as ready for review |
|
@thesahindia 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.
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movlog.movAndroid: mWeb ChromeiOS: HybridAppios.movfeed mock GPS location: Screen.Recording.2026-08-25.at.8.58.01.PM.moviOS: mWeb SafariMacOS: Chrome / Safari |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #98608 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
Valforte
left a comment
There was a problem hiding this comment.
@mkhutornyi in your videos you didn't follow the test steps described in the issue body. Specifically, the physically move, or feed mock GPS locations instructions. Can you please record with that included or update the test steps if that's not necessary.
Also in the iOS recording I could not see the waypoints automatically update from coordinates to human-readable addresses behavior.
Here's video: Screen.Recording.2026-08-25.at.8.58.01.PM.mov
For some reason, this doesn't work on iOS at all for me. This also happens on (Tested with same coordinate on both android/ios devices) Screen.Recording.2026-08-25.at.8.53.47.PM.mov |
|
🚧 Valforte has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/Valforte in version: 9.4.62-1 🚀
|
Help site review: no changes requiredI reviewed this PR against the help site content in Why: This is a purely internal code refactor. It swaps an unsanctioned Onyx read ( There is:
Since nothing user-facing changed, no draft help site PR was created. @mkhutornyi, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.4.62-4 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
src/components/GPSTripStateChecker/useUpdateGpsTripOnReconnect.tsreadGPS_DRAFT_DETAILSvia the internal deep importreact-native-onyx/dist/OnyxUtils(OnyxUtils.get(...)), which is not a sanctioned Onyx read API and couples the hook to Onyx internals.Since this is a React hook running in the render path, this PR reads the key through the supported
useOnyx()API instead. The originalOnyxUtils.get()call happened inside the asynconReconnecthandler after awaiting reverse geocoding, specifically to grab the freshestgpsDraftDetailsand avoid a race (geocoding can take a few seconds, during which the draft may change). To preserve that guarantee, theuseOnyxvalue is mirrored into a ref so the post-awaitread still reflects the newest value.The ref is synchronized in a
useEffectrather than assigned during render — assigning a ref in the render body fails the React Compiler compliance check (Cannot access refs during render) on both the Babel and OXC compilers, and this file is currently memoized by both, so a render-body assignment would regressreact-compiler-compliance-check. The effect commits well before the geocoding promise resolves, so the race protection is preserved.Changes:
import OnyxUtils from 'react-native-onyx/dist/OnyxUtils'.const [gpsDraftDetails] = useOnyx(ONYXKEYS.GPS_DRAFT_DETAILS)and mirrored it intolatestGpsDraftDetailsRefvia auseEffect.OnyxUtils.get(...)read withlatestGpsDraftDetailsRef.current.The rest of the merge logic (fallback to
gpsPoints, patching start/end addresses, and the trimmed-endpoint update) is unchanged.Fixed Issues
$ #98608
Tests
37.7749,-122.4194), not street addresses.gpsDraftDetails.Offline tests
Same as Tests
QA Steps
N/A
General regression tests should cover this
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.