Remove OnyxUtils.get() from GPSTripStateChecker (use useOnyx) - #98703
Remove OnyxUtils.get() from GPSTripStateChecker (use useOnyx)#98703MelvinBot wants to merge 1 commit into
Conversation
…tion Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
I checked the CI on this PR — there's no code-related failure to fix. When your request came in, several checks ( The only remaining red check is Summary: all build/lint/type/test/perf checks are green; nothing to patch. This PR just needs a human review + approval to clear the last check. |
|
@codex review |
|
@MelvinBot review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4f4ddd6a0
ℹ️ 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".
| const gpsTrip = await OnyxUtils.get(ONYXKEYS.GPS_DRAFT_DETAILS); | ||
|
|
||
| if (!gpsTrip?.isTracking) { | ||
| if (!gpsDraftDetails?.isTracking) { |
There was a problem hiding this comment.
Re-read GPS state after notification cleanup
On iOS, checkAndCleanGpsNotification() awaits liveActivityManager.listAll(), so this closure can become stale while it is suspended. If the hydrated state is not tracking and the user starts or resumes a trip before that call finishes, this branch still sees the old false value, detects the newly started background task, and stops it while Onyx remains isTracking: true, causing the trip to stop collecting locations. The removed OnyxUtils.get() ran after this await and observed the latest state; retain the latest subscribed value in a ref or otherwise verify the current tracking state after cleanup before stopping the task.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is not something introduced by this PR so I'd leave as is.
@MelvinBot confirm
There was a problem hiding this comment.
Mostly agree it's fine to leave as-is, with one nuance so the call is made with eyes open.
The check-then-act race itself is pre-existing. Even before this PR, once the handler read isTracking: false there was still an await (hasStartedLocationUpdatesAsync, index.native.tsx:51) before stopLocationUpdatesAsync, index.native.tsx:53 — a trip started in that gap would have been wrongly stopped too. So the class of race isn't new.
But strictly, this PR does slightly widen the window. The old OnyxUtils.get() read after await checkAndCleanGpsNotification(), whereas the new code reads the closure snapshot captured before it (index.native.tsx:48→50). On iOS that cleanup awaits liveActivityManager.listAll(), index.ios.ts:165, so that native call is now inside the stale window where it wasn't before. Codex's mechanism is technically valid.
Practically, the risk is negligible — so I'm fine leaving it. This is the once-per-mount app-restart handler; the dangerous branch only runs when there's no active trip on launch, and it would require the user to start a brand-new trip during a sub-second startup async window (the continue-trip modal is itself gated behind the splash screen being hidden). Not a realistic sequence.
If you ever want exact parity with the old read-after-cleanup semantics, the minimal change is to read the latest subscribed value from a ref after await checkAndCleanGpsNotification() instead of the pre-await snapshot — but that's optional and arguably outside this refactor's scope. 👍 to leaving as-is.
Review — LGTM ✅The change faithfully implements the approved proposal, meets every acceptance criterion, and all build/lint/type/test/perf checks are green. What's correct
Behavioral-equivalence check (the one thing worth verifying)The old code read the value via Testing note: this component is native-only — Nothing blocking from me. |
|
Both failing checks are approval/process gates, not code failures — there's nothing to fix in code.
Every actual CI check is green: Bun tests, all Both gates clear once this PR gets a reviewer and an independent approval — no push needed from me. |
Regression hunt — none found ✅I went through the diff adversarially against the old behavior. The change is behaviorally equivalent to the pre-PR code for every app-restart path, and all CI checks are green. Details on each candidate I checked below. Regression candidates checked & ruled out1. Value read moved before the 2. Gating on 3. One-shot semantics / double-fire 4. Snapshot correctness 5. Value shape On testing: this component is native-only — No regressions from me. |
Tests
Offline tests
QA StepsRun tests 1–5 above on iOS: Native and Android: Native. For Android: mWeb Chrome, iOS: mWeb Safari, and MacOS: Chrome / Safari: this component renders Regression areas to watch: GPS distance expense creation end-to-end (start → record → stop → save), account switching and delegate switching while a trip is in progress (the "trip in progress" warning modal should still appear and still stop the trip), and switching to Expensify Classic while a trip is in progress. |
|
Still the same two checks failing, and both are approval/process gates — not code failures, so there's nothing for me to push:
Every real CI check (Bun tests, all |
|
Still nothing to fix here — the only two red checks are approval/process gates, not code failures:
All actual CI (Bun tests, every |
DetailsExplanation of Change
This change removes the deep import and serves the app-restart check from the existing
Note: this component is native-only — the web build ( Fixed Issues$ #98609 Tests
Offline testsSame as Tests QA StepsSame as Tests
PR Author Checklist
@MelvinBot please set the PR body to the content in the details section above |
|
✅ Done — I set the PR body to the content you provided (Explanation of Change, Fixed Issues, Tests/Offline/QA, and the completed PR Author Checklist). |
|
@MelvinBot mark as ready for review |
|
@mkhutornyi 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 — this PR is now marked Ready for review. ✅ One thing to flag: the Tests and QA Steps sections are still placeholders. As the human co-author, please fill those in (suggested native repro steps are already stubbed in the description) so reviewers know exactly what was validated before approval. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb ChromeiOS: HybridAppios.movprod.moviOS: mWeb SafariMacOS: Chrome / Safari |
|
Nothing changed since my last comment — the only failing check is All real CI checks (tests, typecheck, ESLint, builds, perf) are green, so there's no code for me to fix or push here. |
Explanation of Change
OnyxUtils.get()(imported from the internal deep pathreact-native-onyx/dist/OnyxUtils) is not a sanctioned way to read Onyx data.GPSTripStateChecker/index.native.tsxwas using it inside its mount effect to re-readGPS_DRAFT_DETAILS, even though the component already subscribes to the same key viauseOnyx(ONYXKEYS.GPS_DRAFT_DETAILS).This change removes the deep import and serves the app-restart check from the existing
useOnyxsubscription:import OnyxUtils from 'react-native-onyx/dist/OnyxUtils'and theOnyxUtils.get(...)call.const [gpsDraftDetails, gpsDraftDetailsMetadata] = useOnyx(...)) and gate the restart effect ongpsDraftDetailsMetadata.status === 'loaded'. Gating on the loaded status (rather than reading the value eagerly) matters so we don't misread the not-yet-hydrated state as "no trip" and wrongly stop an in-progress trip's background task.useRefflag so the restart handler still runs exactly once per mount, even though the subscription value can change later in the session.[]-dependencyuseEffectso it still runs only on unmount.Note: this component is native-only — the web build (
index.tsx) rendersnull— so this change has no effect on web.Fixed Issues
$ #98609
PROPOSAL: #98609 (comment)
Tests
Offline tests
Same as Tests
QA Steps
Same as Tests
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.