Repository navigation
[Batch 6] Migrate GPS trip ConfirmModal usages to useConfirmModal - #102013
Conversation
Batch 6: the three user-triggered confirms in GPSButtons plus the two Onyx-driven controllers, GPSInProgressModal and GPSTripStateChecker. The controllers read gpsDraftDetails and isOffline through refs so a trip answered after points kept accumulating is submitted in full rather than truncated to its show-time state. GPSTripStateChecker names its entry on the modal stack and closes it by ID when tracking stops, the trip changes owner, or the splash screen comes back, which the old derived isVisible did for free. Its cancel stops the trip rather than dismissing the prompt, so it opts out of shouldHandleNavigationBack and distinguishes an auto-close from a real answer. Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com>
|
I created this PR from an upstream branch because I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
|
|
|
🤖 Web test steps verification results: ❌ could not run I couldn't produce screenshots or recordings for this PR on Web, for two independent reasons. 1. There are no documented testing steps yet. The 2. Nothing this PR changes renders on Web. Even with steps and a working browser, a Web run would exercise none of the migrated code:
Both null-rendering Web variants are the files I also could not drive a browser in this run at all — Web prep failed with no dev SSL certificate, so no browser-based verification was possible even for the generic console check. What would actually verify this: an Android or iOS native run. This PR carries the Unrelated but blocking: the |
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.
|
Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com>
|
Fixed and pushed — What was failing: The one check still red is Local verificationRun from the
Note: a bare This is a native-only surface and browser control was unavailable in this environment, so nothing here was verified on a device or in a browser. Everything above is static analysis and the automated checks. |
| buttonVariant={CONST.BUTTON_VARIANT.DANGER} | ||
| /> | ||
| ); | ||
| return null; |
There was a problem hiding this comment.
@MelvinBot
Let's refactor this to use hooks instead of empty components
There was a problem hiding this comment.
Done in 1f9471f. Both null-rendering components are now hooks, called from AuthScreens:
GPSInProgressModal→useGPSInProgressModalGPSTripStateChecker→useGPSTripStateChecker, with its helper hooks moved alongside it- Each keeps a no-op
index.tsfor web, likeuseContactPermissionModal - Called in
AuthScreens.tsx:140-141
The hook bodies are unchanged. The test moved to tests/unit/hooks/useGPSTripStateChecker.test.tsx and uses renderHook.
One trade-off: AuthScreens now owns the GPS_DRAFT_DETAILS subscription, so it re-renders on each GPS point while a trip is tracking. React Compiler memoizes its JSX, so the navigator tree should bail out, but you may want to confirm that on a device.
Checks run
npm testforuseGPSTripStateChecker.test.tsxandHybridAppActionsTest.ts: 20/20 passednpm run typecheck: passednpm run linton the changed files: cleanreact-compiler-compliance-checkon the changed files: cleannpm run fmt: no diff
I have not tested this on a device.
| }); | ||
| }, [closeModalByID, onContinueTrip, onViewTrip, shouldShowContinueTripModal, showConfirmModal, translate]); | ||
|
|
||
| return null; |
There was a problem hiding this comment.
@MelvinBot
Let's refactor this to use hooks instead of empty components
There was a problem hiding this comment.
Done in 1f9471f. This is now useGPSTripStateChecker, called from AuthScreens.tsx:140. See the other thread for the details and the re-render trade-off.
GPSTripStateChecker and GPSInProgressModal rendered nothing and only existed to run effects, so they become useGPSTripStateChecker and useGPSInProgressModal, called from AuthScreens. The web variants stay no-ops. Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com>
|
@MelvinBot |
…ConfirmModals Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com> # Conflicts: # config/eslint/eslint.seatbelt.tsv # src/pages/iou/request/step/IOURequestStepDistanceGPS/GPSButtons/index.tsx
|
Merged
Lint, typecheck, and the React Compiler check pass on the changed file. |
|
@MelvinBot |
|
@codex |
PR ReviewThe migration looks correct, but please look at one performance issue before merging. Two smaller items follow. 1. Before this PR, the two null-rendering components kept their While a trip is running, the app writes to that key every 50 m ( You can fix this in either of these ways:
2. The continue-trip prompt doesn't come back after an auto-close (minor) When the prompt auto-closes, 3. The PR description is out of date It still describes What I checked
|
|
@MelvinBot |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cec12da2c4
ℹ️ 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".
| useGPSTripStateChecker(); | ||
| useGPSInProgressModal(); |
There was a problem hiding this comment.
Release global GPS prompts when AuthScreens unmounts
When the session changes while either GPS prompt is open (for example, logout or session expiry), AppNavigator.native.tsx unmounts AuthScreens and renders PublicScreens, but the app-level ModalProvider keeps its modal stack alive. These hooks do not remove their entries on unmount, so the old continue-trip or switch-to-OldDot prompt remains over the public flow; answering it can then run stale trip/navigation actions after the authenticated UI has gone away. The previous inline modal components disappeared with AuthScreens.
Useful? React with 👍 / 👎.
|
I updated the PR description with test steps and checked the checklist items I can confirm.
I left the device-testing items unchecked because nobody has run these steps on a device yet: offline, high-traffic account, screenshots, all platforms, console errors, impacted components, deeplink, and retest after merging I also fixed stale file paths in the Explanation, because the controllers moved to |
|
@MelvinBot |
…unts Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com>
|
Added your changes in eee12aa. The diff applied cleanly to both hooks:
Lint, typecheck, the React Compiler check, and |
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
|
@MelvinBot |
|
I couldn't mark this PR as ready for review. The command that does it needs approval, and this run doesn't have it. The PR is still a draft. To open it, click Ready for review at the bottom of the PR page. The branch is up to date with |
|
@MelvinBot listen to @ZhenjaHorbach please 😂 |
| }); | ||
| await waitForBatchedUpdatesWithAct(); | ||
|
|
||
| // Then the trip is submitted with the points as they stand now; reading the show-time value would truncate it |
There was a problem hiding this comment.
❌ CONSISTENCY-15 (docs)
This comment uses a semicolon to join two independent sentences, which the plain-comment style avoids. Split it into two sentences.
// Then the trip is submitted with the points as they stand now. Reading the show-time value would truncate itReviewed at: 4dc06db | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4dc06dbb34
ℹ️ 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 isTripStopped = isTripStoppedUtil(gpsDraftDetails); | ||
|
|
||
| const showDisabledServicesModal = () => { | ||
| showConfirmModal({ |
There was a problem hiding this comment.
Prevent duplicate GPS prompts from repeated presses
When location services are disabled, two taps on Resume/Start can both finish the awaited hasServicesEnabledAsync() check and call this function before any modal is dismissed. Because this global-modal call has neither an ID nor an open guard, ModalProvider creates a separate stack entry for each invocation, whereas the previous boolean state could only render one modal; dismissing the first Location services prompt then immediately exposes another. The same unguarded pattern also affects the zero-distance acknowledgement. Track whether each prompt is already open (or otherwise suppress repeat calls) before showing it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c9cb70d. All three GPSButtons prompts now go through one showPrompt helper. It skips the call while a prompt is already open, then clears the flag when the prompt resolves (GPSButtons/index.tsx:64-83).
This matches the "is open" ref guard already used in useGPSInProgressModal and useGPSTripStateChecker. I didn't use a stable modal id alone. A repeat call with the same id returns the same promise, so the confirm action (opening Settings) would still run once per press.
|
Everything
The PR Author Checklist still has unchecked items that need a person, such as screenshots and test runs on each platform. |
|
@MelvinBot |
…ment Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com>
|
@roryabraham 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] |
|
I fixed the last two review comments in c9cb70d.
Lint, typecheck, the React Compiler check, and |
|
No product review needed. |
|
@MelvinBot |
…ConfirmModals Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com> # Conflicts: # config/eslint/eslint.seatbelt.tsv
|
I fixed the merge conflicts by merging The only conflict was in
Main made no changes to the GPS files this PR moves or edits. I couldn't run ESLint locally because it ran out of memory, so CI will confirm lint. |
|
🚧 roryabraham 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/roryabraham in version: 9.5.2-0 🚀
|
|
No help site update needed. This PR moves the GPS trip modals to |
|
Deploy Blocker #102961 was identified to be related to this PR. |
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.5.2-4 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
Batch 6 of the
ConfirmModal→useConfirmModal()migration: the five GPS trip modals.ConfirmModalis deprecated, so each instance moves off the inline-rendered component and onto the global modal stack.GPSButtons/index.tsx— three user-triggered confirmsZero-distance acknowledgement, "location required", and "location services disabled" are all straightforward: a press handler calls
showConfirmModal()and acts onModalActions.CONFIRM. The threeuseStatevisibility flags are gone, since the stack now owns visibility.useGPSInProgressModal/index.native.ts(wasGPSInProgressModal/index.native.tsx) — null-rendering controllerThe prompt is asked for by
closeReactNativeApp, a non-React action that cannot call a hook, so theIS_GPS_IN_PROGRESS_MODAL_OPENOnyx flag stays as the cross-boundary trigger and the component becomes a controller hook that turns the flag into ashowConfirmModal()call from auseEffectbehind auseRefguard. Nothing other than the user's answer clears the flag, so the entry never has to be taken down by hand.useGPSTripStateChecker/index.native.ts(wasGPSTripStateChecker/index.native.tsx) — the hard oneIts
isVisiblewas a four-term derived expression, and three of those terms can flip while the prompt is open, which used to hide it for free. Three things had to be handled explicitly:closeModal(), becausecloseModal()pops whatever sits on top and would take an unrelated modal down if one had been opened over this one.onCancel={onViewTrip}stops the trip. The wrapper collapses cancel and backdrop press into a singleCLOSE, andshouldHandleNavigationBacksilently defaultsundefined→true, which would have routed Android hardware-back into "stop the trip". It is now passed explicitly asfalse. AuseRefflag also tells an auto-close apart from a real answer, so the auto-hide above cannot be mistaken for the user choosing to stop the trip.getGpsPoints(gpsDraftDetails)read the current render's Onyx value, but a.then()continuation captures whatever the value was at show time. GPS points keep accumulating intoGPS_DRAFT_DETAILSwhile tracking, so a trip answered minutes later would have been submitted truncated. Both controllers now readgpsDraftDetailsandisOfflinethrough refs inside the continuation.Both controllers are now called as hooks from
AuthScreensinstead of being rendered there as components.The
no-deprecated/ConfirmModalseatbelt entries for all three files drop out ofconfig/eslint/eslint.seatbelt.tsv(1 + 1 + 3 = the five instances in this batch).Tests
tests/unit/hooks/useGPSTripStateChecker.test.tsxreplacestests/unit/components/GPSTripStateCheckerTest.tsx: the prompt no longer renders in the page-local tree, so the suite mocksuseConfirmModalwith the existingtests/utils/mockUseConfirmModalhelper and asserts on what was pushed. Cases cover the traps above — that nothing is pushed behind the splash screen, thatshouldHandleNavigationBackisfalse, that the entry is closed by ID without stopping the trip when tracking stops, and that a trip stopped by the user submits every point recorded up to the moment they answered rather than the show-time set.Note
This is a native-only surface, and browser verification was unavailable in this environment, so the changes below are verified by the automated checks and unit tests only. They have not been exercised on a device.
Fixed Issues
$ #100156
PROPOSAL:
Tests
GPS tracking is native-only. Run these on Android and iOS native.
Zero-distance trip (
GPSButtons)Location permission denied (
GPSButtons)Location services disabled (
GPSButtons)Continue trip after the app closes (
useGPSTripStateChecker)Switch to Expensify Classic during a trip (
useGPSInProgressModal, HybridApp only)Offline tests
QA Steps
Same as tests, on Android and iOS native.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari